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

bhyve slirp backend enhancements #2

Open
PHSpline opened this issue Jun 30, 2024 · 3 comments
Open

bhyve slirp backend enhancements #2

PHSpline opened this issue Jun 30, 2024 · 3 comments

Comments

@PHSpline
Copy link
Contributor

Hi @markjdb, thanks for implementing slirp backend support in bhyve. Are there any plans to allow for the various SlirpConfig parameters to be overridden? Currently it seems as though the DHCP range is hardcoded, resulting in the same 10.0.2.15 IP being assigned to each slirp interface in the case where multiple slirp-backed interfaces are used within a single VM. It's easy enough to assign a static IP to the interfaces but it'd be cool if I could have each interface be assigned IPs from different subnets, via DHCP.

Additionally, it would be neat if the DHCP server could be disabled per interface as well (disable_dhcp parameter in SlirpConfig) in the case of bridging multiple slirp-backed interfaces together and wanting only one to have an active DHCP server. Likely a niche use case, but I can't help but think about it for completeness.

Another thought I had was regarding the rootless bhyve stuff - would it be feasible to also allow a mode where bhyve starts out as root to configure networking and then immediately drops privileges to a non-root user using e.g. setuid/setgid? This way, the more performant netgraph backend could still be used while also reducing root user exposure.

@markjdb
Copy link
Owner

markjdb commented Jun 30, 2024

Hi Seth, I'm glad the slirp backend is useful to you. I didn't have any plans to change anything since my needs are simple and covered by the existing functionality.

As you note we hard-code the the 10.0.2.0/24 subnet in the slirp parameters for each subnet. To start, we should at least automatically create a new subnet for each interface. In the absence of any explicit configuration, I'm inclined to make each interface use 10.0.X.0/24 with X=2 for the first interface, and autoincrementing up to X=254, beyond which one would need to explicitly specify the desired subnet somehow. Does that seem reasonable to you?

To disable DHCP, I propose letting disable_dhcp be a key in the slirp configuration, so one can write slirp,hostfwd=...,disable_dhcp,.... Maybe it would be better to have an explicit flags=disable_dhcp,... key-value pair, I'm not sure. I think I prefer the former.

Dropping privileges after VM initialization is likely a good idea, though bhyve's Capsicum sandbox should in principle provide even stronger protections, so I don't immediately see any examples of attack vectors that would be closed by that change. It is still useful as a defense-in-depth measure at least. That's somewhat orthogonal to rootless bhyve IMO, but it should be easy to implement this as a part of that project.

@markjdb
Copy link
Owner

markjdb commented Jun 30, 2024

You'd also asked about IPv6 support: again, I don't personally need it, but there's no real reason it couldn't be supported. Do you have a specific use-case in mind?

@PHSpline
Copy link
Contributor Author

PHSpline commented Jun 30, 2024

Everything you have suggested sounds perfect! Good point about Capsicum, I forgot about that.

As for IPv6, I don't have a need for it at the moment either. I suppose by the time it is needed, perhaps it is better to just jump to netgraph instead of making the slirp backend more complex.

Thanks!

markjdb added a commit that referenced this issue Jul 16, 2024
if_vtnet holds a queue lock when processing incoming packets.  It sends
them up the stack without dropping the lock, which is incorrect.  For
example, witness generates warnings when if_vtnet receives ND6 packets:

 may sleep with the following non-sleepable locks held:
exclusive sleep mutex vtnet1-rx0 (vtnet1-rx0) r = 0 (0xfffff80001790000) locked @ /root/freebsd/sys/dev/virtio/network/if_vtnet.c:2202
stack backtrace:
.#0 0xffffffff80bd6785 at witness_debugger+0x65
.#1 0xffffffff80bd78e3 at witness_warn+0x413
.#2 0xffffffff80d8a504 at in6_update_ifa+0xcb4
.#3 0xffffffff80db7030 at in6_ifadd+0x1e0
.#4 0xffffffff80db3734 at nd6_ra_input+0x1024
.#5 0xffffffff80d8430b at icmp6_input+0x5ab
.#6 0xffffffff80d9db68 at ip6_input+0xc78
.#7 0xffffffff80cbb21d at netisr_dispatch_src+0xad
.#8 0xffffffff80c9db0a at ether_demux+0x16a
.#9 0xffffffff80c9f15a at ether_nh_input+0x34a
.#10 0xffffffff80cbb21d at netisr_dispatch_src+0xad
.#11 0xffffffff80c9df69 at ether_input+0xd9
.#12 0xffffffff80986240 at vtnet_rxq_eof+0x790
.#13 0xffffffff809859fc at vtnet_rx_vq_process+0x9c
.#14 0xffffffff80b19dc6 at ithread_loop+0x266
.#15 0xffffffff80b161f2 at fork_exit+0x82
.#16 0xffffffff8105322e at fork_trampoline+0xe

