[PATCH 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 the packet contents already produced by
vlan_dev_hard_header(): when an in-band VLAN header was pushed,
veth->h_vlan_proto matches vlan->vlan_proto and no additional hwaccel tag
is needed; otherwise the packet still carries the encapsulated protocol
and the xmit path must add the VLAN tag.

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().

Reported-by: Yizhou Zhao <zhaoyz24@xxxxxxxxxxxxxxxxxxxxx>
Reported-by: Yuxiang Yang <yangyx22@xxxxxxxxxxxxxxxxxxxxx>
Reported-by: Ao Wang <wangao@xxxxxxxxxx>
Reported-by: Xuewei Feng <fengxw06@xxxxxxx>
Reported-by: Qi Li <qli01@xxxxxxxxxxxxxxx>
Reported-by: Ke Xu <xuke@xxxxxxxxxxxxxxx>
Signed-off-by: Yizhou Zhao <zhaoyz24@xxxxxxxxxxxxxxxxxxxxx>
---
 net/8021q/vlan_dev.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index c40f7d5..30cabf7 100644
--- 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;
@@ -109,9 +109,18 @@ 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...
+	 *
+	 * Do not re-read VLAN_FLAG_REORDER_HDR here.  The flag may have been
+	 * changed after vlan_dev_hard_header() ran, so using it again can make
+	 * the xmit path disagree with the header path for the same skb.
+	 *
+	 * Instead, look at the packet contents produced by the header path.  If
+	 * an in-band VLAN header was pushed, h_vlan_proto already matches the
+	 * VLAN protocol and no hwaccel tag is needed.  Otherwise h_vlan_proto is
+	 * the encapsulated protocol and the tag must be added here.
 	 */
-	if (vlan->flags & VLAN_FLAG_REORDER_HDR ||
-	    veth->h_vlan_proto != vlan->vlan_proto) {
+	if (veth->h_vlan_proto != vlan->vlan_proto) {
 		u16 vlan_tci;
+
 		vlan_tci = vlan->vlan_id;
 		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
 		__vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
@@ -223,7 +232,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)
-- 
2.43.0