[PATCH net-next v2 0/9] net: bridge: vlan: minor cleanups and annotations

[PATCH net-next v2 0/9] net: bridge: vlan: minor cleanups and annotations

[Nikolay Aleksandrov]

Hi,
In preparation for a complete vlan fast-path re-work, this set cleans up
a few minor things - removing legacy code barriers that don't help,
                     annotating lockless accesses and using proper helpers.
No functional changes are intended. Sashiko will find many pre-existing
problems, none of them are related to this set. I will try to fix
everything pre-existing separately in -net, there will probably be a few
critical items. So if there are missed places to annotate, or if there're
actual functional changes, these obviously should be addressed in this set.


v2: fix a bit longer lines in patch 8 (were 83 chars)
    fix patch 9, many places were missed, use br_vlan_get_state
    consistently (Ido)

Thanks,
 Nik

Nikolay Aleksandrov (9):
  net: bridge: vlan: drop legacy memory barriers
  net: bridge: vlan: annotate lockless pvid use
  net: bridge: mst: use br_get_pvid helper
  net: bridge: vlan: annotate lockless vlan flags use
  net: bridge: vlan: annotate lockless use of private flags
  net: bridge: vlan: annotate lockless use of num_vlans
  net: bridge: vlan: annotate lockless use of msti
  net: bridge: vlan: add missing tinfo.tunnel_id annotations
  net: bridge: use br_vlan_get_state to get vlan state

 net/bridge/br_arp_nd_proxy.c   |  6 ++++--
 net/bridge/br_mst.c            | 19 ++++++++++-------
 net/bridge/br_multicast.c      | 18 ++++++++++------
 net/bridge/br_netlink.c        | 10 ++++-----
 net/bridge/br_netlink_tunnel.c | 14 ++++++------
 net/bridge/br_private.h        | 37 +++++++++++++++++---------------
 net/bridge/br_vlan.c           | 39 +++++++++++++++++-----------------
 net/bridge/br_vlan_options.c   |  8 ++++---
 net/bridge/br_vlan_tunnel.c    |  2 +-
 9 files changed, 85 insertions(+), 68 deletions(-)

-- 
2.47.3

[PATCH net-next v2 1/9] net: bridge: vlan: drop legacy memory barriers

[Nikolay Aleksandrov]

These memory barriers are legacy code from the old vlan implementation
that is not needed anymore. They're also affecting the fast-path.
The pvid is changed under lock and is read with rcu only in the fast-path
there is no need for a barrier, nothing to commit prior to changing it nor
invalidate prior to reading.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_private.h | 1 -
 net/bridge/br_vlan.c    | 2 --
 2 files changed, 3 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index d337b1cfb980..5b2c22ad669e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1687,7 +1687,6 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
 	if (!vg)
 		return 0;
 
-	smp_rmb();
 	return vg->pvid;
 }
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1e0e436629ec..2ae6ffb23399 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -40,7 +40,6 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 	if (vg->pvid == v->vid)
 		return;
 
-	smp_wmb();
 	br_vlan_set_pvid_state(vg, v->state);
 	vg->pvid = v->vid;
 }
@@ -50,7 +49,6 @@ static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 	if (vg->pvid != vid)
 		return;
 
-	smp_wmb();
 	vg->pvid = 0;
 }
 
-- 
2.47.3

[PATCH net-next v2 2/9] net: bridge: vlan: annotate lockless pvid use

[Nikolay Aleksandrov]

Use READ/WRITE_ONCE to annotate lockless pvid use, br_get_pvid() is the
helper that should be used outside of the bridge vlan code. The rest of
the vlan places that access pvid directly all have rtnl.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_private.h | 2 +-
 net/bridge/br_vlan.c    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 5b2c22ad669e..0efd2e79e255 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1687,7 +1687,7 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
 	if (!vg)
 		return 0;
 
-	return vg->pvid;
+	return READ_ONCE(vg->pvid);
 }
 
 static inline u16 br_vlan_flags(const struct net_bridge_vlan *v, u16 pvid)
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 2ae6ffb23399..a53054fcc093 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -41,7 +41,7 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 		return;
 
 	br_vlan_set_pvid_state(vg, v->state);
-	vg->pvid = v->vid;
+	WRITE_ONCE(vg->pvid, v->vid);
 }
 
 static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
@@ -49,7 +49,7 @@ static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 	if (vg->pvid != vid)
 		return;
 