Simply drop the lock before entering the network stack.  We could batch
this by linking a small number of packets using m_nextpkt; ether_input()
handles such batches.

PR:		253888
Reviewed by:	zlei
MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D45950
markjdb added a commit that referenced this issue Sep 1, 2024
if_vtnet holds a queue lock when processing incoming packets.  It sends
them up the stack without dropping the lock, which is incorrect.  For
example, witness generates warnings when if_vtnet receives ND6 packets:

 may sleep with the following non-sleepable locks held:
exclusive sleep mutex vtnet1-rx0 (vtnet1-rx0) r = 0 (0xfffff80001790000) locked @ /root/freebsd/sys/dev/virtio/network/if_vtnet.c:2202
stack backtrace:
.#0 0xffffffff80bd6785 at witness_debugger+0x65
.#1 0xffffffff80bd78e3 at witness_warn+0x413
.#2 0xffffffff80d8a504 at in6_update_ifa+0xcb4
.#3 0xffffffff80db7030 at in6_ifadd+0x1e0
.#4 0xffffffff80db3734 at nd6_ra_input+0x1024
.#5 0xffffffff80d8430b at icmp6_input+0x5ab
.#6 0xffffffff80d9db68 at ip6_input+0xc78
.#7 0xffffffff80cbb21d at netisr_dispatch_src+0xad
.#8 0xffffffff80c9db0a at ether_demux+0x16a
.#9 0xffffffff80c9f15a at ether_nh_input+0x34a
.#10 0xffffffff80cbb21d at netisr_dispatch_src+0xad
.#11 0xffffffff80c9df69 at ether_input+0xd9
.#12 0xffffffff80986240 at vtnet_rxq_eof+0x790
.#13 0xffffffff809859fc at vtnet_rx_vq_process+0x9c
.#14 0xffffffff80b19dc6 at ithread_loop+0x266
.#15 0xffffffff80b161f2 at fork_exit+0x82
.#16 0xffffffff8105322e at fork_trampoline+0xe

Simply drop the lock before entering the network stack.  We could batch
this by linking a small number of packets using m_nextpkt; ether_input()
handles such batches.

PR:		253888
Reviewed by:	zlei
MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D45950
markjdb pushed a commit that referenced this issue Dec 5, 2024
Avoid calling _callout_stop_safe with a non-sleepable lock held when
detaching by initializing callout_init_rw() with CALLOUT_SHAREDLOCK.

It avoids the following WITNESS warning when stopping the service:

    # service ipfilter stop
    calling _callout_stop_safe with the following non-sleepable locks held:
    shared rw ipf filter load/unload mutex (ipf filter load/unload mutex) r = 0 (0xffff0000417c7530) locked @ /usr/src/sys/netpfil/ipfilter/netinet/fil.c:7926
    stack backtrace:
    #0 0xffff00000052d394 at witness_debugger+0x60
    #1 0xffff00000052e620 at witness_warn+0x404
    #2 0xffff0000004d4ffc at _callout_stop_safe+0x8c
    #3 0xffff0000f7236674 at ipfdetach+0x3c
    #4 0xffff0000f723fa4c at ipf_ipf_ioctl+0x788
    #5 0xffff0000f72367e0 at ipfioctl+0x144
    #6 0xffff00000034abd8 at devfs_ioctl+0x100
    #7 0xffff0000005c66a0 at vn_ioctl+0xbc
    #8 0xffff00000034b2cc at devfs_ioctl_f+0x24
    #9 0xffff0000005331ec at kern_ioctl+0x2e0
    #10 0xffff000000532eb4 at sys_ioctl+0x140
    #11 0xffff000000880480 at do_el0_sync+0x604
    #12 0xffff0000008579ac at handle_el0_sync+0x4c

PR:		282478
Suggested by:	markj
Reviewed by:	cy
Approved by:	emaste (mentor)
MFC after:	1 week
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants