Page MenuHomeFreeBSD

bhyve: virtio-net: disable receive until features are negotiated
ClosedPublic

Authored by vmaffione on Mon, Nov 18, 9:39 PM.

Details

Summary

This patch fixes a race condition where the receive callback is called while the device is being reset. Since the rx_merge variable may change during reset, the receive callback may operate inconsistently with what the guest expects.
Also, get rid of the unused rx_vhdrlen variable.

Test Plan

Tested with FreeBSD guests with both TAP and VALE net backends.

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.Mon, Nov 18, 9:39 PM
markj accepted this revision as: markj.Mon, Nov 18, 9:51 PM
markj added inline comments.
usr.sbin/bhyve/pci_virtio_net.c
576 ↗(On Diff #64545)

The initialization is not needed.

585 ↗(On Diff #64545)

Fix comment style while you're here?

This revision is now accepted and ready to land.Mon, Nov 18, 9:51 PM
vmaffione updated this revision to Diff 64547.Mon, Nov 18, 9:56 PM
vmaffione marked 2 inline comments as done.

Addressed comment.

This revision now requires review to proceed.Mon, Nov 18, 9:56 PM
markj accepted this revision as: markj.Mon, Nov 18, 9:58 PM

Seems ok to me.

This revision is now accepted and ready to land.Mon, Nov 18, 9:58 PM
jhb added inline comments.Tue, Nov 19, 5:17 PM
usr.sbin/bhyve/pci_virtio_net.c
515 ↗(On Diff #64547)

My only thought is perhaps we should assume rx_merge is not supported until we negotiate that it is. The race would not have been fatal (but is still good to fix) if the default been the more compatible behavior. That is, maybe 'rx_merge' should be 0 by default instead of 1.

vmaffione updated this revision to Diff 64579.Tue, Nov 19, 5:43 PM

Init rx_merge to 0

This revision now requires review to proceed.Tue, Nov 19, 5:43 PM
vmaffione marked an inline comment as done.Tue, Nov 19, 5:44 PM
vmaffione added inline comments.
usr.sbin/bhyve/pci_virtio_net.c
515 ↗(On Diff #64547)

Agreed.

markj accepted this revision as: markj.Tue, Nov 19, 5:51 PM
This revision is now accepted and ready to land.Tue, Nov 19, 5:51 PM
jhb accepted this revision.Tue, Nov 19, 6:15 PM

The race would not have been fatal (but is still good to fix) if the default been the more compatible behavior.

I observed a situation that the virtio queue is in an inconsistent state (FreeBSD guest, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=242023).

vmaffione marked an inline comment as done.Tue, Nov 19, 9:12 PM

Il giorno mar 19 nov 2019 alle ore 19:21 aleksandr.fedorov_itglobal.com
(Aleksandr Fedorov) <phabric-noreply@freebsd.org> ha scritto:

aleksandr.fedorov_itglobal.com added a comment.

> The race would not have been fatal (but is still good to fix) if the

default been the more compatible behavior.

I observed a situation that the virtio queue is in an inconsistent state

(FreeBSD guest, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=242023).
Do you mean that you observed additional issues after this patch?

CHANGES SINCE LAST ACTION

https://reviews.freebsd.org/D22440/new/

REVISION DETAIL

https://reviews.freebsd.org/D22440

EMAIL PREFERENCES

https://reviews.freebsd.org/settings/panel/emailpreferences/

To: vmaffione, jhb, markj, bhyve
Cc: imp, aleksandr.fedorov_itglobal.com, rgrimes, woodsb02, bcran