Bug 241808
| Summary: | bhyve e1000 broken after r354288 | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Aleksandr Fedorov <afedorov> |
| Component: | bhyve | Assignee: | freebsd-virtualization (Nobody) <virtualization> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | afedorov, emaste, jhb, vmaffione |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
| URL: | https://reviews.freebsd.org/D22286 | ||
Very interesting, thanks for catching it. I think the real problem here (even before r354288) is that nothing prevents a call to kevent(fd, EV_ENABLE) to happen before kevent(fd, EVADD) is called, as you are reporting. On mevent_add() and mevent_update(), mevent_notify() is called to wakeup the I/O thread, that will call kevent(changelist) to update the kernel. A race condition is possible where the client calls mevent_add() and mevent_update(EV_ENABLE) before the I/O thread has the chance to wake up and calls mevent_build()+kevent(changelist), which is exactly what happens in your case. r354288 only makes this race condition more likely (this explains why I did not catch the bug on my machine when I tested e1000). Your fix makes the race condition less likely, but I think it's still there. I tried to rework and simplify mevent.c to make sure EV_ADD is always called before EV_ENABLE or EV_DISABLE. Let me know what you think about that: https://reviews.freebsd.org/D22286 (In reply to Aleksandr Fedorov from comment #0) By the way it makes sense to move the call to netbe_rx_enable() where you suggest, and also call netbe_rx_disable() in the corresponding rx_disable function. A commit references this bug: Author: vmaffione Date: Tue Nov 12 21:07:51 UTC 2019 New revision: 354659 URL: https://svnweb.freebsd.org/changeset/base/354659 Log: bhyve: rework mevent processing to fix a race condition At the end of both mevent_add() and mevent_update(), mevent_notify() is called to wakeup the I/O thread, that will call kevent(changelist) to update the kernel. A race condition is possible where the client calls mevent_add() and mevent_update(EV_ENABLE) before the I/O thread has the chance to wake up and call mevent_build()+kevent(changelist) in response to mevent_add(). The mevent_add() is therefore ignored by the I/O thread, and kevent(fd, EV_ENABLE) is called before kevent(fd, EV_ADD), resuliting in a failure of the kevent(fd, EV_ENABLE) call. PR: 241808 Reviewed by: jhb, markj MFC with: r354288 Differential Revision: https://reviews.freebsd.org/D22286 Changes: head/usr.sbin/bhyve/mevent.c A commit references this bug: Author: vmaffione Date: Tue Nov 26 18:12:13 UTC 2019 New revision: 355117 URL: https://svnweb.freebsd.org/changeset/base/355117 Log: MFC r354659 bhyve: rework mevent processing to fix a race condition At the end of both mevent_add() and mevent_update(), mevent_notify() is called to wakeup the I/O thread, that will call kevent(changelist) to update the kernel. A race condition is possible where the client calls mevent_add() and mevent_update(EV_ENABLE) before the I/O thread has the chance to wake up and call mevent_build()+kevent(changelist) in response to mevent_add(). The mevent_add() is therefore ignored by the I/O thread, and kevent(fd, EV_ENABLE) is called before kevent(fd, EV_ADD), resuliting in a failure of the kevent(fd, EV_ENABLE) call. PR: 241808 Reviewed by: jhb, markj Differential Revision: https://reviews.freebsd.org/D22286 Changes: _U stable/12/ stable/12/usr.sbin/bhyve/mevent.c |
I found a problem trying to run the following command: sh /usr/share/examples/bhyve/vmrun.sh -c 2 -m 4096M -n e1000 -t tap1000 -d freebsd-12-0.img freebsd-0 Autoboot in 9 seconds, hit [Enter] to boot or any other key to stop Loading kernel... /boot/kernel/kernel text=0x1678a68 data=0x1cd288+0x768b40 syms=[0x8+0x174cd8+0x8+0x19224a] Loading configured modules... /boot/entropy size=0x1000 Error return from kevent change: No such file or directory ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ---<<BOOT>>--- Copyright (c) 1992-2018 The FreeBSD Project. Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 The Regents of the University of California. All rights reserved. FreeBSD is a registered trademark of The FreeBSD Foundation. FreeBSD 12.0-RELEASE r341666 GENERIC amd64 FreeBSD clang version 6.0.1 (tags/RELEASE_601/final 335540) (based on LLVM 6.0.1) .... driver bug: Unable to set devclass (class: atkbdc devname: (unknown)) Unhandled ps2 mouse command 0xe1 psm0: <PS/2 Mouse> irq 12 on atkbdc0 psm0: [GIANT-LOCKED] psm0: model Generic PS/2 mouse, device ID 0 Error return from kevent change: No such file or directory Error return from kevent change: No such file or directory Error return from kevent change: No such file or directory Error return from ke vent change: No such file or directory Error return from kevent change: No such file or directory uart0: <16550 or compatible> port 0x3f8-0x3ff irq 4 flags 0x10 on acpi0 uart0: console (9600,n,8,1) uart1: <16550 or compatible> port 0x2f8-0x2ff irq 3 on acpi0 vga0: <Generic ISA VGA> at port 0x3b0-0x3bb iomem 0xb0000-0xb7fff pnpid PNP0900 on isa0 As you can see, kevent () returns with an error when trying to update the list of events: https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/mevent.c?view=markup#l487 The guest virtual machine does not respond to the keyboard, etc. So, I added some debugging output to find out the cause of the error. Index: usr.sbin/bhyve/mevent.c =================================================================== --- usr.sbin/bhyve/mevent.c (revision 354546) +++ usr.sbin/bhyve/mevent.c (working copy) @@ -200,6 +200,7 @@ mevent_qlock(); LIST_FOREACH_SAFE(mevp, &change_head, me_list, tmpp) { + if (mevp->me_closefd) { /* * A close of the file descriptor will remove the @@ -218,6 +219,8 @@ kev[i].flags = mevent_kq_flags(mevp); kev[i].fflags = mevent_kq_fflags(mevp); kev[i].udata = mevp; + printf("%s: fd %d, flags: %04X\r\n", __func__, + mevp->me_fd, kev[i].flags); i++; } @@ -264,6 +267,8 @@ return (NULL); } + printf("%s fd: %d\r\n", __func__, tfd); + mevp = NULL; mevent_qlock(); @@ -338,12 +343,15 @@ if (evp->me_state == MEV_DEL_PENDING) return (EINVAL); + printf("%s: old state: %d new state: %d fd: %d\r\n", + __func__, evp->me_state, newstate, evp->me_fd); + /* * No update needed if state isn't changing */ if (evp->me_state == newstate) return (0); - + mevent_qlock(); evp->me_state = newstate; @@ -484,6 +492,9 @@ if (numev) { ret = kevent(mfd, changelist, numev, NULL, 0, NULL); if (ret == -1) { + for (int i = 0; i < numev; i++) + printf("%s: %d: descr: %lu flags: %04X\r\n", + __func__, i, changelist[i].ident, changelist[i].flags); perror("Error return from kevent change"); } } Guest kernel output: Autoboot in 9 seconds, hit [Enter] to boot or any other key to stop Loading kernel... /boot/kernel/kernel text=0x1678a68 data=0x1cd288+0x768b40 syms=[0x8+0x174cd8+0x8+0x19224a] Loading configured modules... /boot/entropy size=0x1000 mevent_add_state fd: 0 <-- Add stdin to kqueue mevent_add_state fd: 4 <-- Add tapX descriptor to kqueue (https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/net_backends.c?view=markup#l223) mevent_update: old state: 5 new state: 2 fd: 4 <-- Update tapX descriptor state from MEV_ADD_DISABLED to MEV_ENABLE mevent_add_state fd: 19 mevent_add_state fd: 7 mevent_build: fd 7, flags: 0001 mevent_build: fd 19, flags: 0001 mevent_build: fd 4, flags: 0004 <--- !!!!! Build eventlist for kevent() (tapX descriptor) - struct kevent::flags == EV_ENABLE, But before that, there was no call to kevent () with the EV_ADD flag for this descriptor!!!! mevent_build: fd 0, flags: 0001 mevent_dispatch: 0: descr: 7 flags: 0001 mevent_dispatch: 1: descr: 19 flags: 0001 mevent_dispatch: 2: descr: 4 flags: 0004 mevent_dispatch: 3: descr: 0 flags: 0001 Error return from kevent change: No such file or directory <-- We got an error!!! ---<<BOOT>>--- Copyright (c) 1992-2018 The FreeBSD Project. Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 As I understand, the problem is that the e1000 backend activates the reception of packets too soon - before the kevent () system call is called to add descriptor to kqueue from mevent_dispatch(). If I move the netbe_rx_enable() call to e82545_rx_enable(), the guest machine is working fine: Index: usr.sbin/bhyve/pci_e82545.c =================================================================== --- usr.sbin/bhyve/pci_e82545.c (revision 354546) +++ usr.sbin/bhyve/pci_e82545.c (working copy) @@ -1526,6 +1526,7 @@ { sc->esc_rx_enabled = 1; + netbe_rx_enable(sc->esc_be); } static void @@ -2351,8 +2352,6 @@ net_genmac(pi, sc->esc_mac.octet); } - netbe_rx_enable(sc->esc_be); - /* H/w initiated reset */ e82545_reset(sc, 0);