Page MenuHomeFreeBSD
Differential D22440

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

Authored by vmaffione on Nov 18 2019, 9:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 2:21 AM
Unknown Object (File)
Tue, Nov 12, 12:52 AM
Unknown Object (File)
Mon, Nov 11, 4:04 PM
Unknown Object (File)
Sat, Nov 9, 9:22 AM
Unknown Object (File)
Sat, Nov 9, 9:14 AM
Unknown Object (File)
Fri, Nov 8, 9:34 PM
Unknown Object (File)
Fri, Nov 8, 8:45 PM
Unknown Object (File)
Fri, Nov 8, 8:18 PM
View All 82 Files
Subscribers
afedorov
bcran
imp
rgrimes
woodsb02

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

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(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.Nov 18 2019, 9:51 PM
vmaffione updated this revision to Diff 64547.Nov 18 2019, 9:56 PM
vmaffione marked 2 inline comments as done.

Addressed comment.

This revision now requires review to proceed.Nov 18 2019, 9:56 PM
markj accepted this revision as: markj.Nov 18 2019, 9:58 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 ↗(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.Nov 19 2019, 5:43 PM

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 ↗(On Diff #64547)

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
afedorov added a comment.Nov 19 2019, 6:21 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.
vmaffione marked an inline comment as done.Nov 19 2019, 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

afedorov mentioned this in D27381: [bhyve] virtio-net: Do not allow receiving packets until features have been negotiated..Nov 26 2020, 12:22 PM

Revision Contents
Changeset List

PathSize
head/
usr.sbin/
bhyve/
33 lines

Diff 64583

View Options

head/usr.sbin/bhyve/pci_virtio_net.c

Loading...