-	vg->pvid = 0;
+	WRITE_ONCE(vg->pvid, 0);
 }
 
 /* Update the BRIDGE_VLAN_INFO_PVID and BRIDGE_VLAN_INFO_UNTAGGED flags of @v.
-- 
2.47.3

[PATCH net-next v2 3/9] net: bridge: mst: use br_get_pvid helper

[Nikolay Aleksandrov]

The only place that accessed pvid directly outside of the bridge vlan code
was mst, change it to use the br_get_pvid helper.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_mst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
index 43a300ae6bfa..95e49ec8065e 100644
--- a/net/bridge/br_mst.c
+++ b/net/bridge/br_mst.c
@@ -86,7 +86,7 @@ static void br_mst_vlan_set_state(struct net_bridge_vlan_group *vg,
 	if (br_vlan_get_state(v) == state)
 		return;
 
-	if (v->vid == vg->pvid)
+	if (v->vid == br_get_pvid(vg))
 		br_vlan_set_pvid_state(vg, state);
 
 	br_vlan_set_state(v, state);
-- 
2.47.3

[PATCH net-next v2 4/9] net: bridge: vlan: annotate lockless vlan flags use

[Nikolay Aleksandrov]

Annotate vlan flags data races, they can be read lockless.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_multicast.c |  2 +-
 net/bridge/br_netlink.c   |  6 +++---
 net/bridge/br_private.h   |  4 ++--
 net/bridge/br_vlan.c      | 19 +++++++++++--------
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3ef5d8bbf552..bbcaedf15ca1 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -912,7 +912,7 @@ static void __br_multicast_query_handle_vlan(struct net_bridge_mcast *brmctx,
 	else if (br_multicast_ctx_is_vlan(brmctx))
 		vlan = brmctx->vlan;
 
-	if (vlan && !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
+	if (vlan && !(READ_ONCE(vlan->flags) & BRIDGE_VLAN_INFO_UNTAGGED)) {
 		u16 vlan_proto;
 
 		if (br_vlan_get_proto(brmctx->br->dev, &vlan_proto) != 0)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index b2cd4e39326d..62ce96d9ba32 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -41,7 +41,7 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg,
 		if (v->vid == pvid)
 			flags |= BRIDGE_VLAN_INFO_PVID;
 
-		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		if (READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_UNTAGGED)
 			flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 
 		if (vid_range_start == 0) {
@@ -385,7 +385,7 @@ static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
 		if (v->vid == pvid)
 			flags |= BRIDGE_VLAN_INFO_PVID;
 
-		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		if (READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_UNTAGGED)
 			flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 
 		if (vid_range_start == 0) {
@@ -437,7 +437,7 @@ static int br_fill_ifvlaninfo(struct sk_buff *skb,
 		if (v->vid == pvid)
 			vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
 
-		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		if (READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_UNTAGGED)
 			vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 
 		if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 0efd2e79e255..96554d293d22 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -665,13 +665,13 @@ static inline int br_is_root_bridge(const struct net_bridge *br)
 /* check if a VLAN entry is global */
 static inline bool br_vlan_is_master(const struct net_bridge_vlan *v)
 {
-	return v->flags & BRIDGE_VLAN_INFO_MASTER;
+	return READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_MASTER;
 }
 
 /* check if a VLAN entry is used by the bridge */
 static inline bool br_vlan_is_brentry(const struct net_bridge_vlan *v)
 {
-	return v->flags & BRIDGE_VLAN_INFO_BRENTRY;
+	return READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_BRENTRY;
 }
 
 /* check if we should use the vlan entry, returns false if it's only context */
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index a53054fcc093..c8b2e3bfd42d 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -60,6 +60,7 @@ static bool __vlan_flags_update(struct net_bridge_vlan *v, u16 flags,
 				bool commit)
 {
 	struct net_bridge_vlan_group *vg;
+	u16 vlan_flags;
 	bool change;
 
 	if (br_vlan_is_master(v))
@@ -68,8 +69,9 @@ static bool __vlan_flags_update(struct net_bridge_vlan *v, u16 flags,
 		vg = nbp_vlan_group(v->port);
 
 	/* check if anything would be changed on commit */
+	vlan_flags = v->flags;
 	change = !!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
-		 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED);
+		 ((flags ^ vlan_flags) & BRIDGE_VLAN_INFO_UNTAGGED);
 
 	if (!commit)
 		goto out;
@@ -80,9 +82,10 @@ static bool __vlan_flags_update(struct net_bridge_vlan *v, u16 flags,
 		__vlan_delete_pvid(vg, v->vid);
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
-		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
+		vlan_flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 	else
-		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
+		vlan_flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
+	WRITE_ONCE(v->flags, vlan_flags);
 
 out:
 	return change;
@@ -413,7 +416,7 @@ static int __vlan_del(struct net_bridge_vlan *v)
 	}
 
 	if (br_vlan_should_use(v)) {
-		v->flags &= ~BRIDGE_VLAN_INFO_BRENTRY;
+		WRITE_ONCE(v->flags, v->flags & ~BRIDGE_VLAN_INFO_BRENTRY);
 		vg->num_vlans--;
 	}
 
@@ -522,7 +525,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 	 * hardware on each egress port as appropriate. So only strip the VLAN
 	 * header if forwarding offload is not being used.
 	 */
-	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
+	if (READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_UNTAGGED &&
 	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
 		__vlan_hwaccel_clear_tag(skb);
 
@@ -753,7 +756,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
 		}
 
 		refcount_inc(&vlan->refcnt);
-		vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
+		WRITE_ONCE(vlan->flags, vlan->flags | BRIDGE_VLAN_INFO_BRENTRY);
 		vg->num_vlans++;
 		*changed = true;
 		br_multicast_toggle_one_vlan(vlan, true);
@@ -1489,7 +1492,7 @@ int br_vlan_fill_forward_path_mode(struct net_bridge *br,
 	if (!v || !br_vlan_should_use(v))
 		return -EINVAL;
 
-	if (!(v->flags & BRIDGE_VLAN_INFO_UNTAGGED))
+	if (!(READ_ONCE(v->flags) & BRIDGE_VLAN_INFO_UNTAGGED))
 		return 0;
 
 	if (path->bridge.vlan_mode == DEV_PATH_BR_VLAN_TAG)
@@ -1550,7 +1553,7 @@ int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
 		return -ENOENT;
 
 	p_vinfo->vid = vid;
-	p_vinfo->flags = v->flags;
+	p_vinfo->flags = READ_ONCE(v->flags);
 	if (vid == br_get_pvid(vg))
 		p_vinfo->flags |= BRIDGE_VLAN_INFO_PVID;
 	return 0;
-- 
2.47.3

[PATCH net-next v2 5/9] net: bridge: vlan: annotate lockless use of private flags

[Nikolay Aleksandrov]

Annotate vlan private flags data races, they can be read lockless.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_arp_nd_proxy.c |  6 ++++--
 net/bridge/br_multicast.c    | 16 ++++++++++------
 net/bridge/br_private.h      |  9 ++++++---
 net/bridge/br_vlan.c         |  2 +-
 net/bridge/br_vlan_options.c |  6 ++++--
 5 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 87a40e2f1c50..b82fa6589ab9 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -521,7 +521,8 @@ bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid)
 		v = br_vlan_find(vg, vid);
 		if (!v)
 			return false;
-		return !!(v->priv_flags & BR_VLFLAG_NEIGH_SUPPRESS_ENABLED);
+		return !!(READ_ONCE(v->priv_flags) &
+			  BR_VLFLAG_NEIGH_SUPPRESS_ENABLED);
 	}
 	return test_bit(BR_NEIGH_SUPPRESS_BIT, &p->flags);
 }
