Page MenuHomeFreeBSD

bhyve: Add a slirp network backend
ClosedPublic

Authored by markj on Nov 8 2023, 6:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 9:27 AM
Unknown Object (File)
Tue, Nov 12, 8:46 PM
Unknown Object (File)
Tue, Nov 12, 8:01 PM
Unknown Object (File)
Oct 3 2024, 11:01 PM
Unknown Object (File)
Oct 1 2024, 12:49 PM
Unknown Object (File)
Sep 28 2024, 12:25 AM
Unknown Object (File)
Sep 26 2024, 4:01 PM
Unknown Object (File)
Sep 23 2024, 9:53 PM

Details

Summary

This enables a subset of the functionality provided by QEMU's user
networking implementation. In particular, it uses net/libslirp, the
same library as QEMU.

libslirp is permissively licensed but has some dependencies which make
it impractical to bring into the base system (glib in particular). I
thus opted to make bhyve dlopen the libslirp.so, which can be installed
via pkg. The library header is imported into bhyve.

The slirp backend takes a "hostfwd" which is identical to QEMU's
hostfwd. When configured, bhyve opens a host socket and listens for
connections, which get forwarded to the guest. For instance,
"hostfwd=tcp::1234-:22" allows one to ssh into the guest by ssh'ing to
port 1234 on the host, e.g., via 127.0.0.1. I didn't try to hook up
guestfwd support since I don't personally have a use-case for it yet,
and I think it won't interact nicely with the capsicum sandbox.

The data path is kind of complicated because there's quite a lot of
callbacks involved. When the backend receives a packet to be sent to
the guest, libslirp calls slirp_cb_send_packet(), which puts the packet
into a socket buffer, to be read by the mevent thread in slirp_recv().
Packets transmitted by the guest are handed to slirp_send(), which calls
slirp_input() to perform library processing.

We use a dedicated thread to handle events; the slirp interface is
really designed for a thread which calls poll(), so it was cleaner to
add a new event loop rather than try to translate between pollfds and
mevent. This loop polls for incoming connection requests and handles
libslirp timeout events (retransmits, etc.). A mutex is used to
serialize calls into libslirp after initialization is done.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Nov 8 2023, 6:05 PM

May I ask what use case you have for this? I am just wondering what this will solve that cannot be done already?

In D42510#970280, @bz wrote:

May I ask what use case you have for this? I am just wondering what this will solve that cannot be done already?

It lets one access a VM without requiring any network configuration. In particular, you don't have to be root to configure networking. This removes one obstacle to running bhyve as a non-root user.

In my case, I use QEMU (and its user networking, which also uses libslirp) to automate running the test suite for different architectures, and I do this as a normal user. I would like to use bhyve instead.

In D42510#970280, @bz wrote:

May I ask what use case you have for this? I am just wondering what this will solve that cannot be done already?

It lets one access a VM without requiring any network configuration. In particular, you don't have to be root to configure networking. This removes one obstacle to running bhyve as a non-root user.

Can you put the bit (running bhyve as non-root user/no network config needed) somewhere into the description/proposed commit message as that'll help understand.

usr.sbin/bhyve/libslirp.h
2

Is there an upstream for this file? Should we track it via contrib? Or is it slow moving enough that it wouldn't help much.

usr.sbin/bhyve/libslirp.h
2

It's here: https://gitlab.freedesktop.org/slirp/libslirp

I think it's ok to keep it here. Assuming that the library is maintained properly, I expect it to stay ABI-compatible with this header. SlirpConfig may be extended but it has its own versioning scheme. I don't feel too strongly about this though. We could import libslirp, but it can't be built as part of the base system. (The glib dependency means that we can't build it; I think libslirp's use of glib is simple enough that we could shim it away. That's on my list of things to look into, but I think using dlopen() isn't too bad...)

I'll copy the libslirp copyright file into this header.

rew added a subscriber: rew.

