Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

portfwd: Fix handling the last UDP message #2971

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

nirs
Copy link
Member

@nirs nirs commented Dec 3, 2024

PacketCon.ReadFrom documentation says[1]:

Callers should always process the n > 0 bytes returned before
considering the error err.

But we handled err first, and dropped the last read bytes. Fixed by sending a message if n > 0, and handling the error after the send.

I'm not sure what how this bug affects the system. It was found while reviewing #2969.

stream.Recv() does is not documented, but the trivial implementation ensure that we get nil message on any error. Add comment to make it more clear.

[1] https://pkg.go.dev/net#PacketConn

Fixes #2970

Thanks: Tamir Duberstein [email protected]

PacketCon.ReadFrom documentation says[1]:

> Callers should always process the n > 0 bytes returned before
> considering the error err.

But we handled err first, and dropped the last read bytes. Fixed by
sending a message if n > 0, and handling the error after the send.

I'm not sure what how this bug affects the system. It was found while
reviewing lima-vm#2969.

stream.Recv() does is not documented, but the trivial implementation
ensure that we get nil message on any error. Add comment to make it more
clear.

[1] https://pkg.go.dev/net#PacketConn

Fixes lima-vm#2970

Thanks: Tamir Duberstein <[email protected]>
Signed-off-by: Nir Soffer <[email protected]>
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@nirs
Copy link
Member Author

nirs commented Dec 4, 2024

Thanks, LGTM

Note that I did not test yet, trying to setup UDP port forwarding now...

@nirs

This comment was marked as outdated.

@nirs nirs marked this pull request as draft December 4, 2024 01:46
@nirs
Copy link
Member Author

nirs commented Dec 6, 2024

@balajiv113 is this fixed in tcpproxy that you want to use in #2985?

@nirs
Copy link
Member Author

nirs commented Dec 7, 2024

I tested this change and latest release (1.0.2) to make sure this is not a regression introduced recently and I see the same behavior.

  • ssh forwarder: ok
    • iperf3: ok
    • copy file: ok
    • disconnect on host: detected in guest
  • grpc forwarder/tcp:
    • iperf3: ok
    • copy file: ok
    • disconnect on host: not detected in guest
  • grpc forwarder/udp:
    • iperf3: cannot test since udp usage is dynamic
      • @balajiv113 suggested a hack to listen to both tcp and udp for testing, I did not try yet
    • typing: nc disconnects after sending 2-3 lines to guest, or after few seconds
    • disconnect on host: not detect in guest - I think this is expected, since there is not connection involved, there is no way the host or the guest can detect a disconnect
    • copy file small: ok
    • copy file big: not all data transferred, maybe because port forwarding closed, or maybe macOS dropped packet because buffer space is not available (ENOBUFS).

Created test vm

_output/bin/limactl create --tty=0 --name=test

Enable UDP port forwarding:

portForwards:
 - guestIP: "127.0.0.1"
   hostIP: "127.0.0.1"
   proto: any

Testing SSH forwarder

setup:

_output/bin/limactl start test
_output/bin/limactl shell test sudo apt-get update
_output/bin/limactl shell test sudo apt-get install iperf3
_output/bin/limactl shell test sudo systemctl enable iperf3 --now

iperf3 (host to vm):

