Re: [PATCH RFC net 0/3] net: fix stacked VLAN header corruption in TX offloads and OVS

[Ilya Maximets <i.maximets@ovn.org>]

On 11/11/26 6:37 PM, Gabriel Goller wrote:
> Since commit 938863727076 ("tc: adjust network header after 2nd vlan
> push"), a second VLAN push leaves network_header pointing to the
> in-band VLAN header. The igb driver TSO and OVS IP set actions still
> expect the L3 header, so they write to the VLAN header and corrupt
> the packet. Patch 1 restores the L3 header in validate_xmit_skb() for
> CHECKSUM_PARTIAL packets, which covers packets tagged by tc or BPF.
> Patch 2 restores it in OVS right after the push, before later actions
> run. Patch 3 adds a KUnit test. tc classification after a push keeps
> working.
> 
> The kernel uses two conventions for skb->network_header when a packet
> carries an in-band VLAN tag:
> 
> * network_header points at the L3 header
> * network_header points at the in-band VLAN header
> 
> Since commit 938863727076 ("tc: adjust network header after 2nd vlan
> push"), skb_vlan_push() uses the second convention when it pushes a
> second tag. The flow dissector, and so tc flower, needs that layout to
> classify the packet after the push. Drivers doing TSO or checksum
> offload expect the first one. vlan_features_check() clears TSO for
> multi-tagged packets, so most drivers fall back to software GSO, which
> finds the L3 header itself. igb keeps TSO for these packets and reads
> the IP header at skb->network_header, so it builds the TSO context from
> the VLAN header and corrupts the packet. OVS set actions after a second
> push write to the wrong header in the same way.
> 
> Which convention should a packet with in-band VLAN tags follow? This
> series keeps the tc layout, converts to L3 in validate_xmit_skb() for
> CHECKSUM_PARTIAL packets, and restores L3 right after the push in OVS.
> If L3 should hold everywhere instead, the flow dissector would need to
> find in-band tags from mac_len, and skb_vlan_push() could go back to
> its old behavior.

IMO, having 'network_header' pointing to something that is not a network
layer doesn't make much sense, and having two separate meanings for the
same field is a recipe for more issues introduced or discovered later.
Others may, of course, disagree.

Best regards, Ilya Maximets.