Page MenuHomeFreeBSD

bhyve: rework mevent processing to fix a race condition
ClosedPublic

Authored by vmaffione on Fri, Nov 8, 9:43 PM.

Details

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).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vmaffione created this revision.Fri, Nov 8, 9:43 PM
vmaffione edited the summary of this revision. (Show Details)Fri, Nov 8, 9:49 PM

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

This revision is now accepted and ready to land.Sat, Nov 9, 3:07 PM

Thanks for testing.

jhb added inline comments.Sat, Nov 9, 6:37 PM
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 updated this revision to Diff 64154.Sun, Nov 10, 7:05 PM
vmaffione marked 2 inline comments as done.

Addressed two comments.

This revision now requires review to proceed.Sun, Nov 10, 7:05 PM

Comments addressed.

markj accepted this revision as: markj.Mon, Nov 11, 1:28 PM
markj added inline comments.
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?

This revision is now accepted and ready to land.Mon, Nov 11, 1:28 PM
vmaffione updated this revision to Diff 64195.Mon, Nov 11, 6:49 PM
vmaffione marked 2 inline comments as done.

Move check under the lock.
Minor refactor.

This revision now requires review to proceed.Mon, Nov 11, 6:49 PM

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 added inline comments.Mon, Nov 11, 6:51 PM
usr.sbin/bhyve/mevent.c
312 ↗(On Diff #64154)

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

markj accepted this revision as: markj.Mon, Nov 11, 8:09 PM
markj added inline comments.
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.

This revision is now accepted and ready to land.Mon, Nov 11, 8:09 PM
vmaffione updated this revision to Diff 64205.Mon, Nov 11, 9:23 PM
vmaffione marked 3 inline comments as done.

Addressed more comments.

This revision now requires review to proceed.Mon, Nov 11, 9:23 PM
markj accepted this revision as: markj.Mon, Nov 11, 10:59 PM
This revision is now accepted and ready to land.Mon, Nov 11, 10:59 PM
This revision was automatically updated to reflect the committed changes.