bhyve: rework mevent processing to fix a race condition

[vmaffione]

Summary

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.

Test Plan

Tested with vtnet+TAP and e1000+TAP (netperf + ping between a VM and the host).

[afedorov]

The changes look good to me. And I can not reproduce the bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241808

[vmaffione]

Thanks for testing.

[jhb]

usr.sbin/bhyve/mevent.c

196 ↗

(On Diff #64092)

Perhaps put the comment above the line? That is the more typical style in FreeBSD. You could also remove the EV_ADD check and just clear the bit unconditionally with the comment explaining why.

300 ↗

(On Diff #64092)

Maybe make this 'bool enable' instead? Then the switch statement below can be simplified I think to be:

newstate = evp->me_state;
if (enable) {
   newstate &= ~EV_DISABLE;
   newstate |= EV_ENABLE;
} else {
   newstate &= ~EV_ENABLE;
   newstate |= EV_DISABLE;
}

This avoids the assert and I think is clearer about what the code is doing. You could also perhaps do something like this if you leave 'commnd':

newstate = evp->me_state & ~(EV_ENABLE | EV_DISABLE);
newstate |= command;

I don't think you need the assertion since the function is static and it's easy to verify all the two callers in the same file.

[vmaffione]

Addressed two comments.

[vmaffione]

Comments addressed.

[markj]

usr.sbin/bhyve/mevent.c

183 ↗

(On Diff #64154)

I would suggest keeping the mevent_kq_flags() wrapper just for consistency with these other wrappers.

312 ↗

(On Diff #64154)

Unrelated to your change, but I don't see why this check is safe. Isn't it possible for the mevent thread to be concurrently freeing the event object?

[vmaffione]

Move check under the lock.
Minor refactor.

[vmaffione]

Addressed more comments.

usr.sbin/bhyve/mevent.c

312 ↗

(On Diff #64154)

The check should be done under qlock, indeed.

As far as I can understand, it is implicit that the client cannot call mevent_enable(), mevent_disable() or use the mevp after calling mevent_delete() or mevent_delete_close(). If this is true I think we are safe.

[vmaffione]

usr.sbin/bhyve/mevent.c

312 ↗

(On Diff #64154)

At that point the check should probably be an assert, I guess.

[markj]

usr.sbin/bhyve/mevent.c

153 ↗

(On Diff #64195)

Style: missing parens around the return value.

306 ↗

(On Diff #64195)

Fix style here and above, since you're changing these lines anyway?

312 ↗

(On Diff #64154)

Indeed, I think an assertion makes more sense.

[vmaffione]

Addressed more comments.