@@ -535,7 +536,8 @@ bool br_is_neigh_forward_grat_enabled(const struct net_bridge_port *p, u16 vid)
 		v = br_vlan_find(vg, vid);
 		if (!v)
 			return false;
-		return !!(v->priv_flags & BR_VLFLAG_NEIGH_FORWARD_GRAT_ENABLED);
+		return !!(READ_ONCE(v->priv_flags) &
+			  BR_VLFLAG_NEIGH_FORWARD_GRAT_ENABLED);
 	}
 	return test_bit(BR_NEIGH_FORWARD_GRAT_BIT, &p->flags);
 }
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index bbcaedf15ca1..cd984173320f 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2172,7 +2172,7 @@ static void br_multicast_enable_port_ctx(struct net_bridge_mcast_port *pmctx)
 
 	spin_lock_bh(&br->multicast_lock);
 	if (br_multicast_port_ctx_is_vlan(pmctx) &&
-	    !(pmctx->vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED)) {
+	    !(READ_ONCE(pmctx->vlan->priv_flags) & BR_VLFLAG_MCAST_ENABLED)) {
 		spin_unlock_bh(&br->multicast_lock);
 		return;
 	}
@@ -2209,7 +2209,7 @@ static void br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
 
 	spin_lock_bh(&br->multicast_lock);
 	if (br_multicast_port_ctx_is_vlan(pmctx) &&
-	    !(pmctx->vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED)) {
+	    !(READ_ONCE(pmctx->vlan->priv_flags) & BR_VLFLAG_MCAST_ENABLED)) {
 		spin_unlock_bh(&br->multicast_lock);
 		return;
 	}
@@ -4083,7 +4083,8 @@ int br_multicast_rcv(struct net_bridge_mcast **brmctx,
 			*pmctx = &vlan->port_mcast_ctx;
 		}
 
-		if (!(masterv->priv_flags & BR_VLFLAG_GLOBAL_MCAST_ENABLED))
+		if (!(READ_ONCE(masterv->priv_flags) &
+		      BR_VLFLAG_GLOBAL_MCAST_ENABLED))
 			return 0;
 	}
 
@@ -4383,7 +4384,8 @@ void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on)
 			return;
 
 		spin_lock_bh(&br->multicast_lock);
-		vlan->priv_flags ^= BR_VLFLAG_MCAST_ENABLED;
+		WRITE_ONCE(vlan->priv_flags, vlan->priv_flags ^
+					    BR_VLFLAG_MCAST_ENABLED);
 		spin_unlock_bh(&br->multicast_lock);
 
 		if (on)
@@ -4399,7 +4401,8 @@ void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on)
 
 		br = vlan->port->br;
 		spin_lock_bh(&br->multicast_lock);
-		vlan->priv_flags ^= BR_VLFLAG_MCAST_ENABLED;
+		WRITE_ONCE(vlan->priv_flags, vlan->priv_flags ^
+					    BR_VLFLAG_MCAST_ENABLED);
 		if (on)
 			__br_multicast_enable_port_ctx(&vlan->port_mcast_ctx);
 		else
@@ -4477,7 +4480,8 @@ bool br_multicast_toggle_global_vlan(struct net_bridge_vlan *vlan, bool on)
 	if (on == !!(vlan->priv_flags & BR_VLFLAG_GLOBAL_MCAST_ENABLED))
 		return false;
 
