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

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

[Gabriel Goller]

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.

We hit this in production with VLAN-tagged traffic forwarded by OVS out
of an igb NIC. The bug was actually triggered, not just found by
code inspection.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=8318

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
Gabriel Goller (3):
      net: restore the L3 header before VLAN TX offload checks
      openvswitch: preserve the L3 header after stacked VLAN pushes
      net: test stacked VLAN headers before TX offloads

 net/core/dev.c                                     |  17 +++
 net/core/net_test.c                                | 145 +++++++++++++++++++++
 net/openvswitch/actions.c                          |   6 +-
 .../selftests/net/openvswitch/openvswitch.sh       |  34 +++++
 4 files changed, 201 insertions(+), 1 deletion(-)
---
base-commit: 8d5ce4751fad9499ea408163af7f410f18a85782
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git main
change-id: 20261111-net_restore_the-59b5fb335fe3

Best regards,
--  
Gabriel Goller <g.goller@proxmox.com>

[PATCH RFC net 1/3] net: restore the L3 header before VLAN TX offload checks

[Gabriel Goller]

A second VLAN push leaves network_header pointing at an in-band VLAN
header. Drivers such as igb can interpret those bytes as an IP header
when preparing TSO and corrupt the packet.

Restore the L3 header before offload feature checks for VLAN-tagged
CHECKSUM_PARTIAL packets. Keep the generic skb_vlan_push() layout intact
because tc classification after a push depends on it.

This fix belongs in the common TX path because packets tagged by tc or
BPF do not pass through OVS actions. Keep it separate from the OVS fix:
OVS must preserve L3 before executing subsequent actions, including on
packets without checksum offload. TX validation cannot repair corruption
that has already occurred in an action, whether in OVS or tc.

validate_xmit_skb() also runs on noqueue upper devices such as bond,
team or macvlan. For CHECKSUM_PARTIAL packets tagged above such a
device, network_header then sits at L3 while skb->protocol is still a
VLAN type. The flow dissector parses the IP header as a VLAN header in
that state, which affects bond layer3+4 hashing and tc flower on the
lower device's egress. Bridged packets already get the same conversion
in br_dev_queue_push_xmit() since commit df356d5e81b0 ("bridge: Fix
network header pointer for vlan tagged packets").

The IPv4/IPv6 KUnit reproducer fails without this fix and passes with it.
All 22 enabled net_core KUnit cases and software tc_actions and tc_flower
tests pass in QEMU. Actual igb hardware TSO has not been tested.

Fixes: 938863727076 ("tc: adjust network header after 2nd vlan push")
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 net/core/dev.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3c58db732761..fa47fe60b444 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4219,6 +4219,20 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 		     !(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST)))
 		skb->ip_summed = CHECKSUM_NONE;
 
+	/* VLAN actions may leave network_header at an in-band VLAN header.
+	 * Hardware checksum and segmentation offloads need the L3 header.
+	 */
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    eth_type_vlan(skb->protocol)) {
+		int depth;
+
+		if (!vlan_get_protocol_and_depth(skb, skb->protocol, &depth))
+			goto out_unhandled_proto;
+
+		skb_set_network_header(skb, depth);
+		skb_reset_mac_len(skb);
+	}
+
 	features = netif_skb_features(skb);
 	skb = validate_xmit_vlan(skb, features);
 	if (unlikely(!skb))
@@ -4267,6 +4281,9 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 
 	return skb;
 
+out_unhandled_proto:
+	kfree_skb_reason(skb, SKB_DROP_REASON_UNHANDLED_PROTO);
+	goto out_null;
 out_kfree_skb:
 	kfree_skb(skb);
 out_null:

-- 
2.47.3

[PATCH RFC net 2/3] openvswitch: preserve the L3 header after stacked VLAN pushes

[Gabriel Goller]

A second VLAN push moves network_header to the in-band VLAN header.
Subsequent OVS set actions still expect it to point at IP, so they can
modify the wrong bytes and corrupt the packet.

