[PATCH v2 net] vlan: fix REORDER_HDR race between header and xmit paths

[PATCH v2 net] vlan: fix REORDER_HDR race between header and xmit paths

[Yizhou Zhao]

vlan_dev_change_flags() updates vlan->flags under RTNL, but the VLAN
data path reads the same field without RTNL.  In particular,
vlan_dev_hard_header() and vlan_dev_hard_start_xmit() may observe
different values of VLAN_FLAG_REORDER_HDR for the same skb.

This can lead to inconsistent tagging.  If REORDER_HDR is cleared when
vlan_dev_hard_header() runs, the function pushes an in-band VLAN header
into the skb.  If REORDER_HDR is then observed as set by
vlan_dev_hard_start_xmit(), the xmit path may also attach a hardware
accelerated VLAN tag, causing the packet to be emitted with two VLAN
tags.  Conversely, if the flag changes in the other direction, the skb
may be emitted without the expected VLAN tag.

Avoid making the xmit decision depend on a second unsynchronized read of
vlan->flags.  Instead, use skb->protocol which was set to vlan->vlan_proto
by vlan_dev_hard_header() when it pushed a VLAN header (REORDER_HDR off),
or left as the encapsulated protocol otherwise (REORDER_HDR on).
Checking skb->protocol first also preserves the short-circuit evaluation
order introduced by commit dacab578c7c6c ("vlan: fix a potential
uninit-value in vlan_dev_hard_start_xmit()"): when no VLAN header was
pushed, skb->protocol != vlan->vlan_proto is true and veth->h_vlan_proto
is not read, avoiding the uninit-value issue.

Also use READ_ONCE() for the data-path read in vlan_dev_hard_header()
and WRITE_ONCE() for the control-path update in vlan_dev_change_flags().

Fixes: 6ab3b487db77 ("[VLAN]: Fix nested VLAN transmit bug")
Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
Reported-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
Reported-by: Ao Wang <wangao@seu.edu.cn>
Reported-by: Xuewei Feng <fengxw06@126.com>
Reported-by: Qi Li <qli01@tsinghua.edu.cn>
Reported-by: Ke Xu <xuke@tsinghua.edu.cn>
Assisted-by: GLM:GLM-5.1
Signed-off-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
---
Changes in v2:
- Replace `vlan->flags & VLAN_FLAG_REORDER_HDR` with `skb->protocol != vlan->vlan_proto`
  in xmit.  v1 used `veth->h_vlan_proto != vlan->vlan_proto` alone, which
  re-introduced the uninit-value bug fixed by commit dacab578c7c6c ("vlan:
  fix a potential uninit-value in vlan_dev_hard_start_xmit()"): when
  REORDER_HDR is on, vlan_dev_hard_header() does not push a VLAN header and
  veth->h_vlan_proto may reference uninitialized data.  skb->protocol avoids
  this because vlan_dev_hard_header() sets it to vlan->vlan_proto only when
  it pushes a VLAN header (REORDER_HDR off), preserving the short-circuit.
- Add Fixes: tag to 6ab3b487db77 ("[VLAN]: Fix nested VLAN transmit bug"),
  the commit that first introduced the vlan->flags read in xmit.
- Link to v1: https://lore.kernel.org/netdev/20260529073004.77147-1-zhaoyz24@mails.tsinghua.edu.cn/
---
 net/8021q/vlan_dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -54,7 +54,7 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
 	u16 vlan_tci = 0;
 	int rc;

-	if (!(vlan->flags & VLAN_FLAG_REORDER_HDR)) {
+	if (!(READ_ONCE(vlan->flags) & VLAN_FLAG_REORDER_HDR)) {
 		vhdr = skb_push(skb, VLAN_HLEN);

 		vlan_tci = vlan->vlan_id;
@@ -110,7 +110,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 	 * NOTE: THIS ASSUMES DIX ETHERNET, SPECIFICALLY NOT SUPPORTING
 	 * OTHER THINGS LIKE FDDI/TokenRing/802.3 SNAPs...
 	 */
-	if (vlan->flags & VLAN_FLAG_REORDER_HDR ||
+	if (skb->protocol != vlan->vlan_proto ||
 	    veth->h_vlan_proto != vlan->vlan_proto) {
 		u16 vlan_tci;
 		vlan_tci = vlan->vlan_id;
@@ -226,7 +226,7 @@ int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
 		     VLAN_FLAG_BRIDGE_BINDING))
 		return -EINVAL;

-	vlan->flags = (old_flags & ~mask) | (flags & mask);
+	WRITE_ONCE(vlan->flags, (old_flags & ~mask) | (flags & mask));

 	if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
 		if (vlan->flags & VLAN_FLAG_GVRP)

Re: [PATCH v2 net] vlan: fix REORDER_HDR race between header and xmit paths

[Jakub Kicinski]

On Wed,  3 Jun 2026 16:10:17 +0800 Yizhou Zhao wrote:
> vlan_dev_change_flags() updates vlan->flags under RTNL, but the VLAN
> data path reads the same field without RTNL.  In particular,
> vlan_dev_hard_header() and vlan_dev_hard_start_xmit() may observe
> different values of VLAN_FLAG_REORDER_HDR for the same skb.
> 
> This can lead to inconsistent tagging.  If REORDER_HDR is cleared when
> vlan_dev_hard_header() runs, the function pushes an in-band VLAN header
> into the skb.  If REORDER_HDR is then observed as set by
> vlan_dev_hard_start_xmit(), the xmit path may also attach a hardware
> accelerated VLAN tag, causing the packet to be emitted with two VLAN
> tags.  Conversely, if the flag changes in the other direction, the skb
> may be emitted without the expected VLAN tag.
> 
> Avoid making the xmit decision depend on a second unsynchronized read of
> vlan->flags.  Instead, use skb->protocol which was set to vlan->vlan_proto
> by vlan_dev_hard_header() when it pushed a VLAN header (REORDER_HDR off),
> or left as the encapsulated protocol otherwise (REORDER_HDR on).
> Checking skb->protocol first also preserves the short-circuit evaluation
> order introduced by commit dacab578c7c6c ("vlan: fix a potential
> uninit-value in vlan_dev_hard_start_xmit()"): when no VLAN header was
> pushed, skb->protocol != vlan->vlan_proto is true and veth->h_vlan_proto
> is not read, avoiding the uninit-value issue.
> 
> Also use READ_ONCE() for the data-path read in vlan_dev_hard_header()
> and WRITE_ONCE() for the control-path update in vlan_dev_change_flags().

# selftests: net/forwarding: mirror_vlan.sh
# 11.35 [+11.35] TEST: ingress mirror to vlan                                       [ OK ]
# 17.47 [+6.12] TEST: egress mirror to vlan                                         [ OK ]
# 23.58 [+6.12] TEST: ingress mirror tagged to vlan                                 [FAIL]
# 23.59 [+0.00] Expected to capture >= 10 packets, got 0.
# 29.70 [+6.11] TEST: egress mirror tagged to vlan                                  [FAIL]
# 29.70 [+0.00] Expected to capture >= 10 packets, got 0.
not ok 1 selftests: net/forwarding: mirror_vlan.sh # exit=1