```console
% iperf3 -c 127.0.0.1     
Connecting to host 127.0.0.1, port 5201
[  5] local 127.0.0.1 port 63456 connected to 127.0.0.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec   319 MBytes  2.68 Gbits/sec                  
[  5]   1.00-2.00   sec   315 MBytes  2.64 Gbits/sec                  
[  5]   2.00-3.00   sec   314 MBytes  2.64 Gbits/sec                  
[  5]   3.00-4.00   sec   300 MBytes  2.52 Gbits/sec                  
[  5]   4.00-5.00   sec   323 MBytes  2.71 Gbits/sec                  
[  5]   5.00-6.00   sec   318 MBytes  2.67 Gbits/sec                  
[  5]   6.00-7.00   sec   301 MBytes  2.52 Gbits/sec                  
[  5]   7.00-8.00   sec   313 MBytes  2.62 Gbits/sec                  
[  5]   8.00-9.00   sec   310 MBytes  2.60 Gbits/sec                  
[  5]   9.00-10.00  sec   318 MBytes  2.66 Gbits/sec                  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-10.00  sec  3.06 GBytes  2.63 Gbits/sec                  sender
[  5]   0.00-10.01  sec  3.05 GBytes  2.62 Gbits/sec                  receiver

copy file - guest:

$ nc -l 127.0.0.1 6666 > test.data
(nc exits after file was host disconnected)
$ sha256sum test.data 
64e7339a0293f65863dbc88a052329e0b0666fffbc081c8e02cf90123a1aa20c  test.data

copy file - host:

% time nc 127.0.0.1 6666 < ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz
nc 127.0.0.1 6666 < ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz  0.02s user 0.20s system 90% cpu 0.254 total
% sha256sum ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz 
64e7339a0293f65863dbc88a052329e0b0666fffbc081c8e02cf90123a1aa20c  /Users/nsoffer/Downloads/lima-1.0.1-Darwin-arm64.tar.gz

Testing GRPC forwarder

setup:

LIMA_SSH_PORT_FORWARDER=false _output/bin/limactl start test

TCP

iperf3 (host to vm):

 % iperf3 -c 127.0.0.1                                                
Connecting to host 127.0.0.1, port 5201
[  5] local 127.0.0.1 port 63509 connected to 127.0.0.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec   454 MBytes  3.81 Gbits/sec                  
[  5]   1.00-2.00   sec   466 MBytes  3.91 Gbits/sec                  
[  5]   2.00-3.00   sec   468 MBytes  3.93 Gbits/sec                  
[  5]   3.00-4.00   sec   467 MBytes  3.92 Gbits/sec                  
[  5]   4.00-5.00   sec   464 MBytes  3.89 Gbits/sec                  
[  5]   5.00-6.00   sec   409 MBytes  3.43 Gbits/sec                  
[  5]   6.00-7.00   sec   439 MBytes  3.68 Gbits/sec                  
[  5]   7.00-8.00   sec   448 MBytes  3.75 Gbits/sec                  
[  5]   8.00-9.00   sec   466 MBytes  3.91 Gbits/sec                  
[  5]   9.00-10.00  sec   459 MBytes  3.85 Gbits/sec                  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-10.00  sec  4.43 GBytes  3.81 Gbits/sec                  sender
[  5]   0.00-10.00  sec  4.43 GBytes  3.81 Gbits/sec                  receiver

copy file - guest:

$ nc -l 127.0.0.1 6666 > test.data
^C
$ sha256sum test.data 
64e7339a0293f65863dbc88a052329e0b0666fffbc081c8e02cf90123a1aa20c  test.data

nc did not exit, had to interrupt it

copy file - host:

% time nc 127.0.0.1 6666 < ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz
^C
nc 127.0.0.1 6666 < ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz  0.02s user 0.16s system 1% cpu 14.849 total

nc did not exit, had to interrupt it

UDP

guest:

$ nc -u -l 127.0.0.1 6666
line 1
line 2

nc received only 2 lines

host

% nc -u 127.0.0.1 6666                                          
line 1
line 2
line 3

nc terminated after typing the third line.

ha.stderr.log:

{"level":"debug","msg":"guest agent event: local_ports_added:{protocol:\"udp\"  ip:\"127.0.0.1\"  port:6666}","time":"2024-12-07T02:01:37+02:00"}
{"level":"info","msg":"Forwarding UDP from 127.0.0.1:6666 to 127.0.0.1:6666","time":"2024-12-07T02:01:37+02:00"}
{"level":"debug","msg":"guest agent event: local_ports_removed:{protocol:\"udp\"  ip:\"127.0.0.1\"  port:6666}","time":"2024-12-07T02:02:01+02:00"}
{"level":"debug","msg":"removing listener for hostAddress: 127.0.0.1:6666, guestAddress: 127.0.0.1:6666","time":"2024-12-07T02:02:01+02:00"}
{"level":"debug","msg":"Port forwarding closed proto:udp host:127.0.0.1:6666 guest:127.0.0.1:6666","time":"2024-12-07T02:02:01+02:00"}

Looks like port forwarding was closed while the guest was listening.

Also host and guest agent clocks are not synchronized.

copy file small - guest:

$ nc -u -l 127.0.0.1 6666 > test.data
^C
$ sha256sum test.data 
289cc81bd1c1d86c89031bed62c3498e89124690794b42857a3d5ca86e89443b  test.data

nc did not exit, had to interrupt it.

copy file small - host:

% echo "testing small payload" | nc -u 127.0.0.1 6666 
^C
% echo "testing small payload" | sha256sum           
289cc81bd1c1d86c89031bed62c3498e89124690794b42857a3d5ca86e89443b  -

nc did not exit, had to interrupt it.

copy file big - guest:

$ nc -u -l 127.0.0.1 6666 > test.data
^C

$ ls -lh test.data 
-rw-rw-r-- 1 nsoffer nsoffer 28M Dec  7 02:19 test.data
$ sha256sum test.data 
0af76d53fef7e3b8c66d36df36db6117224f8291d276b55dba00d82fd8d8bb85  test.data

nc did not exit, had to interrupt it.

copy file small - host:

% nc -u 127.0.0.1 6666 < ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz
^C
% sha256sum ~/Downloads/lima-1.0.1-Darwin-arm64.tar.gz
64e7339a0293f65863dbc88a052329e0b0666fffbc081c8e02cf90123a1aa20c  /Users/nsoffer/Downloads/l

nc did not exit, had to interrupt it.

ha.stderr.log:

{"level":"debug","msg":"guest agent event: local_ports_added:{protocol:\"udp\"  ip:\"127.0.0.1\"  port:6666}","time":"2024-12-07T02:19:25+02:00"}
{"level":"info","msg":"Forwarding UDP from 127.0.0.1:6666 to 127.0.0.1:6666","time":"2024-12-07T02:19:25+02:00"}
{"level":"debug","msg":"guest agent event: local_ports_removed:{protocol:\"udp\"  ip:\"127.0.0.1\"  port:6666}","time":"2024-12-07T02:19:49+02:00"}
{"level":"debug","msg":"removing listener for hostAddress: 127.0.0.1:6666, guestAddress: 127.0.0.1:6666","time":"2024-12-07T02:19:49+02:00"}
{"level":"debug","msg":"Port forwarding closed proto:udp host:127.0.0.1:6666 guest:127.0.0.1:6666","time":"2024-12-07T02:19:49+02:00"}

port forwarding seems to close at the middle of the transfer.

@nirs nirs marked this pull request as ready for review December 7, 2024 02:11
@nirs
Copy link
Member Author

nirs commented Dec 7, 2024

@balajiv113 can you review? this conflicts with #2985 but I think it will take time until it is ready, and we can release this fix in 1.0.3.

Copy link
Member

@balajiv113 balajiv113 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

But do we have a usecase which this change fixes ???

@nirs
Copy link
Member Author

nirs commented Dec 8, 2024

But do we have a usecase which this change fixes ???

No, I could not reproduce the issue. This came from: https://github.com/lima-vm/lima/pull/2969/files#r1868401701

@AkihiroSuda AkihiroSuda added this to the v1.0.3 milestone Dec 10, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit 0d9572b into lima-vm:master Dec 10, 2024
29 checks passed
@nirs nirs deleted the portfwd-fix-last-message branch December 10, 2024 08:08
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Dec 28, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [lima-vm/lima](https://github.com/lima-vm/lima) | patch | `v1.0.2` -> `v1.0.3` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>lima-vm/lima (lima-vm/lima)</summary>

### [`v1.0.3`](https://github.com/lima-vm/lima/releases/tag/v1.0.3)

[Compare Source](lima-vm/lima@v1.0.2...v1.0.3)

#### Changes

-   QEMU:
    -   Support Apple M4 ([#&#8203;3032](lima-vm/lima#3032))
-   gRPC port forwarder:
    -   Improvements on stability ([#&#8203;2971](lima-vm/lima#2971), [#&#8203;2985](lima-vm/lima#2985), etc. thanks to [@&#8203;nirs](https://github.com/nirs) [@&#8203;balajiv113](https://github.com/balajiv113))
-   Templates:
    -   `archlinux`: allow 9p again ([#&#8203;3048](lima-vm/lima#3048))
    -   `centos-stream-10`: New template ([#&#8203;3023](lima-vm/lima#3023), [#&#8203;3047](lima-vm/lima#3047), thanks to [@&#8203;afbjorklund](https://github.com/afbjorklund))
    -   `opensuse`: disable virtiofs due to the lack of the kernel module in the default installation ([#&#8203;3056](lima-vm/lima#3056))
    -   Updated to the latest revisions ([#&#8203;3043](lima-vm/lima#3043))

Full changes: https://github.com/lima-vm/lima/milestone/52?closed=1
Thanks to [@&#8203;PascalBourdier](https://github.com/PascalBourdier) [@&#8203;afbjorklund](https://github.com/afbjorklund) [@&#8203;alexandear](https://github.com/alexandear) [@&#8203;balajiv113](https://github.com/balajiv113) [@&#8203;cpick](https://github.com/cpick) [@&#8203;jandubois](https://github.com/jandubois) [@&#8203;nirs](https://github.com/nirs) [@&#8203;olamilekan000](https://github.com/olamilekan000)

#### Usage

```console
[macOS]$ limactl create
[macOS]$ limactl start
...
INFO[0029] READY. Run `lima` to open the shell.

[macOS]$ lima uname
Linux
```

***

The binaries were built automatically on GitHub Actions.
The build log is available for 90 days: https://github.com/lima-vm/lima/actions/runs/12517401436

The sha256sum of the SHA256SUMS file itself is `69423d9f9044fc9264925d24cd38c1d0efb4367cfb46c568313f53d6f0ed7ee2` .

***

Release manager: [@&#8203;AkihiroSuda](https://github.com/AkihiroSuda)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS44My4zIiwidXBkYXRlZEluVmVyIjoiMzkuODMuMyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

portfwd: Read bytes ignored on is io.EOF returned from ReadFrom
4 participants