Add bhyve uart raw tcp backend

[aokblast]

Summary

This is a updated patch from this old patch according to the modification from markj on uart part of bhyve.

This patch add raw tcp connection ability on bhyve and has been tested on my own machine.

An example about this is as following:

bhyve -c 2 -m 4G -w -H \
                                       -s 0,hostbridge \
                                       -s 1,virtio-blk,./FreeBSD-14.0-STABLE-amd64-20240118-cef433d3fb38-266364.raw \
                                       -s 30,xhci,tablet \
                                       -s 31,lpc -l com1,tcp=127.0.0.1:8085 \
                                       -l bootrom,/usr/local/share/uefi-firmware/BHYVE_UEFI.fd \
                                        uefivm

Then we can use nc by:

netcat 127.0.0.1 8085

[markj]

usr.sbin/bhyve/uart_backend.c

308

conn_fd is leaked in the error path.

316

[markj]

usr.sbin/bhyve/bhyve.8

1125–1126

Also, 0.0.0.0 is not a valid address. Here it really means, "bind to a wildcard address". Probably 127.0.0.1 is a better example.

1135

Above, the syntax is documented as tcp:0.0.0.0:1234, but here it's tcp=0.0.0.0:1234. Which one is right?

1141

usr.sbin/bhyve/uart_backend.c

79

uart_tcp_softc would be a better name in my opinion. ("softc" is an abbreviation for "software context", very common in BSD code.)

435

Discussing in person, since other backends return errors, we should do the same here too.

455

You probably want to set the SO_REUSEADDR socket option. Otherwise, if I use the TCP console and reboot the VM, the previous connection will linger and I hit this error.

Also, just use warn() here, instead of warnx(). Then strerror(errno) is automatically appended to the warning. That is, just write: warn("bind()");.

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

Fix it!!

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[corvink]

usr.sbin/bhyve/uart_backend.c

313

This won't unlock sc->mtx.

537

Isn't it a race condition when setting is_socket after adding the mevent handler? If the mevent fires very fast, is_socket might be unset. It's very unlikely to hit the race condition but we should avoid it.

[aokblast]

• Add bhyve uart tcp backend
• Add manual page

[aokblast]

Prevent race condition in is_socket

• Add bhyve uart tcp backend
• Add manual page

[corvink]

Is this ready? So far, LGTM.

usr.sbin/bhyve/uart_backend.c

334

Have you addressed this? We should lock sc->mtx on uart_tcp_disconnect, shouldn't we?

483

I'm unfamiliar with capsicum. Do we have to make sure to limit the rights of bind_fd before this mevent fires?

[lwhsu]

In D45120#1060064, @corvink wrote:

Is this ready? So far, LGTM.

We're still testing this with OpenStack and plan to push it once it completes.

[aokblast]

usr.sbin/bhyve/uart_backend.c

334

One reason is that we don't use recursive lock so the caller should take the responsibility to lock it. Also, the caller is always a (read, write) operation, we should have already lock it in the caller.

See comment:

• When connected based protocal disconnected, we use this handler to clean it
• Notice that this function is a helper function thus the caller should
• take the responsibilty to lock the softc

483

In our condition, we have to let the fd received by accept syscall, e.g.(fd = accept(bind_fd)) have read permission. However, we cannot modify the permission after get into capsicum mode. So the only way to achieve it is to let bind_fd have read permission. And when doing accept system call, fd will inherit all capability from bind_fd.

537

You are right. Thanks for your kindly remind!

[lwhsu]

In D45120#1060104, @lwhsu wrote:

In D45120#1060064, @corvink wrote:

Is this ready? So far, LGTM.

We're still testing this with OpenStack and plan to push it once it completes.

We finally completed the test, this works fine with OpenStack Nova serialproxy and console. Please merge it!

[lwhsu]

In D45120#1061309, @lwhsu wrote:

In D45120#1060104, @lwhsu wrote:

In D45120#1060064, @corvink wrote:

Is this ready? So far, LGTM.

We're still testing this with OpenStack and plan to push it once it completes.

We finally completed the test, this works fine with OpenStack Nova serialproxy and console. Please merge it!

I rebase'd this patch and adjust the comments a little bit at https://github.com/lwhsu/freebsd-src/tree/bhyve-tcp-uart

[aokblast]

Rebase and refactor comment

[markj]

Most of the comments are minor, this mostly looks good to me. But it looks like we might double-close the socket when disconnecting.

usr.sbin/bhyve/bhyve.8

625

usr.sbin/bhyve/uart_backend.c

208

225–227

343

sc->tty.rfd is the mevent fd. mevent_delete_close() schedules the mevent fd to be closed, so are we double-closing the descriptor here?

485

Indentation here should be by four spaces.

540

This can be set in uart_tcp_backend(), no?

[aokblast]

Fix some typo and double close

[aokblast]

usr.sbin/bhyve/uart_backend.c

540

I set in here as a remind that if we add some protocol (like udp) needs socket, we should set is_socket ourself. This prevent dig into tcp code when adding new protocol. But I think you are right, it is better to put it into tcp_backend

[markj]

Looks ok, minor comments. If you send me a patch generated with git format-patch, I'll push it. Please explain the intended use-case(s) for this feature in the commit log message. (I think openstack was the main one?)

usr.sbin/bhyve/uart_backend.c

456

If you use warn("bind(%s:%s)", addr, port) instead of warnx(...), you'll get a better error message.

468

Same here. warn() automatically appends strerror(errno) to the output string.

499

IMO it's better to limit rights before the mevent_add() call, while the descriptor is still private.

539

The comment should be moved too (or simply removed).

[aokblast]

Use warn instead of warnx on system call

[emaste]

usr.sbin/bhyve/bhyve.8

1125–1126

0.0.0.0 comment not addressed, and this reads as if we can access the 800 by 600 pixel console via TCP -- maybe "accessed via VNC at .... and a serial console that can be accessed via TCP at ..."?

[aokblast]

Thanks for your feedback and here is the patch

0001-bhyve-Add-raw-tcp-to-uart-backend.patch10 KBDownload

[aokblast]

Elaborate manual over TCP raw socket

[aokblast]

For the 0.0.0.0 problem. I think we should fix it in the separated patch as we should also modify the VNC part.
Here is the branch that follows up the upstream modification of this patch:

https://github.com/aokblast/freebsd-src/tree/bhyve_uart_tcp_backend

[aokblast]

User 127.0.0.1 instead of 0.0.0.0 in manual

[aokblast]

Use ::1 for safe ipv6 address

[aokblast]

Hello, is there any update about this? Anyone want to merge this can use this to have direct patch.

[corvink]

It would be a very good idea to improve your commit message. It would be nice to explain the use case in more detail (if possible). Additionally, it would be great to mention limitations of this new feature e.g. that user have to make sure to protect the tcp socket from unprivileged access.

[aokblast]

Sorry I cannot find other use case. Do you have any idea on the use case of raw tcp? Due to the unsafeness of raw TCP. We should use this feature in localhost. And because of the lack of terminal command in raw TCP support. It is much better to use modem device rather than raw tcp in localhost. So this feature actually is for proxy only (like OpenStack) because most of these applications implement raw tcp connection rather than modem device. But if we support telnet it will be useful because it is much simpler than modem device on configuration. I am planning to do so but we need this feature first because telnet is built upon tcp.

[aokblast]

Elaborate commit message and manual