-	vlan->priv_flags ^= BR_VLFLAG_GLOBAL_MCAST_ENABLED;
+	WRITE_ONCE(vlan->priv_flags, vlan->priv_flags ^
+				    BR_VLFLAG_GLOBAL_MCAST_ENABLED);
 	br_multicast_toggle_vlan(vlan, on);
 
 	return true;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 96554d293d22..c7b64b5da012 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1273,21 +1273,24 @@ br_multicast_ctx_vlan_global_disabled(const struct net_bridge_mcast *brmctx)
 {
 	return br_multicast_ctx_is_vlan(brmctx) &&
 	       (!br_opt_get(brmctx->br, BROPT_MCAST_VLAN_SNOOPING_ENABLED) ||
-		!(brmctx->vlan->priv_flags & BR_VLFLAG_GLOBAL_MCAST_ENABLED));
+		!(READ_ONCE(brmctx->vlan->priv_flags) &
+		  BR_VLFLAG_GLOBAL_MCAST_ENABLED));
 }
 
 static inline bool
 br_multicast_ctx_vlan_disabled(const struct net_bridge_mcast *brmctx)
 {
 	return br_multicast_ctx_is_vlan(brmctx) &&
-	       !(brmctx->vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED);
+	       !(READ_ONCE(brmctx->vlan->priv_flags) &
+		 BR_VLFLAG_MCAST_ENABLED);
 }
 
 static inline bool
 br_multicast_port_ctx_vlan_disabled(const struct net_bridge_mcast_port *pmctx)
 {
 	return br_multicast_port_ctx_is_vlan(pmctx) &&
-	       !(pmctx->vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED);
+	       !(READ_ONCE(pmctx->vlan->priv_flags) &
+		 BR_VLFLAG_MCAST_ENABLED);
 }
 
 static inline bool
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index c8b2e3bfd42d..102792f20a5e 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1497,7 +1497,7 @@ int br_vlan_fill_forward_path_mode(struct net_bridge *br,
 
 	if (path->bridge.vlan_mode == DEV_PATH_BR_VLAN_TAG)
 		path->bridge.vlan_mode = DEV_PATH_BR_VLAN_KEEP;
-	else if (v->priv_flags & BR_VLFLAG_TAGGING_BY_SWITCHDEV)
+	else if (READ_ONCE(v->priv_flags) & BR_VLFLAG_TAGGING_BY_SWITCHDEV)
 		path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG_HW;
 	else
 		path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG;
diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
index cb0f556ff40d..bd049d268fe7 100644
--- a/net/bridge/br_vlan_options.c
+++ b/net/bridge/br_vlan_options.c
@@ -276,7 +276,8 @@ static int br_vlan_process_one_opts(const struct net_bridge *br,
 		}
 
 		if (val != enabled) {
-			v->priv_flags ^= BR_VLFLAG_NEIGH_SUPPRESS_ENABLED;
+			WRITE_ONCE(v->priv_flags, v->priv_flags ^
+				   BR_VLFLAG_NEIGH_SUPPRESS_ENABLED);
 			*changed = true;
 		}
 	}
@@ -292,7 +293,8 @@ static int br_vlan_process_one_opts(const struct net_bridge *br,
 		}
 
 		if (val != enabled) {
-			v->priv_flags ^= BR_VLFLAG_NEIGH_FORWARD_GRAT_ENABLED;
+			WRITE_ONCE(v->priv_flags, v->priv_flags ^
+				   BR_VLFLAG_NEIGH_FORWARD_GRAT_ENABLED);
 			*changed = true;
 		}
 	}
-- 
2.47.3

[PATCH net-next v2 6/9] net: bridge: vlan: annotate lockless use of num_vlans

[Nikolay Aleksandrov]

Annotate data races around vlan group's num_vlans, it can be read lockless.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_netlink.c |  4 ++--
 net/bridge/br_vlan.c    | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 62ce96d9ba32..ae76df0de05a 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -81,7 +81,7 @@ static int br_get_num_vlan_infos(struct net_bridge_vlan_group *vg,
 		return 0;
 
 	if (filter_mask & RTEXT_FILTER_BRVLAN)
-		return vg->num_vlans;
+		return READ_ONCE(vg->num_vlans);
 
 	rcu_read_lock();
 	num_vlans = __get_num_vlan_infos(vg, filter_mask);
@@ -531,7 +531,7 @@ static int br_fill_ifinfo(struct sk_buff *skb,
 		else
 			vg = br_vlan_group_rcu(br);
 
-		if (!vg || !vg->num_vlans) {
+		if (!vg || !READ_ONCE(vg->num_vlans)) {
 			rcu_read_unlock();
 			goto done;
 		}
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 102792f20a5e..95187a607ec2 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -344,7 +344,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 				goto out_filt;
 			}
 		}
-		vg->num_vlans++;
+		WRITE_ONCE(vg->num_vlans, vg->num_vlans + 1);
 	}
 
 	/* set the state before publishing */
@@ -367,7 +367,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 out_fdb_insert:
 	if (br_vlan_should_use(v)) {
 		br_fdb_find_delete_local(br, p, dev->dev_addr, v->vid);
-		vg->num_vlans--;
+		WRITE_ONCE(vg->num_vlans, vg->num_vlans - 1);
 	}
 
 out_filt:
@@ -417,7 +417,7 @@ static int __vlan_del(struct net_bridge_vlan *v)
 
 	if (br_vlan_should_use(v)) {
 		WRITE_ONCE(v->flags, v->flags & ~BRIDGE_VLAN_INFO_BRENTRY);
-		vg->num_vlans--;
+		WRITE_ONCE(vg->num_vlans, vg->num_vlans - 1);
 	}
 
 	if (masterv != v) {
@@ -695,7 +695,7 @@ bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
 		return true;
 
 	vg = nbp_vlan_group_rcu(p);
-	if (!vg || !vg->num_vlans)
+	if (!vg || !READ_ONCE(vg->num_vlans))
 		return false;
 
 	if (!br_vlan_get_tag(skb, vid) && skb->vlan_proto != br->vlan_proto)
@@ -757,7 +757,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
 
 		refcount_inc(&vlan->refcnt);
 		WRITE_ONCE(vlan->flags, vlan->flags | BRIDGE_VLAN_INFO_BRENTRY);
-		vg->num_vlans++;
+		WRITE_ONCE(vg->num_vlans, vg->num_vlans + 1);
 		*changed = true;
 		br_multicast_toggle_one_vlan(vlan, true);
 	}
-- 
2.47.3