OVS needs to retain the L3 header immediately after the push. Repairing
it only in validate_xmit_skb() is too late for intervening actions and
would not cover packets without CHECKSUM_PARTIAL. Keep this fix separate
from TX validation because it protects action execution, not just offload
preparation. Conversely, fixing OVS alone does not protect packets tagged
by tc or BPF before transmit.

The generic skb_vlan_push() layout is retained for tc classification.
The OVS push-then-set selftest fails without this fix and passes with it
in QEMU.

Fixes: 938863727076 ("tc: adjust network header after 2nd vlan push")
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 net/openvswitch/actions.c                          |  6 +++-
 .../selftests/net/openvswitch/openvswitch.sh       | 34 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 12e339cbf791..bf0bb5e6de2c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -197,9 +197,10 @@ static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
 		     const struct ovs_action_push_vlan *vlan)
 {
+	bool tag_present = skb_vlan_tag_present(skb);
 	int err;
 
-	if (skb_vlan_tag_present(skb)) {
+	if (tag_present) {
 		invalidate_flow_key(key);
 	} else {
 		key->eth.vlan.tci = vlan->vlan_tci;
@@ -207,6 +208,9 @@ static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
 	}
 	err = skb_vlan_push(skb, vlan->vlan_tpid,
 			    ntohs(vlan->vlan_tci) & ~VLAN_CFI_MASK);
+	/* OVS actions and TX offloads expect network_header to stay at L3. */
+	if (!err && tag_present)
+		skb->network_header += VLAN_HLEN;
 	skb_reset_mac_len(skb);
 	return err;
 }
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 21444657a040..339e3e09e643 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -28,6 +28,7 @@ tests="
 	tunnel_metadata				ovs: test extraction of tunnel metadata
 	drop_reason				drop: test drop reasons are emitted
 	pop_vlan				vlan: POP_VLAN action strips tag
+	push_vlan_set				vlan: SET after stacked VLAN pushes
 	dec_ttl					ttl: dec_ttl decrements IP TTL
 	flow_set				flow-set: Flow modify
 	action_set				set: SET action rewrites fields
@@ -1501,6 +1502,39 @@ test_tunnel_metadata() {
 	return 0
 }
 
+test_push_vlan_set() {
+	local sbx="test_push_vlan_set"
+	local actions
+
+	sbx_add "$sbx" || return $?
+	ovs_add_dp "$sbx" vlandp || return 1
+	ovs_add_netns_and_veths "$sbx" vlandp \
+		ns1 veth1 ns1veth 192.0.2.1/24 || return 1
+	ovs_add_netns_and_veths "$sbx" vlandp \
+		ns2 veth2 ns2veth 192.0.2.2/24 || return 1
+
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+
+	# SET must address the IP header while both VLAN tags are present.
+	# Pop them afterwards so the receiver needs no VLAN configuration.
+	actions='push_vlan(vid=200,pcp=0,tpid=0x8100),'
+	actions+='push_vlan(vid=100,pcp=0,tpid=0x8100),'
+	actions+='set(ipv4(src=192.0.2.1,dst=192.0.2.2,'
+	actions+='proto=1,tos=0,ttl=63,frag=0)),pop_vlan,pop_vlan,2'
+	ovs_add_flow "$sbx" vlandp \
+		'in_port(1),eth(),eth_type(0x0800),ipv4(proto=1),icmp()' \
+		"$actions" || return 1
+	ovs_sbx "$sbx" ip netns exec ns1 ping -c 3 -W 2 \
+		192.0.2.2 || return 1
+
+	return 0
+}
+
 test_pop_vlan() {
 	local sbx="test_pop_vlan"
 	sbx_add "$sbx" || return $?

-- 
2.47.3

[PATCH RFC net 3/3] net: test stacked VLAN headers before TX offloads

[Gabriel Goller]

Cover the QinQ regression without requiring an igb NIC, so incorrect L3
header offsets are caught before they reach hardware offload processing.

The IPv4 and IPv6 cases fail without the TX header fix and pass with it.
All 22 enabled net_core KUnit cases pass in QEMU. This does not validate
segmentation on physical hardware.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 net/core/net_test.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/net/core/net_test.c b/net/core/net_test.c
index 2b5080c22663..73a795f1443e 100644
--- a/net/core/net_test.c
+++ b/net/core/net_test.c
@@ -1721,7 +1721,152 @@ static void netmem_ref_test_tcp_devmem_recv(struct kunit *test)
 #endif
 #endif
 
+/* VLAN TX offloads need the L3 header, not the inner VLAN header. */
+
+#include <linux/if_vlan.h>
+#include <linux/etherdevice.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <net/flow_dissector.h>
+
+static void vlan_test_free_skb(void *ptr)
+{
+	kfree_skb(ptr);
+}
+
+static void vlan_test_free_netdev(void *ptr)
+{
+	free_netdev(ptr);
+}
+
+static netdev_features_t vlan_test_features_check(struct sk_buff *skb,
+						  struct net_device *dev,
+						  netdev_features_t features)
+{
+	struct kunit *test = *(struct kunit **)netdev_priv(dev);
+	unsigned int ip_len = vlan_get_protocol(skb) == htons(ETH_P_IP) ?
+			      sizeof(struct iphdr) : sizeof(struct ipv6hdr);
+
+	KUNIT_EXPECT_EQ(test, skb_network_offset(skb),
+			skb_transport_offset(skb) - ip_len);
+
+	/* Like igb, allow hardware TSO with multiple VLAN tags. */
+	return features;
+}
+
+static const struct net_device_ops vlan_test_netdev_ops = {
+	.ndo_features_check = vlan_test_features_check,
+};
+
+static void vlan_xmit_test(struct kunit *test)
+{
+	const __be16 *proto = test->param_value;
+	unsigned int ip_len = *proto == htons(ETH_P_IP) ?
+			      sizeof(struct iphdr) : sizeof(struct ipv6hdr);
+	unsigned int len = ETH_HLEN + ip_len + sizeof(struct tcphdr) + 2880;
+	struct net_device *dev;
+	struct flow_keys keys;
+	struct sk_buff *skb, *xmit;
+	unsigned int nhoff;
+	bool again = false;
+	int i;
+
+	dev = alloc_netdev(sizeof(test), "vlan_test%d", NET_NAME_UNKNOWN,
+			   ether_setup);
+	KUNIT_ASSERT_NOT_NULL(test, dev);
+	KUNIT_ASSERT_EQ(test,
+			kunit_add_action_or_reset(test, vlan_test_free_netdev, dev),
+			0);
+	dev->netdev_ops = &vlan_test_netdev_ops;
+	*(struct kunit **)netdev_priv(dev) = test;
+	dev->features = NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_TSO |
+			NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_TX;
+	dev->vlan_features = dev->features;
+
+	skb = alloc_skb(len, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, skb);
+	KUNIT_ASSERT_EQ(test,
+			kunit_add_action_or_reset(test, vlan_test_free_skb, skb),
+			0);
+	skb_put_zero(skb, len);
+	skb_reset_mac_header(skb);
+	skb_set_network_header(skb, ETH_HLEN);
+	skb_set_transport_header(skb, ETH_HLEN + ip_len);
+	skb_reset_mac_len(skb);
+	skb->protocol = *proto;
+	skb->dev = dev;
+	eth_hdr(skb)->h_proto = *proto;
+	if (*proto == htons(ETH_P_IP)) {
+		ip_hdr(skb)->version = 4;
+		ip_hdr(skb)->ihl = 5;
+		ip_hdr(skb)->protocol = IPPROTO_TCP;
+		ip_hdr(skb)->tot_len = htons(len - ETH_HLEN);
+	} else {
+		ipv6_hdr(skb)->version = 6;
+		ipv6_hdr(skb)->nexthdr = IPPROTO_TCP;
+		ipv6_hdr(skb)->payload_len = htons(len - ETH_HLEN - ip_len);
+	}
+	tcp_hdr(skb)->source = htons(1234);
+	tcp_hdr(skb)->dest = htons(4321);
+	tcp_hdr(skb)->doff = sizeof(struct tcphdr) / 4;
+	skb->ip_summed = CHECKSUM_PARTIAL;
+	skb->csum_start = skb->transport_header;
+	skb->csum_offset = offsetof(struct tcphdr, check);
+	skb_shinfo(skb)->gso_size = 1440;
+	skb_shinfo(skb)->gso_segs = 2;
+	skb_shinfo(skb)->gso_type = *proto == htons(ETH_P_IP) ?
+				  SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
+
+	for (i = 0; i < 3; i++) {
+		KUNIT_ASSERT_EQ(test,
+				skb_vlan_push(skb, htons(ETH_P_8021Q), 200 + i),
+				0);
+		skb_reset_mac_len(skb);
+		/* Classification after a push must still see all VLAN tags. */
+		memset(&keys, 0, sizeof(keys));
+		KUNIT_EXPECT_TRUE(test,
+				  __skb_flow_dissect(&init_net, skb,
+						     &flow_keys_dissector, &keys,
+						     NULL, 0, 0, 0, 0));
+		KUNIT_EXPECT_EQ(test, keys.basic.n_proto, *proto);
+		KUNIT_EXPECT_EQ(test, keys.basic.ip_proto, IPPROTO_TCP);
+		KUNIT_EXPECT_EQ(test, keys.ports.src, htons(1234));
+		KUNIT_EXPECT_EQ(test, keys.ports.dst, htons(4321));
+		KUNIT_EXPECT_EQ(test, (u16)keys.vlan.vlan_id, 200 + i);
+
+		xmit = skb_clone(skb, GFP_KERNEL);
+		KUNIT_ASSERT_NOT_NULL(test, xmit);
+		xmit = validate_xmit_skb_list(xmit, dev, &again);
+		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xmit);
+		nhoff = ETH_HLEN + i * VLAN_HLEN;
+		KUNIT_EXPECT_EQ(test, skb_network_offset(xmit), nhoff);
+		KUNIT_EXPECT_EQ(test, skb_transport_offset(xmit), nhoff + ip_len);
+		KUNIT_EXPECT_EQ(test, skb_checksum_start_offset(xmit),
+				nhoff + ip_len);
+		KUNIT_EXPECT_EQ(test, (u8)ip_hdr(xmit)->version,
+				*proto == htons(ETH_P_IP) ? 4 : 6);
+		KUNIT_EXPECT_TRUE(test, skb_is_gso(xmit));
+		KUNIT_EXPECT_PTR_EQ(test, xmit->next, NULL);
+		kfree_skb_list(xmit);
+	}
+}
+
+static const __be16 vlan_test_protos[] = {
+	htons(ETH_P_IP),
+	htons(ETH_P_IPV6),
+};
+
+static void vlan_test_desc(const __be16 *proto, char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s",
+		 *proto == htons(ETH_P_IP) ? "IPv4" : "IPv6");
+}
+
+KUNIT_ARRAY_PARAM(vlan, vlan_test_protos, vlan_test_desc);
+
 static struct kunit_case net_test_cases[] = {
+	KUNIT_CASE_PARAM(vlan_xmit_test, vlan_gen_params),
 	KUNIT_CASE_PARAM(gso_test_func, gso_test_gen_params),
 	KUNIT_CASE_PARAM(ip_tunnel_flags_test_run,
 			 ip_tunnel_flags_test_gen_params),

-- 
2.47.3

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

[Ilya Maximets]

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.