Page MenuHomeFreeBSD

iflib: Stop interface before (un)registering VLAN
ClosedPublic

Authored by erj on Oct 18 2019, 10:44 PM.

Details

Summary

We have a possibly unique situation in the FreeBSD iavf(4) driver that requires a change in how VLANs are unregistered in iflib.

By default on ice(4), the Linux PF enables VLAN anti-spoof for VFs that it spawns. This functionality causes the PF driver to generate MDD events when the VF transmits a packet with a VLAN tag that the PF didn't explicitly allow for the VF.

These "Malicious Driver Detection" events cause the offending packet to be dropped, which wouldn't be a big deal if it stopped there. However, the PF disrupts the VF's functionality, requiring the VF to be ifconfig down/up to continue transmitting/receiving again.

To go back to the original problem, it can happen that a VLAN is unregistered (so the VF notifies the PF that it is no longer using that VLAN), but the VF's software queues still have VLAN tagged packets in them, causing them to be to transmitted after the VLAN has been unconfigured, causing the MDD events described above. This patch changes the iflib_unregister_vlan() flow to do an iflib_stop() first before unregistering the VLAN, causing those still-queued Tx packets to be flushed before removing the VLAN, thus avoiding this situation. This patch should still be functionally equivalent to what was done before in other respects, since it just separates the iflib_stop() and iflib_init_locked() steps that were included in the one iflib_if_init_locked() function call before.

That said, I don't think IFCAP_VLAN_HWFILTER really applies to this situation; this would probably still be a problem even if it was turned off. An older revision of this patch just checked that capability; that's the smallest possible diff that would solve this issue, but I don't think it's the best solution.

As well, this patch is against stable/11, but it really applies to all versions of iflib.

Diff Detail

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

Event Timeline

erj added a reviewer: gallatin.
  • Add a specific iflib flag for VLAN anti-spoof and use it
erj retitled this revision from iflib: Stop interface before unregistering VLAN to iflib: Stop interface before (un)registering VLAN.
jacob.e.keller_intel.com added inline comments.
sys/net/iflib.c
4163–4166 ↗(On Diff #63517)

I know we've received some complaints about start/stop for adding VLANs. I wonder if we should just have a flag that indicates if you need or (more precisely) don't need to stop the interface after adding a VLAN.

Then, drivers which could add VLANs without a re-init would set that flag, and drivers which don't have the flag would stop the interface first, and then re-init after.

sys/net/iflib.c
4163–4166 ↗(On Diff #63517)

We talked about this in an iflib meeting about a month ago, I think. Eg, that there were various events that some nics might want a restart on, but that others would be OK with. We were talking about having an event code that would be passed to an iflib method. The method would return true or false (restart needed or not) based on the event. By default, if a driver did not implement the method, he would get a default method that would always return true (restart needed).

sys/net/iflib.c
4163–4166 ↗(On Diff #63517)

Yea, I like that idea.

  • Follow Drew's suggestion and create an iflib method for this

Fix incorrect type usage

  • Use iflib_init_locked()
This revision is now accepted and ready to land.Apr 8 2020, 1:53 PM
This revision was automatically updated to reflect the committed changes.