[PATCH net-next v2 7/9] net: bridge: vlan: annotate lockless use of msti

[Nikolay Aleksandrov]

Annotate data races around vlan msti field, it can be read lockless.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_mst.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
index 95e49ec8065e..76a5bf287578 100644
--- a/net/bridge/br_mst.c
+++ b/net/bridge/br_mst.c
@@ -123,7 +123,7 @@ int br_mst_set_state(struct net_bridge_port *p, u16 msti, u8 state,
 
 	err = 0;
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
-		if (v->brvlan->msti != msti)
+		if (READ_ONCE(v->brvlan->msti) != msti)
 			continue;
 
 		br_mst_vlan_set_state(vg, v, state);
@@ -176,7 +176,7 @@ int br_mst_vlan_set_msti(struct net_bridge_vlan *mv, u16 msti)
 	if (err && err != -EOPNOTSUPP)
 		return err;
 
-	mv->msti = msti;
+	WRITE_ONCE(mv->msti, msti);
 
 	list_for_each_entry(p, &mv->br->port_list, list) {
 		vg = nbp_vlan_group(p);
@@ -249,7 +249,9 @@ size_t br_mst_info_size(const struct net_bridge_vlan_group *vg)
 	sz = nla_total_size(0);
 
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
-		if (test_bit(v->brvlan->msti, seen))
+		u16 msti = READ_ONCE(v->brvlan->msti);
+
+		if (test_bit(msti, seen))
 			continue;
 
 		/* IFLA_BRIDGE_MST_ENTRY */
@@ -259,7 +261,7 @@ size_t br_mst_info_size(const struct net_bridge_vlan_group *vg)
 			/* IFLA_BRIDGE_MST_ENTRY_STATE */
 			nla_total_size(sizeof(u8));
 
-		__set_bit(v->brvlan->msti, seen);
+		__set_bit(msti, seen);
 	}
 
 	return sz;
-- 
2.47.3

[PATCH net-next v2 8/9] net: bridge: vlan: add missing tinfo.tunnel_id annotations

[Nikolay Aleksandrov]

When I annotated data races around tinfo.tunnel_id before, I missed a few
places (e.g. bridge notifications, rhashtable compare), so add them now.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: assign tunid_curr/last later due to long lines

 net/bridge/br_netlink_tunnel.c | 14 ++++++++------
 net/bridge/br_vlan_tunnel.c    |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_netlink_tunnel.c b/net/bridge/br_netlink_tunnel.c
index e7eceab5b515..e64054628ac9 100644
--- a/net/bridge/br_netlink_tunnel.c
+++ b/net/bridge/br_netlink_tunnel.c
@@ -29,8 +29,10 @@ static size_t __get_vlan_tinfo_size(void)
 bool vlan_tunid_inrange(const struct net_bridge_vlan *v_curr,
 			const struct net_bridge_vlan *v_last)
 {
-	__be32 tunid_curr = tunnel_id_to_key32(v_curr->tinfo.tunnel_id);
-	__be32 tunid_last = tunnel_id_to_key32(v_last->tinfo.tunnel_id);
+	__be32 tunid_curr, tunid_last;
+
+	tunid_curr = tunnel_id_to_key32(READ_ONCE(v_curr->tinfo.tunnel_id));
+	tunid_last = tunnel_id_to_key32(READ_ONCE(v_last->tinfo.tunnel_id));
 
 	return (be32_to_cpu(tunid_curr) - be32_to_cpu(tunid_last)) == 1;
 }
@@ -43,7 +45,7 @@ static int __get_num_vlan_tunnel_infos(struct net_bridge_vlan_group *vg)
 	/* Count number of vlan infos */
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		/* only a context, bridge vlan not activated */
-		if (!br_vlan_should_use(v) || !v->tinfo.tunnel_id)
+		if (!br_vlan_should_use(v) || !READ_ONCE(v->tinfo.tunnel_id))
 			continue;
 
 		if (!vtbegin) {
@@ -124,19 +126,19 @@ static int br_fill_vlan_tinfo_range(struct sk_buff *skb,
 	if (vtend && (vtend->vid - vtbegin->vid) > 0) {
 		/* add range to skb */
 		err = br_fill_vlan_tinfo(skb, vtbegin->vid,
-					 vtbegin->tinfo.tunnel_id,
+					 READ_ONCE(vtbegin->tinfo.tunnel_id),
 					 BRIDGE_VLAN_INFO_RANGE_BEGIN);
 		if (err)
 			return err;
 
 		err = br_fill_vlan_tinfo(skb, vtend->vid,
-					 vtend->tinfo.tunnel_id,
+					 READ_ONCE(vtend->tinfo.tunnel_id),
 					 BRIDGE_VLAN_INFO_RANGE_END);
 		if (err)
 			return err;
 	} else {
 		err = br_fill_vlan_tinfo(skb, vtbegin->vid,
-					 vtbegin->tinfo.tunnel_id,
+					 READ_ONCE(vtbegin->tinfo.tunnel_id),
 					 0);
 		if (err)
 			return err;
diff --git a/net/bridge/br_vlan_tunnel.c b/net/bridge/br_vlan_tunnel.c
index 257cae9f1569..04751f15d708 100644
--- a/net/bridge/br_vlan_tunnel.c
+++ b/net/bridge/br_vlan_tunnel.c
@@ -22,7 +22,7 @@ static inline int br_vlan_tunid_cmp(struct rhashtable_compare_arg *arg,
 	const struct net_bridge_vlan *vle = ptr;
 	__be64 tunid = *(__be64 *)arg->key;
 
-	return vle->tinfo.tunnel_id != tunid;
+	return READ_ONCE(vle->tinfo.tunnel_id) != tunid;
 }
 
 static const struct rhashtable_params br_vlan_tunnel_rht_params = {
-- 
2.47.3

[PATCH net-next v2 9/9] net: bridge: use br_vlan_get_state to get vlan state

[Nikolay Aleksandrov]

The vlan state can change under RTNL or RCU (mst), so use the helper
to get the state properly throughout the bridge driver.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: use br_vlan_get_state consistently where needed (Ido)

 net/bridge/br_mst.c          |  7 ++++---
 net/bridge/br_private.h      | 21 +++++++++++----------
 net/bridge/br_vlan.c         |  2 +-
 net/bridge/br_vlan_options.c |  2 +-
 4 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
index 76a5bf287578..ecd8bba7430e 100644
--- a/net/bridge/br_mst.c
+++ b/net/bridge/br_mst.c
@@ -70,7 +70,7 @@ int br_mst_get_state(const struct net_device *dev, u16 msti, u8 *state)
 
 	list_for_each_entry(v, &vg->vlan_list, vlist) {
 		if (v->brvlan->msti == msti) {
-			*state = v->state;
+			*state = br_vlan_get_state(v);
 			return 0;
 		}
 	}
@@ -145,7 +145,7 @@ static void br_mst_vlan_sync_state(struct net_bridge_vlan *pv, u16 msti)
 		 * it.
 		 */
 		if (v != pv && v->brvlan->msti == msti) {
-			br_mst_vlan_set_state(vg, pv, v->state);
+			br_mst_vlan_set_state(vg, pv, br_vlan_get_state(v));
 			return;
 		}
 	}
@@ -282,7 +282,8 @@ int br_mst_fill_info(struct sk_buff *skb,
 		nest = nla_nest_start_noflag(skb, IFLA_BRIDGE_MST_ENTRY);
 		if (!nest ||
 		    nla_put_u16(skb, IFLA_BRIDGE_MST_ENTRY_MSTI, v->brvlan->msti) ||
-		    nla_put_u8(skb, IFLA_BRIDGE_MST_ENTRY_STATE, v->state)) {
+		    nla_put_u8(skb, IFLA_BRIDGE_MST_ENTRY_STATE,
+			       br_vlan_get_state(v))) {
 			err = -EMSGSIZE;
 			break;
 		}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index c7b64b5da012..df1500391f1d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -687,6 +687,15 @@ static inline bool br_vlan_should_use(const struct net_bridge_vlan *v)
 	return true;
 }
 
+/* The vlan state can be changed with only rcu held by the mst code so
+ * annotate the lock-free read. br_vlan_set_state() is kept further down
+ * because it needs br_multicast_update_vlan_mcast_ctx().
+ */
+static inline u8 br_vlan_get_state(const struct net_bridge_vlan *v)
+{
+	return READ_ONCE(v->state);
+}
+
 static inline bool nbp_state_should_learn(const struct net_bridge_port *p)
 {
 	return p->state == BR_STATE_LEARNING || p->state == BR_STATE_FORWARDING;
@@ -1299,7 +1308,7 @@ br_multicast_port_ctx_state_disabled(const struct net_bridge_mcast_port *pmctx)
 	return pmctx->port->state == BR_STATE_DISABLED ||
 	       (br_multicast_port_ctx_is_vlan(pmctx) &&
 		(br_multicast_port_ctx_vlan_disabled(pmctx) ||
-		 pmctx->vlan->state == BR_STATE_DISABLED));
+		 br_vlan_get_state(pmctx->vlan) == BR_STATE_DISABLED));
 }
 
 static inline bool
@@ -1308,7 +1317,7 @@ br_multicast_port_ctx_state_stopped(const struct net_bridge_mcast_port *pmctx)
 	return br_multicast_port_ctx_state_disabled(pmctx) ||
 	       pmctx->port->state == BR_STATE_BLOCKING ||
 	       (br_multicast_port_ctx_is_vlan(pmctx) &&
-		pmctx->vlan->state == BR_STATE_BLOCKING);
+		br_vlan_get_state(pmctx->vlan) == BR_STATE_BLOCKING);
 }
 
 static inline bool
