iflib: fix vlan offload processing across multiple drivers.

[vmaffione]

Summary

The logic that sets iri_vtag and M_VLANTAG does not handle the case where the vtag is 0 (and IPI
The iflib ift_rxd_pkt_get() routine includes processing for the
IFCAP_RXCSUM and IFCAP_VLAN_HWTAGGING offloads. This change
rearranges the igb processing to be more similar to the other
Intel drivers.

Also see
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260068

[vmaffione]

Use iri_ifp rather than iflib_get_ifp().

[erj]

sys/dev/e1000/igb_txrx.c

501 ↗

(On Diff #99376)

This is a subtle bug that I caught in another of our drivers, but you shouldn't just check if the vtag is non-zero; that improperly strips the packet of a possible priority 0 & VLAN 0-tagged header. You should still check the descriptor see if a tag exists.

[vmaffione]

sys/dev/e1000/igb_txrx.c

501 ↗

(On Diff #99376)

Do you mean checking for (staterr & E1000_RXD_STAT_VP)? That is done in the loop above, to avoid possible issues with multi-descriptor packets. Or what should I do?

[erj]

sys/dev/e1000/igb_txrx.c

501 ↗

(On Diff #99376)

Sorry, maybe it wasn't completely clear in my last comment, but you need to change the check from if there is a non-zero VLAN tag to if a VLAN tag was detected because it's possible that there is a VLAN tag that is just all 0's and we need to still inform the upper layers that it's there with M_VLANTAG -- one way to do that would be to copy that (staterr & E1000_RXD_STAT_VP) check down, and replace the if (vtag) highlighted here with it.

[vmaffione]

That's right, thanks. The same issue is there for at least em and ixgbe.
I can fix those if you like.

[vmaffione]

sys/dev/e1000/igb_txrx.c

501 ↗

(On Diff #99376)

I see, thanks. Btw, I was also worried about multi-descriptor packets: I expect the "vtag" to be present in the descriptor that contains the E1000_RXD_STAT_VP bit (staterr), but not in the other packet fragments. This means that it may be wrong to check for the VP bit outside of the loop.

[vmaffione]

Fix the iri_vtag setting logic for all the iflib drivers.

[vmaffione]

Also fix scctx->isc_capenable and isc_capabilities usage in igc.

[vmaffione]

sys/dev/e1000/igb_txrx.c

501 ↗

(On Diff #99376)

Never mind. The VLAN info (RXD_STAT_VP bit and vtag) is set in all the descriptors of a multi-descriptor packet. So it is safe to access those info outside of the loop.
I tried to fix the issue for all the iflib drivers.

[kbowling]

I'm less optimistic about the ifp seeing the change required in all drivers, @gallatin odes anyone have any idea on the impact of the ifp pointer chase and fetch versus grabbing it from the already fetched scctx in these hot paths? How would we sync scctx_capenable with ifnet, that seems to be a critical missing implementation from the outset.

[vmaffione]

Note that this patch contains a number of changes that fix separate issues:

1. fix VLAN offload processing handling the case of vtag == 0.
2. use if_getcapenable() rather the isc_capenable because the latter is not synchronized.
3. use iri_ifp rather than iflib_get_ifp() to avoid an additional indirection.

Those changes could/should be committed separately.
Regarding point (2), I agree with you that we should avoid the additional indirection by accessing isc_capenable. But the current status is that we have some drivers that use isc_capenable (and are therefore broken) and some drivers that use iflib_get_ifp() and are therefore correct although suboptimal. I would then propose to make the change in two steps. The first step is to restore correctness (this patch), as soon as possible, since there is no point in having "fast broken" code. Then we can have a second step where we properly synchronize isc_capenable and then change back all the drivers to access that.

[vmaffione]

use iri_ifp rather than iflib_get_ifp() in ice and ixgbe.

[vmaffione]

...or we can also switch to isc_capenable in the same changeset.
The last diff tweaks iflib's ioctl handler to synchronize isc_capenable to if_capenable.

[vmaffione]

Any comments from @erj or @gallatin ?