Tested and works as expected - this will be nice to have, thanks.

This revision is now accepted and ready to land.Nov 12 2023, 7:54 AM
In D42510#970849, @rew wrote:

Tested and works as expected - this will be nice to have, thanks.

Thanks for testing.

I realized today that instantiating a libslirp context per NIC is wrong, it'll break DHCP. The libslirp context should be global, like the libslirp.so handle. I'm not sure why one might use more than one slirp interface with a VM but that's probably due to a lack of imagination.

  • Remove -fsanitize.
  • Add a license header to libslirp.h, copied from libslirp/COPYRIGHT.
  • Add a herald comment, to be expanded, and remove some copy-pasted comments.

Let multiple hostfwd rules be delimited by semicolons. This is a bit
ugly since it requires escaping in the shell. I had an alternate implementation
which lets config.c optionally handle multiple values for a config node, but
it was a bit complex.

Add some comments.

I realized today that instantiating a libslirp context per NIC is wrong, it'll break DHCP. The libslirp context should be global, like the libslirp.so handle. I'm not sure why one might use more than one slirp interface with a VM but that's probably due to a lack of imagination.

In fact, one slirp context per NIC appears to be the way to go. At least, that's what QEMU does. Having a global instance doesn't really work with multiple network devices, if only because it becomes unclear which device slirp_send() should be handing packets to.

Let multiple hostfwd rules be delimited by semicolons. This is a bit
ugly since it requires escaping in the shell. I had an alternate implementation
which lets config.c optionally handle multiple values for a config node, but
it was a bit complex.

If anyone is interested, the alternate implementation is here: https://reviews.freebsd.org/D42572

Update the man page to document support for multiple hostfwd rules.

Would it be worth having a separate slirp.c perhaps for the bits in net_backends.c? I think you could also then just drop BHYVE_SLIRP. If people ever wanted this optional we could make adding slirp.c to SRCS conditional on MK_BHYVE_SLIRP or the like. Impressed that it fits into the existing net_backends abstraction as-is.

This is definitely a feature people have requested over the years, and this is the last major obstacle to non-root bhyve IIRC.

Oh, also, bhyve_config.5 probably needs updating?

In D42510#972442, @jhb wrote:

Would it be worth having a separate slirp.c perhaps for the bits in net_backends.c?

Yes, though this will require a bit of refactoring since all of the net_backend bits are private to net_backend.c. To make things consistent, I'd move each backend into its own file. I think we'd end up with net_backend.c, net_backend_tap.c, net_backend_netgraph.c, net_backend_netmap.c, net_backend_slirp.c.

I do agree with jhb. Why do we need to make it optional? If there's no use case, just drop BHYVE_SLIRP. We can add it later if required.

In D42510#972442, @jhb wrote:

Would it be worth having a separate slirp.c perhaps for the bits in net_backends.c?

Yes, though this will require a bit of refactoring since all of the net_backend bits are private to net_backend.c. To make things consistent, I'd move each backend into its own file. I think we'd end up with net_backend.c, net_backend_tap.c, net_backend_netgraph.c, net_backend_netmap.c, net_backend_slirp.c.

Ah, ok. That refactoring would be fine with me, but I'd also be fine if it was just net_backend_slirp.c for now and the others were split out later? I'm not sure tied to it either way though.

  • Rebase on top of D42689.
  • Move the slirp backend into its own file.
  • Add a dummy notify callback.
  • Bump the socket buffer size.
  • Avoid using warn()/err().
usr.sbin/bhyve/net_backends.c
54–55 ↗(On Diff #130359)

This seems unrelated to this patch.

markj added inline comments.
usr.sbin/bhyve/net_backends.c
54–55 ↗(On Diff #130359)

I removed this change in my local tree.

This revision is now accepted and ready to land.Nov 22 2023, 5:59 PM
This revision was automatically updated to reflect the committed changes.
markj marked an inline comment as done.