@@ -1916,14 +1925,6 @@ bool br_vlan_global_opts_can_enter_range(const struct net_bridge_vlan *v_curr,
 bool br_vlan_global_opts_fill(struct sk_buff *skb, u16 vid, u16 vid_range,
 			      const struct net_bridge_vlan *v_opts);
 
-/* vlan state manipulation helpers using *_ONCE to annotate lock-free access,
- * while br_vlan_set_state() may access data protected by multicast_lock.
- */
-static inline u8 br_vlan_get_state(const struct net_bridge_vlan *v)
-{
-	return READ_ONCE(v->state);
-}
-
 static inline void br_vlan_set_state(struct net_bridge_vlan *v, u8 state)
 {
 	WRITE_ONCE(v->state, state);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 95187a607ec2..72b75757ac85 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -40,7 +40,7 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 	if (vg->pvid == v->vid)
 		return;
 
-	br_vlan_set_pvid_state(vg, v->state);
+	br_vlan_set_pvid_state(vg, br_vlan_get_state(v));
 	WRITE_ONCE(vg->pvid, v->vid);
 }
 
diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
index bd049d268fe7..506668fc31d9 100644
--- a/net/bridge/br_vlan_options.c
+++ b/net/bridge/br_vlan_options.c
@@ -43,7 +43,7 @@ bool br_vlan_opts_eq_range(const struct net_bridge_vlan *v_curr,
 	u8 range_mc_rtr = br_vlan_multicast_router(range_end);
 	u8 curr_mc_rtr = br_vlan_multicast_router(v_curr);
 
-	if (v_curr->state != range_end->state)
+	if (br_vlan_get_state(v_curr) != br_vlan_get_state(range_end))
 		return false;
 
 	if (!__vlan_tun_can_enter_range(v_curr, range_end))
-- 
2.47.3

Re: [PATCH net-next v2 1/9] net: bridge: vlan: drop legacy memory barriers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

- [Medium] Writer-side publish ordering removed: in `__vlan_add()`
  (net/bridge/br_vlan.c) a new VLAN is made visible to RCU readers…

Pre-existing issues:
- [Low] Concern (retained for policy combination, but validated as
  resolved within this series): after this patch `vg->pvid` is read and…

--

net: bridge: vlan: drop legacy memory barriers

This removes the smp_wmb() calls from __vlan_add_pvid() and
__vlan_delete_pvid() and the smp_rmb() from br_get_pvid(), on the grounds
that the pvid is written under lock and read under rcu in the fast-path.
A couple of questions about the write side below.

> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index d337b1cfb980..5b2c22ad669e 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1687,7 +1687,6 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
>  	if (!vg)
>  		return 0;
>  
> -	smp_rmb();
>  	return vg->pvid;
>  }

[Severity: Low]
This isn't a bug introduced by this patch, but with the barriers gone
vg->pvid is left as a plain C access on both sides: br_get_pvid() does
"return vg->pvid;", and the writers in br_vlan.c do "vg->pvid = v->vid;"
and "vg->pvid = 0;". There is also a direct read outside the helper in
br_mst_vlan_set_state():

net/bridge/br_mst.c:br_mst_vlan_set_state() {
	...
	if (v->vid == vg->pvid)
	...
}

The sibling field in the same struct is only touched through accessors
that mark the access:

net/bridge/br_private.h {
	static inline void br_vlan_set_pvid_state(struct net_bridge_vlan_group *vg,
						  u8 state)
	{
		WRITE_ONCE(vg->pvid_state, state);
	}
}

The follow-up patches in this series do address this - "net: bridge: vlan:
annotate lockless pvid use" converts br_get_pvid() to READ_ONCE() and both
writers to WRITE_ONCE(), and "net: bridge: mst: use br_get_pvid helper"
converts the br_mst.c read - so this only applies to the intermediate tree
state at this commit. Would it be simpler to fold the annotation into this
patch so no intermediate state has an unmarked pvid access?

> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1e0e436629ec..2ae6ffb23399 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -40,7 +40,6 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>  	if (vg->pvid == v->vid)
>  		return;
>  
> -	smp_wmb();
>  	br_vlan_set_pvid_state(vg, v->state);
>  	vg->pvid = v->vid;
>  }

