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

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

Details

Reviewers

jhb
markj

Group Reviewers

bhyve

Commits

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

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

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

vmaffione created this revision.Nov 18 2019, 9:39 PM

Herald added a reviewer: bhyve. · View Herald TranscriptNov 18 2019, 9:39 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

Harbormaster completed remote builds in B27627: Diff 64545.Nov 18 2019, 9:39 PM

markj accepted this revision as: markj.Nov 18 2019, 9:51 PM

markj added inline comments.

usr.sbin/bhyve/pci_virtio_net.c
576	The initialization is not needed.
589–590	Fix comment style while you're here?

This revision is now accepted and ready to land.Nov 18 2019, 9:51 PM

Addressed comment.

This revision now requires review to proceed.Nov 18 2019, 9:56 PM

Seems ok to me.

This revision is now accepted and ready to land.Nov 18 2019, 9:58 PM

jhb added inline comments.Nov 19 2019, 5:17 PM

usr.sbin/bhyve/pci_virtio_net.c
515	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.

Init rx_merge to 0

This revision now requires review to proceed.Nov 19 2019, 5:43 PM

vmaffione marked an inline comment as done.Nov 19 2019, 5:44 PM

vmaffione added inline comments.

usr.sbin/bhyve/pci_virtio_net.c
515	Agreed.

markj accepted this revision as: markj.Nov 19 2019, 5:51 PM

This revision is now accepted and ready to land.Nov 19 2019, 5:51 PM

jhb accepted this revision.Nov 19 2019, 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).

Closed by commit rS354864: bhyve: virtio-net: disable receive until features are negotiated (authored by vmaffione). · Explain WhyNov 19 2019, 9:11 PM

This revision was automatically updated to reflect the committed changes.

vmaffione added a commit: rS354864: bhyve: virtio-net: disable receive until features are negotiated.

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