[Severity: Medium]
The commit message says "nothing to commit prior to changing it".  Is that
accurate for the add path?  In __vlan_add() the vlan is published to rcu
readers first, and the pvid store happens after that:

net/bridge/br_vlan.c:__vlan_add() {
	...
	/* set the state before publishing */
	br_vlan_init_state(v);

	err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode,
					    br_vlan_rht_params);
	if (err)
		goto out_fdb_insert;

	__vlan_add_list(v);
	__vlan_flags_commit(v, flags);	/* -> __vlan_add_pvid() */
	...
}

So the removed smp_wmb() sat between the rhashtable insert / list add and
the vg->pvid store.  On a weakly ordered architecture, can a fast-path
reader observe the new vg->pvid before the matching net_bridge_vlan is
visible in vg->vlan_hash?  The rhashtable bucket store has release
semantics for the insert itself, but nothing orders the later plain pvid
store after it, and the WRITE_ONCE() added by the next patch in the series
does not provide store-store ordering either.

If that can happen, the reader in __allowed_ingress() picks up the new pvid
and then fails the lookup:

net/bridge/br_vlan.c:__allowed_ingress() {
	...
	v = br_vlan_find(vg, *vid);
	if (!v || !br_vlan_should_use(v))
		goto drop;
	...
}

which would drop untagged/priority-tagged frames for the length of the
window (this lookup is reached when BROPT_MCAST_VLAN_SNOOPING_ENABLED or
BROPT_VLAN_STATS_ENABLED is set), triggered by something like
"bridge vlan add dev <port> vid N pvid untagged" for a new vlan while
untagged traffic is received on that port.

For the read side the analysis looks fine: the smp_rmb() in br_get_pvid()
preceded the pvid load, so it never ordered the pvid load against the
following hash lookup - that direction comes from the address dependency
into the bucket computation.

If the write-side ordering is still wanted without a fence in the
fast-path, would smp_store_release(&vg->pvid, v->vid) here be the right
replacement?

> @@ -50,7 +49,6 @@ static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>  	if (vg->pvid != vid)
>  		return;
>  
> -	smp_wmb();
>  	vg->pvid = 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903143138.1574598-1-razor%40blackwall.org

Re: [PATCH net-next v2 1/9] net: bridge: vlan: drop legacy memory barriers

[Nikolay Aleksandrov]

On 05/09/2026 23:27, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
> 
> - [Medium] Writer-side publish ordering removed: in `__vlan_add()`
>    (net/bridge/br_vlan.c) a new VLAN is made visible to RCU readers…
> 
> Pre-existing issues:
> - [Low] Concern (retained for policy combination, but validated as
>    resolved within this series): after this patch `vg->pvid` is read and…
> 

I don't think there's anything wrong with the patch, I've answered below.

> --
> 
> net: bridge: vlan: drop legacy memory barriers
> 
> This removes the smp_wmb() calls from __vlan_add_pvid() and
> __vlan_delete_pvid() and the smp_rmb() from br_get_pvid(), on the grounds
> that the pvid is written under lock and read under rcu in the fast-path.
> A couple of questions about the write side below.
> 
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index d337b1cfb980..5b2c22ad669e 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -1687,7 +1687,6 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
>>   	if (!vg)
>>   		return 0;
>>   
>> -	smp_rmb();
>>   	return vg->pvid;
>>   }
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but with the barriers gone
> vg->pvid is left as a plain C access on both sides: br_get_pvid() does
> "return vg->pvid;", and the writers in br_vlan.c do "vg->pvid = v->vid;"
> and "vg->pvid = 0;". There is also a direct read outside the helper in
> br_mst_vlan_set_state():
> 
> net/bridge/br_mst.c:br_mst_vlan_set_state() {
> 	...
> 	if (v->vid == vg->pvid)
> 	...
> }
> 
> The sibling field in the same struct is only touched through accessors
> that mark the access:
> 
> net/bridge/br_private.h {
> 	static inline void br_vlan_set_pvid_state(struct net_bridge_vlan_group *vg,
> 						  u8 state)
> 	{
> 		WRITE_ONCE(vg->pvid_state, state);
> 	}
> }
> 
> The follow-up patches in this series do address this - "net: bridge: vlan:
> annotate lockless pvid use" converts br_get_pvid() to READ_ONCE() and both
> writers to WRITE_ONCE(), and "net: bridge: mst: use br_get_pvid helper"
> converts the br_mst.c read - so this only applies to the intermediate tree
> state at this commit. Would it be simpler to fold the annotation into this
> patch so no intermediate state has an unmarked pvid access?
> 

This is not the point of the change here and it isn't relevant at all.

>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 1e0e436629ec..2ae6ffb23399 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -40,7 +40,6 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>>   	if (vg->pvid == v->vid)
>>   		return;
>>   
>> -	smp_wmb();
>>   	br_vlan_set_pvid_state(vg, v->state);
>>   	vg->pvid = v->vid;
>>   }
> 
> [Severity: Medium]
> The commit message says "nothing to commit prior to changing it".  Is that
> accurate for the add path?  In __vlan_add() the vlan is published to rcu
> readers first, and the pvid store happens after that:
> 
> net/bridge/br_vlan.c:__vlan_add() {
> 	...
> 	/* set the state before publishing */
> 	br_vlan_init_state(v);
> 
> 	err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode,
> 					    br_vlan_rht_params);
> 	if (err)
> 		goto out_fdb_insert;
> 
> 	__vlan_add_list(v);
> 	__vlan_flags_commit(v, flags);	/* -> __vlan_add_pvid() */
> 	...
> }
> 
> So the removed smp_wmb() sat between the rhashtable insert / list add and
> the vg->pvid store.  On a weakly ordered architecture, can a fast-path
> reader observe the new vg->pvid before the matching net_bridge_vlan is
> visible in vg->vlan_hash?  The rhashtable bucket store has release
> semantics for the insert itself, but nothing orders the later plain pvid
> store after it, and the WRITE_ONCE() added by the next patch in the series
> does not provide store-store ordering either.
> 
> If that can happen, the reader in __allowed_ingress() picks up the new pvid
> and then fails the lookup:
> 
> net/bridge/br_vlan.c:__allowed_ingress() {
> 	...
> 	v = br_vlan_find(vg, *vid);
> 	if (!v || !br_vlan_should_use(v))
> 		goto drop;
> 	...
> }
> 
> which would drop untagged/priority-tagged frames for the length of the
> window (this lookup is reached when BROPT_MCAST_VLAN_SNOOPING_ENABLED or
> BROPT_VLAN_STATS_ENABLED is set), triggered by something like
> "bridge vlan add dev <port> vid N pvid untagged" for a new vlan while
> untagged traffic is received on that port.
> 
> For the read side the analysis looks fine: the smp_rmb() in br_get_pvid()
> preceded the pvid load, so it never ordered the pvid load against the
> following hash lookup - that direction comes from the address dependency
> into the bucket computation.
> 
> If the write-side ordering is still wanted without a fence in the
> fast-path, would smp_store_release(&vg->pvid, v->vid) here be the right
> replacement?
> 

This synchronization window is acceptable, we might drop a few packets. The write barrier
here keeping it shorter was really an accident, the barriers predate my conversion of
the vlan code to rhashtable when bitmaps were used and needed the sync.

Re: [PATCH net-next v2 0/9] net: bridge: vlan: minor cleanups and annotations

[Ido Schimmel]

On Thu, Sep 03, 2026 at 05:31:29PM +0300, Nikolay Aleksandrov wrote:
> Hi,
> In preparation for a complete vlan fast-path re-work, this set cleans up
> a few minor things - removing legacy code barriers that don't help,
>                      annotating lockless accesses and using proper helpers.
> No functional changes are intended. Sashiko will find many pre-existing
> problems, none of them are related to this set. I will try to fix
> everything pre-existing separately in -net, there will probably be a few
> critical items. So if there are missed places to annotate, or if there're
> actual functional changes, these obviously should be addressed in this set.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>