[PATCH v2 net-next 0/5] net: bridge: take care of p->flags accesses

[PATCH v2 net-next 0/5] net: bridge: take care of p->flags accesses

[Eric Dumazet]

(struct net_bridge_port)->flags can be read/written locklessly,
and thus can fire KCSAN warnings, or real bugs.

Prefer atomic operations (test_bit(), clear_bit(), set_bit())
and use READ_ONCE() for the remaining uses.

v2: addressed Nikolay's feedback on patches 3 & 4.

Eric Dumazet (5):
  bridge: use atomic ops to read/change p->flags in sysfs
  bridge: use atomic ops to read/change p->flags in br_netlink.c
  net: bridge: use atomic ops to read/change p->flags (I)
  net: bridge: use atomic ops to read/change p->flags (II)
  net: bridge: use atomic ops to read/change p->flags (III)

 net/bridge/br_arp_nd_proxy.c | 22 +++-----
 net/bridge/br_fdb.c          |  2 +-
 net/bridge/br_forward.c      | 15 +++---
 net/bridge/br_if.c           |  2 +-
 net/bridge/br_input.c        | 12 ++---
 net/bridge/br_mrp.c          | 20 ++++----
 net/bridge/br_mrp_netlink.c  |  8 +--
 net/bridge/br_multicast.c    |  5 +-
 net/bridge/br_netlink.c      | 99 +++++++++++++++++++-----------------
 net/bridge/br_stp.c          |  4 +-
 net/bridge/br_stp_bpdu.c     |  2 +-
 net/bridge/br_switchdev.c    |  8 +--
 net/bridge/br_sysfs_if.c     | 61 ++++++++++++----------
 net/bridge/br_vlan_options.c |  2 +-
 14 files changed, 134 insertions(+), 128 deletions(-)

-- 
2.54.0.1099.g489fc7bff1-goog

[PATCH v2 net-next 1/5] bridge: use atomic ops to read/change p->flags in sysfs

[Eric Dumazet]

Change net/bridge/br_sysfs_if.c to use atomic operations
to read/change bits n p->flags.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/bridge/br_sysfs_if.c | 61 ++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 1923c004f0d2b746902f5b6de1bbb72f7f824125..738544901563f570355554f49b2b80cf962f8e71 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -44,40 +44,45 @@ const struct brport_attribute brport_attr_##_name = { 	        \
 	.store	= _store,					\
 };
 
-#define BRPORT_ATTR_FLAG(_name, _mask)				\
+#define BRPORT_ATTR_FLAG(_name, _bitnr)				\
 static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
 {								\
-	return sysfs_emit(buf, "%d\n", !!(p->flags & _mask));	\
+	return sysfs_emit(buf, "%d\n", test_bit(_bitnr, &p->flags));	\
 }								\
 static int store_##_name(struct net_bridge_port *p, unsigned long v) \
 {								\
-	return store_flag(p, v, _mask);				\
+	return store_flag(p, v, _bitnr);				\
 }								\
 static BRPORT_ATTR(_name, 0644,					\
 		   show_##_name, store_##_name)
 
 static int store_flag(struct net_bridge_port *p, unsigned long v,
-		      unsigned long mask)
+		      unsigned long bitnr)
 {
+	unsigned long oflags, flags = READ_ONCE(p->flags);
 	struct netlink_ext_ack extack = {0};
-	unsigned long flags = p->flags;
 	int err;
 
+
+	oflags = flags;
 	if (v)
-		flags |= mask;
+		__set_bit(bitnr, &flags);
 	else
-		flags &= ~mask;
+		__clear_bit(bitnr, &flags);
 
-	if (flags != p->flags) {
-		err = br_switchdev_set_port_flag(p, flags, mask, &extack);
-		if (err) {
-			netdev_err(p->dev, "%s\n", extack._msg);
-			return err;
-		}
+	if (flags == oflags)
+		return 0;
 
-		p->flags = flags;
-		br_port_flags_change(p, mask);
+	err = br_switchdev_set_port_flag(p, flags, BIT(bitnr), &extack);
+	if (err) {
+		netdev_err(p->dev, "%s\n", extack._msg);
+		return err;
 	}
+	if (v)
+		set_bit(bitnr, &p->flags);
+	else
+		clear_bit(bitnr, &p->flags);
+	br_port_flags_change(p, BIT(bitnr));
 	return 0;
 }
 
@@ -247,17 +252,17 @@ static int store_backup_port(struct net_bridge_port *p, char *buf)
 }
 static BRPORT_ATTR_RAW(backup_port, 0644, show_backup_port, store_backup_port);
 
-BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE);
-BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD);
-BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK);
-BRPORT_ATTR_FLAG(learning, BR_LEARNING);
-BRPORT_ATTR_FLAG(unicast_flood, BR_FLOOD);
-BRPORT_ATTR_FLAG(proxyarp, BR_PROXYARP);
-BRPORT_ATTR_FLAG(proxyarp_wifi, BR_PROXYARP_WIFI);
-BRPORT_ATTR_FLAG(multicast_flood, BR_MCAST_FLOOD);
-BRPORT_ATTR_FLAG(broadcast_flood, BR_BCAST_FLOOD);
-BRPORT_ATTR_FLAG(neigh_suppress, BR_NEIGH_SUPPRESS);
-BRPORT_ATTR_FLAG(isolated, BR_ISOLATED);
+BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE_BIT);
+BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD_BIT);
+BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK_BIT);
+BRPORT_ATTR_FLAG(learning, BR_LEARNING_BIT);
+BRPORT_ATTR_FLAG(unicast_flood, BR_FLOOD_BIT);
+BRPORT_ATTR_FLAG(proxyarp, BR_PROXYARP_BIT);
+BRPORT_ATTR_FLAG(proxyarp_wifi, BR_PROXYARP_WIFI_BIT);
+BRPORT_ATTR_FLAG(multicast_flood, BR_MCAST_FLOOD_BIT);
+BRPORT_ATTR_FLAG(broadcast_flood, BR_BCAST_FLOOD_BIT);
+BRPORT_ATTR_FLAG(neigh_suppress, BR_NEIGH_SUPPRESS_BIT);
+BRPORT_ATTR_FLAG(isolated, BR_ISOLATED_BIT);
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 static ssize_t show_multicast_router(struct net_bridge_port *p, char *buf)
@@ -273,8 +278,8 @@ static int store_multicast_router(struct net_bridge_port *p,
 static BRPORT_ATTR(multicast_router, 0644, show_multicast_router,
 		   store_multicast_router);
 
-BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE);
-BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UNICAST);
+BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE_BIT);
+BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UNICAST_BIT);
 #endif
 
 static const struct brport_attribute *brport_attrs[] = {
-- 
2.54.0.1099.g489fc7bff1-goog

[PATCH v2 net-next 2/5] bridge: use atomic ops to read/change p->flags in br_netlink.c

[Eric Dumazet]

Change net/bridge/br_netlink.c to use atomic operations
to read/change bits in p->flags.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/bridge/br_netlink.c | 97 ++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 7cb24de9c77d3d15892723f77288c27a15a6a0ad..12850f60bfcb418918508612070a6b0266ffbe6b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -113,7 +113,7 @@ static size_t br_get_link_af_size_filtered(const struct net_device *dev,
 	num_vlan_infos = br_get_num_vlan_infos(vg, filter_mask);
 	rcu_read_unlock();
 
-	if (p && (p->flags & BR_VLAN_TUNNEL))
+	if (p && test_bit(BR_VLAN_TUNNEL_BIT, &p->flags))
 		vinfo_sz += br_get_vlan_tunnel_info_size(vg);
 
 	/* Each VLAN is returned in bridge_vlan_info along with flags */
@@ -823,7 +823,7 @@ static int br_afspec(struct net_bridge *br,
 		err = 0;
 		switch (nla_type(attr)) {
 		case IFLA_BRIDGE_VLAN_TUNNEL_INFO:
-			if (!p || !(p->flags & BR_VLAN_TUNNEL))
+			if (!p || !test_bit(BR_VLAN_TUNNEL_BIT, &p->flags))
 				return -EINVAL;
 			err = br_parse_vlan_tunnel_info(attr, &tinfo_curr);
 			if (err)
@@ -934,58 +934,64 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
 }
 
 /* Set/clear or port flags based on attribute */
-static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
+static unsigned long br_set_port_flag(unsigned long flags, struct nlattr *tb[],
 			     int attrtype, unsigned long mask)
 {
-	if (!tb[attrtype])
-		return;
-
-	if (nla_get_u8(tb[attrtype]))
-		p->flags |= mask;
-	else
-		p->flags &= ~mask;
+	if (tb[attrtype]) {
+		if (nla_get_u8(tb[attrtype]))
+			flags |= mask;
+		else
+			flags &= ~mask;
+	}
+	return flags;
 }
 
 /* Process bridge protocol info on port */
 static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 		      struct netlink_ext_ack *extack)
 {
-	unsigned long old_flags, changed_mask;
+	unsigned long old_flags, flags, changed_mask;
 	bool br_vlan_tunnel_old;
 	int err;
 
-	old_flags = p->flags;
+retry:
+	flags = old_flags = READ_ONCE(p->flags);
 	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
 
-	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
-			 BR_MULTICAST_FAST_LEAVE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
-	br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
-			 BR_MULTICAST_TO_UNICAST);
-	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
-	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
-	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
-	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
-	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_VLAN_SUPPRESS,
-			 BR_NEIGH_VLAN_SUPPRESS);
-	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_FORWARD_GRAT,
-			 BR_NEIGH_FORWARD_GRAT);
-
-	if ((p->flags & BR_PORT_MAB) &&
-	    (!(p->flags & BR_PORT_LOCKED) || !(p->flags & BR_LEARNING))) {
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_FAST_LEAVE,
+				 BR_MULTICAST_FAST_LEAVE);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_MCAST_FLOOD,
+				 BR_MCAST_FLOOD);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_MCAST_TO_UCAST,
+				 BR_MULTICAST_TO_UNICAST);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_BCAST_FLOOD,
+				 BR_BCAST_FLOOD);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_PROXYARP_WIFI,
+				 BR_PROXYARP_WIFI);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_VLAN_TUNNEL,
+				 BR_VLAN_TUNNEL);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
+				 BR_NEIGH_SUPPRESS);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_NEIGH_VLAN_SUPPRESS,
+				 BR_NEIGH_VLAN_SUPPRESS);
+	flags = br_set_port_flag(flags, tb, IFLA_BRPORT_NEIGH_FORWARD_GRAT,
+				 BR_NEIGH_FORWARD_GRAT);
+
+	if ((flags & BR_PORT_MAB) &&
+	    (!(flags & BR_PORT_LOCKED) || !(flags & BR_LEARNING))) {
 		NL_SET_ERR_MSG(extack, "Bridge port must be locked and have learning enabled when MAB is enabled");
-		p->flags = old_flags;
 		return -EINVAL;
-	} else if (!(p->flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB)) {
+	}
+	if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB)) {
 		struct net_bridge_fdb_flush_desc desc = {
 			.flags = BIT(BR_FDB_LOCKED),
 			.flags_mask = BIT(BR_FDB_LOCKED),
@@ -995,15 +1001,16 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 		br_fdb_flush(p->br, &desc);
 	}
 
-	changed_mask = old_flags ^ p->flags;
+	changed_mask = old_flags ^ flags;
 
-	err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
-	if (err) {
-		p->flags = old_flags;
+	err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
+	if (err)
 		return err;
-	}
 
-	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
+	if (cmpxchg(&p->flags, old_flags, flags) != old_flags)
+		goto retry;
+
+	if (br_vlan_tunnel_old && !(flags & BR_VLAN_TUNNEL))
 		nbp_vlan_tunnel_info_flush(p);
 
 	br_port_flags_change(p, changed_mask);
-- 
2.54.0.1099.g489fc7bff1-goog

[PATCH v2 net-next 3/5] net: bridge: use atomic ops to read/change p->flags (I)

[Eric Dumazet]

Use test_bit() in net/bridge/br_arp_nd_proxy.c,
net/bridge/br_fdb.c and net/bridge/br_forward.c.

Use READ_ONCE(p->flags) in br_recalculate_neigh_suppress_enabled()
as we test two bits at once.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/bridge/br_arp_nd_proxy.c | 22 +++++++---------------
 net/bridge/br_fdb.c          |  2 +-
 net/bridge/br_forward.c      | 15 ++++++++-------
 3 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 5263232278b4fe95775cfcbba71615197e2f5311..23eb6931a2b4ad9137ef20735d8333f376fd49f2 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -29,7 +29,7 @@ void br_recalculate_neigh_suppress_enabled(struct net_bridge *br)
 	bool neigh_suppress = false;
 
 	list_for_each_entry(p, &br->port_list, list) {
-		if (p->flags & (BR_NEIGH_SUPPRESS | BR_NEIGH_VLAN_SUPPRESS)) {
+		if (READ_ONCE(p->flags) & (BR_NEIGH_SUPPRESS | BR_NEIGH_VLAN_SUPPRESS)) {
 			neigh_suppress = true;
 			break;
 		}
@@ -206,8 +206,8 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 			const struct net_bridge_port *dst = READ_ONCE(f->dst);
 			bool replied = false;
 
-			if ((p && (p->flags & BR_PROXYARP)) ||
-			    (dst && (dst->flags & BR_PROXYARP_WIFI)) ||
+			if ((p && test_bit(BR_PROXYARP_BIT, &p->flags)) ||
+			    (dst && test_bit(BR_PROXYARP_WIFI_BIT, &dst->flags)) ||
 			    br_is_neigh_suppress_enabled(dst, vid)) {
 				if (!vid)
 					br_arp_send(br, p, skb->dev, sip, tip,
@@ -511,10 +511,7 @@ bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid)
 	if (!p)
 		return false;
 
-	if (!vid)
-		return !!(p->flags & BR_NEIGH_SUPPRESS);
-
-	if (p->flags & BR_NEIGH_VLAN_SUPPRESS) {
+	if (vid && test_bit(BR_NEIGH_VLAN_SUPPRESS_BIT, &p->flags)) {
 		struct net_bridge_vlan_group *vg = nbp_vlan_group_rcu(p);
 		struct net_bridge_vlan *v;
 
@@ -522,17 +519,13 @@ bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid)
 		if (!v)
 			return false;
 		return !!(v->priv_flags & BR_VLFLAG_NEIGH_SUPPRESS_ENABLED);
-	} else {
-		return !!(p->flags & BR_NEIGH_SUPPRESS);
 	}
+	return test_bit(BR_NEIGH_SUPPRESS_BIT, &p->flags);
 }
 
 bool br_is_neigh_forward_grat_enabled(const struct net_bridge_port *p, u16 vid)
 {
-	if (!vid)
-		return !!(p->flags & BR_NEIGH_FORWARD_GRAT);
-
-	if (p->flags & BR_NEIGH_VLAN_SUPPRESS) {
+	if (vid && test_bit(BR_NEIGH_VLAN_SUPPRESS_BIT, &p->flags)) {
 		struct net_bridge_vlan_group *vg = nbp_vlan_group_rcu(p);
 		struct net_bridge_vlan *v;
 
@@ -540,7 +533,6 @@ bool br_is_neigh_forward_grat_enabled(const struct net_bridge_port *p, u16 vid)
 		if (!v)
 			return false;
 		return !!(v->priv_flags & BR_VLFLAG_NEIGH_FORWARD_GRAT_ENABLED);
-	} else {
-		return !!(p->flags & BR_NEIGH_FORWARD_GRAT);
 	}
+	return test_bit(BR_NEIGH_FORWARD_GRAT_BIT, &p->flags);
 }
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index a114373c98163d1e81b090b9c799adcb1bfeed77..e4570bbed85445bcda08537b81c78bacd163f28d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1514,7 +1514,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 
 	trace_br_fdb_external_learn_add(br, p, addr, vid);
 
-	if (locked && (!p || !(p->flags & BR_PORT_MAB)))
+	if (locked && (!p || !test_bit(BR_PORT_MAB_BIT, &p->flags)))
 		return -EINVAL;
 
 	spin_lock_bh(&br->hash_lock);
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 4a77d07433745152241789818949bd228aa95be0..46c762ca5177e5ffb8bc21077cdb14ca25a4a453 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -24,7 +24,7 @@ static inline int should_deliver(const struct net_bridge_port *p,
 	struct net_bridge_vlan_group *vg;
 
 	vg = nbp_vlan_group_rcu(p);
-	return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
+	return (test_bit(BR_HAIRPIN_MODE_BIT, &p->flags) || skb->dev != p->dev) &&
 		(br_mst_is_enabled(p) || p->state == BR_STATE_FORWARDING) &&
 		br_allowed_egress(vg, skb) && nbp_switchdev_allowed_egress(p, skb) &&
 		!br_skb_isolated(p, skb);
@@ -214,24 +214,24 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 		 */
 		switch (pkt_type) {
 		case BR_PKT_UNICAST:
-			if (!(p->flags & BR_FLOOD))
+			if (!test_bit(BR_FLOOD_BIT, &p->flags))
 				continue;
 			break;
 		case BR_PKT_MULTICAST:
-			if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
+			if (!test_bit(BR_MCAST_FLOOD_BIT, &p->flags) && skb->dev != br->dev)
 				continue;
 			break;
 		case BR_PKT_BROADCAST:
-			if (!(p->flags & BR_BCAST_FLOOD) && skb->dev != br->dev)
+			if (!test_bit(BR_BCAST_FLOOD_BIT, &p->flags) && skb->dev != br->dev)
 				continue;
 			break;
 		}
 
 		/* Do not flood to ports that enable proxy ARP */
-		if (p->flags & BR_PROXYARP)
+		if (test_bit(BR_PROXYARP_BIT, &p->flags))
 			continue;
 		if (BR_INPUT_SKB_CB(skb)->proxyarp_replied) {
-			if (p->flags & BR_PROXYARP_WIFI)
+			if (test_bit(BR_PROXYARP_WIFI_BIT, &p->flags))
 				continue;
 			/* For gratuitous ARPs/NAs, check neigh_forward_grat.
 			 * For regular ARPs/NDs, check only neigh_suppress.
@@ -328,7 +328,8 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 		if ((unsigned long)lport > (unsigned long)rport) {
 			port = lport;
 
-			if (port->flags & BR_MULTICAST_TO_UNICAST) {
+			if (test_bit(BR_MULTICAST_TO_UNICAST_BIT,
+				     &port->flags)) {
 				maybe_deliver_addr(lport, skb, p->eth_addr,
 						   local_orig);
 				goto delivered;
-- 
2.54.0.1099.g489fc7bff1-goog

[PATCH v2 net-next 4/5] net: bridge: use atomic ops to read/change p->flags (II)

[Eric Dumazet]

Use READ_ONCE(p->flags) in br_port_flag_is_set() to keep its ABI.

Use test_bit(), clear_bit(), set_bit() in:

   net/bridge/br_input.c
   net/bridge/br_mrp.c
   net/bridge/br_mrp_netlink.c

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/bridge/br_if.c          |  2 +-
 net/bridge/br_input.c       | 12 ++++++------
 net/bridge/br_mrp.c         | 20 ++++++++++----------
 net/bridge/br_mrp_netlink.c |  8 ++++----
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 463c3c7083dcdd4f9064b12375792b2961a9b674..7ed19aa8ae59a5d2b10babf91985486ba8889854 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -761,6 +761,6 @@ bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag)
 	if (!p)
 		return false;
 
-	return p->flags & flag;
+	return READ_ONCE(p->flags) & flag;
 }
 EXPORT_SYMBOL_GPL(br_port_flag_is_set);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 470615675bdc03ca181b72159a1bcfc7b9d7a6a7..ddb8f002a40e8257db2029a1f7b549c87db52ffd 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -111,7 +111,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				&state, &vlan))
 		goto out;
 
-	if (p->flags & BR_PORT_LOCKED) {
+	if (test_bit(BR_PORT_LOCKED_BIT, &p->flags)) {
 		struct net_bridge_fdb_entry *fdb_src =
 			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
 
@@ -119,7 +119,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			/* FDB miss. Create locked FDB entry if MAB is enabled
 			 * and drop the packet.
 			 */
-			if (p->flags & BR_PORT_MAB)
+			if (test_bit(BR_PORT_MAB_BIT, &p->flags))
 				br_fdb_update(br, p, eth_hdr(skb)->h_source,
 					      vid, BIT(BR_FDB_LOCKED));
 			goto drop;
@@ -140,7 +140,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	nbp_switchdev_frame_mark(p, skb);
 
 	/* insert into forwarding database after filtering to avoid spoofing */
-	if (p->flags & BR_LEARNING)
+	if (test_bit(BR_LEARNING_BIT, &p->flags))
 		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
 
 	promisc = !!(br->dev->flags & IFF_PROMISC);
@@ -164,7 +164,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	}
 
 	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
-	BR_INPUT_SKB_CB(skb)->src_port_isolated = !!(p->flags & BR_ISOLATED);
+	BR_INPUT_SKB_CB(skb)->src_port_isolated = test_bit(BR_ISOLATED_BIT, &p->flags);
 
 	if (IS_ENABLED(CONFIG_INET) &&
 	    (skb->protocol == htons(ETH_P_ARP) ||
@@ -248,7 +248,7 @@ static void __br_handle_local_finish(struct sk_buff *skb)
 	u16 vid = 0;
 
 	/* check if vlan is allowed, to avoid spoofing */
-	if ((p->flags & BR_LEARNING) &&
+	if (test_bit(BR_LEARNING_BIT, &p->flags) &&
 	    nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
 	    br_should_learn(p, skb, &vid))
@@ -359,7 +359,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 	br_tc_skb_miss_set(skb, false);
 
 	p = br_port_get_rcu(skb->dev);
-	if (p->flags & BR_VLAN_TUNNEL)
+	if (test_bit(BR_VLAN_TUNNEL_BIT, &p->flags))
 		br_handle_ingress_vlan_tunnel(skb, p, nbp_vlan_group_rcu(p));
 
 	if (unlikely(is_link_local_ether_addr(dest))) {
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index f1aa67f7a0510d9ff8ee7d171eecc048a5554d2f..3f7126a7d72003c3e0a6366506bb0b55da49f0a4 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -454,7 +454,7 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
 		state = netif_running(br->dev) ?
 				BR_STATE_FORWARDING : BR_STATE_DISABLED;
 		p->state = state;
-		p->flags &= ~BR_MRP_AWARE;
+		clear_bit(BR_MRP_AWARE_BIT, &p->flags);
 		spin_unlock_bh(&br->lock);
 		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->p_port, NULL);
@@ -466,7 +466,7 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
 		state = netif_running(br->dev) ?
 				BR_STATE_FORWARDING : BR_STATE_DISABLED;
 		p->state = state;
-		p->flags &= ~BR_MRP_AWARE;
+		clear_bit(BR_MRP_AWARE_BIT, &p->flags);
 		spin_unlock_bh(&br->lock);
 		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->s_port, NULL);
@@ -478,7 +478,7 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp)
 		state = netif_running(br->dev) ?
 				BR_STATE_FORWARDING : BR_STATE_DISABLED;
 		p->state = state;
-		p->flags &= ~BR_MRP_AWARE;
+		clear_bit(BR_MRP_AWARE_BIT, &p->flags);
 		spin_unlock_bh(&br->lock);
 		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->i_port, NULL);
@@ -526,14 +526,14 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
 	p = br_mrp_get_port(br, instance->p_ifindex);
 	spin_lock_bh(&br->lock);
 	p->state = BR_STATE_FORWARDING;
-	p->flags |= BR_MRP_AWARE;
+	set_bit(BR_MRP_AWARE_BIT, &p->flags);
 	spin_unlock_bh(&br->lock);
 	rcu_assign_pointer(mrp->p_port, p);
 
 	p = br_mrp_get_port(br, instance->s_ifindex);
 	spin_lock_bh(&br->lock);
 	p->state = BR_STATE_FORWARDING;
-	p->flags |= BR_MRP_AWARE;
+	set_bit(BR_MRP_AWARE_BIT, &p->flags);
 	spin_unlock_bh(&br->lock);
 	rcu_assign_pointer(mrp->s_port, p);
 
@@ -593,7 +593,7 @@ int br_mrp_set_port_state(struct net_bridge_port *p,
 {
 	u32 port_state;
 
-	if (!p || !(p->flags & BR_MRP_AWARE))
+	if (!p || !test_bit(BR_MRP_AWARE_BIT, &p->flags))
 		return -EINVAL;
 
 	spin_lock_bh(&p->br->lock);
@@ -619,7 +619,7 @@ int br_mrp_set_port_role(struct net_bridge_port *p,
 {
 	struct br_mrp *mrp;
 
-	if (!p || !(p->flags & BR_MRP_AWARE))
+	if (!p || !test_bit(BR_MRP_AWARE_BIT, &p->flags))
 		return -EINVAL;
 
 	mrp = br_mrp_find_port(p->br, p);
@@ -784,7 +784,7 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 		state = netif_running(br->dev) ?
 				BR_STATE_FORWARDING : BR_STATE_DISABLED;
 		p->state = state;
-		p->flags &= ~BR_MRP_AWARE;
+		clear_bit(BR_MRP_AWARE_BIT, &p->flags);
 		spin_unlock_bh(&br->lock);
 		br_mrp_port_switchdev_set_state(p, state);
 		rcu_assign_pointer(mrp->i_port, NULL);
@@ -809,7 +809,7 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 	p = br_mrp_get_port(br, role->i_ifindex);
 	spin_lock_bh(&br->lock);
 	p->state = BR_STATE_FORWARDING;
-	p->flags |= BR_MRP_AWARE;
+	set_bit(BR_MRP_AWARE_BIT, &p->flags);
 	spin_unlock_bh(&br->lock);
 	rcu_assign_pointer(mrp->i_port, p);
 
@@ -1246,7 +1246,7 @@ static int br_mrp_rcv(struct net_bridge_port *p,
 static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
 {
 	/* If there is no MRP instance do normal forwarding */
-	if (likely(!(p->flags & BR_MRP_AWARE)))
+	if (likely(!test_bit(BR_MRP_AWARE_BIT, &p->flags)))
 		goto out;
 
 	return br_mrp_rcv(p, skb, p->dev);
diff --git a/net/bridge/br_mrp_netlink.c b/net/bridge/br_mrp_netlink.c
index 86f0e75d6e345f8f388ae163860a685279174222..39f07e13fba54ad38198a5f29a4daae2db27d989 100644
--- a/net/bridge/br_mrp_netlink.c
+++ b/net/bridge/br_mrp_netlink.c
@@ -538,9 +538,9 @@ int br_mrp_ring_port_open(struct net_device *dev, u8 loc)
 	}
 
 	if (loc)
-		p->flags |= BR_MRP_LOST_CONT;
+		set_bit(BR_MRP_LOST_CONT_BIT, &p->flags);
 	else
-		p->flags &= ~BR_MRP_LOST_CONT;
+		clear_bit(BR_MRP_LOST_CONT_BIT, &p->flags);
 
 	br_ifinfo_notify(RTM_NEWLINK, NULL, p);
 
@@ -560,9 +560,9 @@ int br_mrp_in_port_open(struct net_device *dev, u8 loc)
 	}
 
 	if (loc)
-		p->flags |= BR_MRP_LOST_IN_CONT;
+		set_bit(BR_MRP_LOST_IN_CONT_BIT, &p->flags);
 	else
-		p->flags &= ~BR_MRP_LOST_IN_CONT;
+		clear_bit(BR_MRP_LOST_IN_CONT_BIT, &p->flags);
 
 	br_ifinfo_notify(RTM_NEWLINK, NULL, p);
 
-- 
2.54.0.1099.g489fc7bff1-goog

[PATCH v2 net-next 5/5] net: bridge: use atomic ops to read/change p->flags (III)

[Eric Dumazet]

Use test_bit(), clear_bit(), set_bit() in:

   net/bridge/br_multicast.c
   net/bridge/br_netlink.c
   net/bridge/br_stp.c
   net/bridge/br_stp_bpdu.c
   net/bridge/br_switchdev.c
   net/bridge/br_vlan_options.c

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/bridge/br_multicast.c    | 5 +++--
 net/bridge/br_netlink.c      | 2 +-
 net/bridge/br_stp.c          | 4 ++--
 net/bridge/br_stp_bpdu.c     | 2 +-
 net/bridge/br_switchdev.c    | 8 ++++----
 net/bridge/br_vlan_options.c | 2 +-
 6 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index c2c93c7404151dcf02e59eb8b868ec3e39dfd5bd..6b3ac473fd228b65d1479963e032a63a9f9210ee 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -281,7 +281,7 @@ static bool br_port_group_equal(struct net_bridge_port_group *p,
 	if (p->key.port != port)
 		return false;
 
-	if (!(port->flags & BR_MULTICAST_TO_UNICAST))
+	if (!test_bit(BR_MULTICAST_TO_UNICAST_BIT, &port->flags))
 		return true;
 
 	return ether_addr_equal(src, p->eth_addr);
@@ -3672,7 +3672,8 @@ br_multicast_leave_group(struct net_bridge_mcast *brmctx,
 	if (!mp)
 		goto out;
 
-	if (pmctx && (pmctx->port->flags & BR_MULTICAST_FAST_LEAVE)) {
+	if (pmctx &&
+	    test_bit(BR_MULTICAST_FAST_LEAVE_BIT, &pmctx->port->flags)) {
 		struct net_bridge_port_group __rcu **pp;
 
 		for (pp = &mp->ports;
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 12850f60bfcb418918508612070a6b0266ffbe6b..928e564e6a3ead46afeff39d772f5b9299cb4b30 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -540,7 +540,7 @@ static int br_fill_ifinfo(struct sk_buff *skb,
 		else
 			err = br_fill_ifvlaninfo(skb, vg);
 
-		if (port && (port->flags & BR_VLAN_TUNNEL))
+		if (port && test_bit(BR_VLAN_TUNNEL_BIT, &port->flags))
 			err = br_fill_vlan_tunnel_info(skb, vg);
 		rcu_read_unlock();
 		if (err)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index b33eb085d9b6cb4cd4f3aff2aa4793d027bbc0f8..46919d73d42f98ad92ad0c4ddd2396a0cdd90c02 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -39,7 +39,7 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 	/* Don't change the state of the ports if they are driven by a different
 	 * protocol.
 	 */
-	if (p->flags & BR_MRP_AWARE)
+	if (test_bit(BR_MRP_AWARE_BIT, &p->flags))
 		return;
 
 	p->state = state;
@@ -179,7 +179,7 @@ static void br_root_selection(struct net_bridge *br)
 		if (!br_should_become_root_port(p, root_port))
 			continue;
 
-		if (p->flags & BR_ROOT_BLOCK)
+		if (test_bit(BR_ROOT_BLOCK_BIT, &p->flags))
 			br_root_port_block(br, p);
 		else
 			root_port = p->port_no;
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7895489ac6fe7ca47cf001c0fea267d5dcbd4371..74ec42ba1e7d086cbfde608c93f5fa24d6b4ed7b 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -178,7 +178,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 	if (!ether_addr_equal(eth_hdr(skb)->h_dest, br->group_addr))
 		goto out;
 
-	if (p->flags & BR_BPDU_GUARD) {
+	if (test_bit(BR_BPDU_GUARD_BIT, &p->flags)) {
 		br_notice(br, "BPDU received on blocked port %u(%s)\n",
 			  (unsigned int) p->port_no, p->dev->name);
 		br_stp_disable_port(p);
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index ee3ad9dfbab99597ab06d5e6e008519d4e17fd6a..990c6b38fd397ad364297b130b0f3736ac7b13a7 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -20,7 +20,7 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct net_bridge_port *p,
 	if (br_multicast_igmp_type(skb))
 		return false;
 
-	return (p->flags & BR_TX_FWD_OFFLOAD) &&
+	return test_bit(BR_TX_FWD_OFFLOAD_BIT, &p->flags) &&
 	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
 }
 
@@ -287,7 +287,7 @@ static int nbp_switchdev_add(struct net_bridge_port *p,
 		return err;
 
 	if (tx_fwd_offload) {
-		p->flags |= BR_TX_FWD_OFFLOAD;
+		set_bit(BR_TX_FWD_OFFLOAD_BIT, &p->flags);
 		static_branch_inc(&br_switchdev_tx_fwd_offload);
 	}
 
@@ -307,8 +307,8 @@ static void nbp_switchdev_del(struct net_bridge_port *p)
 	if (p->hwdom)
 		nbp_switchdev_hwdom_put(p);
 
-	if (p->flags & BR_TX_FWD_OFFLOAD) {
-		p->flags &= ~BR_TX_FWD_OFFLOAD;
+	if (test_bit(BR_TX_FWD_OFFLOAD_BIT, &p->flags)) {
+		clear_bit(BR_TX_FWD_OFFLOAD_BIT, &p->flags);
 		static_branch_dec(&br_switchdev_tx_fwd_offload);
 	}
 }
diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
index 92af1e558fda886301f167463ddf43f14afd2702..fcc200c3e3da260465f9eb8ab1c8c87e2d3f7588 100644
--- a/net/bridge/br_vlan_options.c
+++ b/net/bridge/br_vlan_options.c
@@ -172,7 +172,7 @@ static int br_vlan_modify_tunnel(const struct net_bridge_port *p,
 		NL_SET_ERR_MSG_MOD(extack, "Can't modify tunnel mapping of non-port vlans");
 		return -EINVAL;
 	}
-	if (!(p->flags & BR_VLAN_TUNNEL)) {
+	if (!test_bit(BR_VLAN_TUNNEL_BIT, &p->flags)) {
 		NL_SET_ERR_MSG_MOD(extack, "Port doesn't have tunnel flag set");
 		return -EINVAL;
 	}
-- 
2.54.0.1099.g489fc7bff1-goog

Re: [PATCH v2 net-next 1/5] bridge: use atomic ops to read/change p->flags in sysfs

[Nikolay Aleksandrov]

On 10/06/2026 19:18, Eric Dumazet wrote:
> Change net/bridge/br_sysfs_if.c to use atomic operations
> to read/change bits n p->flags.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   net/bridge/br_sysfs_if.c | 61 ++++++++++++++++++++++------------------
>   1 file changed, 33 insertions(+), 28 deletions(-)
> 

One minor nit below if there's another version, otherwise:
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 1923c004f0d2b746902f5b6de1bbb72f7f824125..738544901563f570355554f49b2b80cf962f8e71 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -44,40 +44,45 @@ const struct brport_attribute brport_attr_##_name = { 	        \
>   	.store	= _store,					\
>   };
>   
> -#define BRPORT_ATTR_FLAG(_name, _mask)				\
> +#define BRPORT_ATTR_FLAG(_name, _bitnr)				\
>   static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
>   {								\
> -	return sysfs_emit(buf, "%d\n", !!(p->flags & _mask));	\
> +	return sysfs_emit(buf, "%d\n", test_bit(_bitnr, &p->flags));	\
>   }								\
>   static int store_##_name(struct net_bridge_port *p, unsigned long v) \
>   {								\
> -	return store_flag(p, v, _mask);				\
> +	return store_flag(p, v, _bitnr);				\
>   }								\
>   static BRPORT_ATTR(_name, 0644,					\
>   		   show_##_name, store_##_name)
>   
>   static int store_flag(struct net_bridge_port *p, unsigned long v,
> -		      unsigned long mask)
> +		      unsigned long bitnr)
>   {
> +	unsigned long oflags, flags = READ_ONCE(p->flags);
>   	struct netlink_ext_ack extack = {0};
> -	unsigned long flags = p->flags;
>   	int err;
>   
> +

this extra new line seems unnecessary, there's already one

> +	oflags = flags;
>   	if (v)
> -		flags |= mask;
> +		__set_bit(bitnr, &flags);
>   	else
> -		flags &= ~mask;
> +		__clear_bit(bitnr, &flags);
>   
> -	if (flags != p->flags) {
> -		err = br_switchdev_set_port_flag(p, flags, mask, &extack);
> -		if (err) {
> -			netdev_err(p->dev, "%s\n", extack._msg);
> -			return err;
> -		}
> +	if (flags == oflags)
> +		return 0;
>   
> -		p->flags = flags;
> -		br_port_flags_change(p, mask);
> +	err = br_switchdev_set_port_flag(p, flags, BIT(bitnr), &extack);
> +	if (err) {
> +		netdev_err(p->dev, "%s\n", extack._msg);
> +		return err;
>   	}
> +	if (v)
> +		set_bit(bitnr, &p->flags);
> +	else
> +		clear_bit(bitnr, &p->flags);
> +	br_port_flags_change(p, BIT(bitnr));
>   	return 0;
>   }
>   
> @@ -247,17 +252,17 @@ static int store_backup_port(struct net_bridge_port *p, char *buf)
>   }
>   static BRPORT_ATTR_RAW(backup_port, 0644, show_backup_port, store_backup_port);
>   
> -BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE);
> -BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD);
> -BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK);
> -BRPORT_ATTR_FLAG(learning, BR_LEARNING);
> -BRPORT_ATTR_FLAG(unicast_flood, BR_FLOOD);
> -BRPORT_ATTR_FLAG(proxyarp, BR_PROXYARP);
> -BRPORT_ATTR_FLAG(proxyarp_wifi, BR_PROXYARP_WIFI);
> -BRPORT_ATTR_FLAG(multicast_flood, BR_MCAST_FLOOD);
> -BRPORT_ATTR_FLAG(broadcast_flood, BR_BCAST_FLOOD);
> -BRPORT_ATTR_FLAG(neigh_suppress, BR_NEIGH_SUPPRESS);
> -BRPORT_ATTR_FLAG(isolated, BR_ISOLATED);
> +BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE_BIT);
> +BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD_BIT);
> +BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK_BIT);
> +BRPORT_ATTR_FLAG(learning, BR_LEARNING_BIT);
> +BRPORT_ATTR_FLAG(unicast_flood, BR_FLOOD_BIT);
> +BRPORT_ATTR_FLAG(proxyarp, BR_PROXYARP_BIT);
> +BRPORT_ATTR_FLAG(proxyarp_wifi, BR_PROXYARP_WIFI_BIT);
> +BRPORT_ATTR_FLAG(multicast_flood, BR_MCAST_FLOOD_BIT);
> +BRPORT_ATTR_FLAG(broadcast_flood, BR_BCAST_FLOOD_BIT);
> +BRPORT_ATTR_FLAG(neigh_suppress, BR_NEIGH_SUPPRESS_BIT);
> +BRPORT_ATTR_FLAG(isolated, BR_ISOLATED_BIT);
>   
>   #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>   static ssize_t show_multicast_router(struct net_bridge_port *p, char *buf)
> @@ -273,8 +278,8 @@ static int store_multicast_router(struct net_bridge_port *p,
>   static BRPORT_ATTR(multicast_router, 0644, show_multicast_router,
>   		   store_multicast_router);
>   
> -BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE);
> -BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UNICAST);
> +BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE_BIT);
> +BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UNICAST_BIT);
>   #endif
>   
>   static const struct brport_attribute *brport_attrs[] = {

Re: [PATCH v2 net-next 2/5] bridge: use atomic ops to read/change p->flags in br_netlink.c

[Nikolay Aleksandrov]

On 10/06/2026 19:18, Eric Dumazet wrote:
> Change net/bridge/br_netlink.c to use atomic operations
> to read/change bits in p->flags.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   net/bridge/br_netlink.c | 97 ++++++++++++++++++++++-------------------
>   1 file changed, 52 insertions(+), 45 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net-next 3/5] net: bridge: use atomic ops to read/change p->flags (I)

[Nikolay Aleksandrov]

On 10/06/2026 19:18, Eric Dumazet wrote:
> Use test_bit() in net/bridge/br_arp_nd_proxy.c,
> net/bridge/br_fdb.c and net/bridge/br_forward.c.
> 
> Use READ_ONCE(p->flags) in br_recalculate_neigh_suppress_enabled()
> as we test two bits at once.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   net/bridge/br_arp_nd_proxy.c | 22 +++++++---------------
>   net/bridge/br_fdb.c          |  2 +-
>   net/bridge/br_forward.c      | 15 ++++++++-------
>   3 files changed, 16 insertions(+), 23 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net-next 4/5] net: bridge: use atomic ops to read/change p->flags (II)

[Nikolay Aleksandrov]

On 10/06/2026 19:18, Eric Dumazet wrote:
> Use READ_ONCE(p->flags) in br_port_flag_is_set() to keep its ABI.
> 
> Use test_bit(), clear_bit(), set_bit() in:
> 
>     net/bridge/br_input.c
>     net/bridge/br_mrp.c
>     net/bridge/br_mrp_netlink.c
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   net/bridge/br_if.c          |  2 +-
>   net/bridge/br_input.c       | 12 ++++++------
>   net/bridge/br_mrp.c         | 20 ++++++++++----------
>   net/bridge/br_mrp_netlink.c |  8 ++++----
>   4 files changed, 21 insertions(+), 21 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net-next 5/5] net: bridge: use atomic ops to read/change p->flags (III)

[Nikolay Aleksandrov]

On 10/06/2026 19:18, Eric Dumazet wrote:
> Use test_bit(), clear_bit(), set_bit() in:
> 
>     net/bridge/br_multicast.c
>     net/bridge/br_netlink.c
>     net/bridge/br_stp.c
>     net/bridge/br_stp_bpdu.c
>     net/bridge/br_switchdev.c
>     net/bridge/br_vlan_options.c
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   net/bridge/br_multicast.c    | 5 +++--
>   net/bridge/br_netlink.c      | 2 +-
>   net/bridge/br_stp.c          | 4 ++--
>   net/bridge/br_stp_bpdu.c     | 2 +-
>   net/bridge/br_switchdev.c    | 8 ++++----
>   net/bridge/br_vlan_options.c | 2 +-
>   6 files changed, 12 insertions(+), 11 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net-next 1/5] bridge: use atomic ops to read/change p->flags in sysfs

[Jakub Kicinski]

On Thu, 11 Jun 2026 07:46:46 +0300 Nikolay Aleksandrov wrote:
> > +		      unsigned long bitnr)
> >   {
> > +	unsigned long oflags, flags = READ_ONCE(p->flags);
> >   	struct netlink_ext_ack extack = {0};
> > -	unsigned long flags = p->flags;
> >   	int err;
> >   
> > +  
> 
> this extra new line seems unnecessary, there's already one

Can fix when applying

Re: [PATCH v2 net-next 2/5] bridge: use atomic ops to read/change p->flags in br_netlink.c

[Jakub Kicinski]

Ido, Nik, do we care about this?

---
bridge: use atomic ops to read/change p->flags in br_netlink.c

The change converts net/bridge/br_netlink.c reads of p->flags to
test_bit()/READ_ONCE() and rewrites br_setport() to compute a new
flags value in a local variable, then commit it with a cmpxchg()
retry loop.

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 7cb24de9c77d..12850f60bfcb 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c

[ ... ]

> @@ -934,58 +934,64 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
>  }
>  
>  /* Set/clear or port flags based on attribute */

[ ... ]

>  /* Process bridge protocol info on port */
>  static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
>  		      struct netlink_ext_ack *extack)
>  {
> -	unsigned long old_flags, changed_mask;
> +	unsigned long old_flags, flags, changed_mask;
>  	bool br_vlan_tunnel_old;
>  	int err;
>  
> -	old_flags = p->flags;
> +retry:
> +	flags = old_flags = READ_ONCE(p->flags);
>  	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;

[ ... ]

> +	if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB)) {
>  		struct net_bridge_fdb_flush_desc desc = {
>  			.flags = BIT(BR_FDB_LOCKED),
>  			.flags_mask = BIT(BR_FDB_LOCKED),
> @@ -995,15 +1001,16 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
>  		br_fdb_flush(p->br, &desc);
>  	}
>  
> -	changed_mask = old_flags ^ p->flags;
> +	changed_mask = old_flags ^ flags;
>  
> -	err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
> -	if (err) {
> -		p->flags = old_flags;
> +	err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
> +	if (err)
>  		return err;
> -	}
>  
> -	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
> +	if (cmpxchg(&p->flags, old_flags, flags) != old_flags)
> +		goto retry;
> +

[High]

Are the side effects in this loop safe to re-execute on a cmpxchg retry?

The retry path now wraps br_fdb_flush() and br_switchdev_set_port_flag()
inside the loop body:

	retry:
		flags = old_flags = READ_ONCE(p->flags);
		...
		if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB))
			br_fdb_flush(p->br, &desc);
		changed_mask = old_flags ^ flags;
		err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
		if (err)
			return err;
		if (cmpxchg(&p->flags, old_flags, flags) != old_flags)
			goto retry;
		...
		br_port_flags_change(p, changed_mask);

Can this retry actually be taken?  br_setport() runs under RTNL, but
several other writers modify p->flags without RTNL via set_bit() and
clear_bit(), for example BR_MRP_LOST_CONT_BIT and BR_MRP_LOST_IN_CONT_BIT
toggled from work-queue and softirq context in net/bridge/br_mrp_netlink.c
(br_mrp_ring_port_open() and br_mrp_in_port_open(), called from
br_mrp_test_work_expired() and br_mrp_mrm_process()).

Because cmpxchg compares the entire word, a concurrent set_bit() or
clear_bit() on any unrelated bit (such as the MRP bits above) makes the
cmpxchg fail and trigger goto retry.

When that happens, what is the effect on switchdev?

	err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);

br_switchdev_set_port_flag() first calls call_switchdev_notifiers() with
SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, then queues a SWITCHDEV_F_DEFER
attribute set for SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:

	attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
	attr.flags = SWITCHDEV_F_DEFER;
	err = switchdev_port_attr_set(p->dev, &attr, extack);

The deferred update from a first iteration is already queued and cannot
be cancelled when the retry loop queues a second one with a different
(val, mask) pair.  Drivers will see notifier callbacks for both
iterations.

For br_fdb_flush(), the BR_PORT_MAB transition

	if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB))
		br_fdb_flush(p->br, &desc);

can re-evaluate to true on a retry and flush FDB entries again.

Finally, br_port_flags_change() runs only once after the loop, with the
final iteration's changed_mask:

	br_port_flags_change(p, changed_mask);

If a bit such as BR_AUTO_MASK or BR_NEIGH_SUPPRESS was set in an
earlier iteration's changed_mask but not in the last one (because a
concurrent writer altered an unrelated bit between iterations), would
the switchdev driver have already been told about that transition while
the bridge software-side recompute in br_port_flags_change() is skipped?

The original code performed each of these side effects exactly once per
br_setport() call.  Should the side effects be moved outside the cmpxchg
loop so only the pure flags computation is retried?

>  	if (br_vlan_tunnel_old && !(flags & BR_VLAN_TUNNEL))
>  		nbp_vlan_tunnel_info_flush(p);
>  
>  	br_port_flags_change(p, changed_mask);
-- 
pw-bot: cr

Re: [PATCH v2 net-next 2/5] bridge: use atomic ops to read/change p->flags in br_netlink.c

[Eric Dumazet]

On Thu, Jun 11, 2026 at 11:31 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Ido, Nik, do we care about this?
>
> ---
> bridge: use atomic ops to read/change p->flags in br_netlink.c
>
> The change converts net/bridge/br_netlink.c reads of p->flags to
> test_bit()/READ_ONCE() and rewrites br_setport() to compute a new
> flags value in a local variable, then commit it with a cmpxchg()
> retry loop.
>
> > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> > index 7cb24de9c77d..12850f60bfcb 100644
> > --- a/net/bridge/br_netlink.c
> > +++ b/net/bridge/br_netlink.c
>
> [ ... ]
>
> > @@ -934,58 +934,64 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
> >  }
> >
> >  /* Set/clear or port flags based on attribute */
>
> [ ... ]
>
> >  /* Process bridge protocol info on port */
> >  static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> >                     struct netlink_ext_ack *extack)
> >  {
> > -     unsigned long old_flags, changed_mask;
> > +     unsigned long old_flags, flags, changed_mask;
> >       bool br_vlan_tunnel_old;
> >       int err;
> >
> > -     old_flags = p->flags;
> > +retry:
> > +     flags = old_flags = READ_ONCE(p->flags);
> >       br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
>
> [ ... ]
>
> > +     if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB)) {
> >               struct net_bridge_fdb_flush_desc desc = {
> >                       .flags = BIT(BR_FDB_LOCKED),
> >                       .flags_mask = BIT(BR_FDB_LOCKED),
> > @@ -995,15 +1001,16 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> >               br_fdb_flush(p->br, &desc);
> >       }
> >
> > -     changed_mask = old_flags ^ p->flags;
> > +     changed_mask = old_flags ^ flags;
> >
> > -     err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
> > -     if (err) {
> > -             p->flags = old_flags;
> > +     err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
> > +     if (err)
> >               return err;
> > -     }
> >
> > -     if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
> > +     if (cmpxchg(&p->flags, old_flags, flags) != old_flags)
> > +             goto retry;
> > +
>
> [High]
>
> Are the side effects in this loop safe to re-execute on a cmpxchg retry?
>
> The retry path now wraps br_fdb_flush() and br_switchdev_set_port_flag()
> inside the loop body:
>
>         retry:
>                 flags = old_flags = READ_ONCE(p->flags);
>                 ...
>                 if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB))
>                         br_fdb_flush(p->br, &desc);
>                 changed_mask = old_flags ^ flags;
>                 err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
>                 if (err)
>                         return err;
>                 if (cmpxchg(&p->flags, old_flags, flags) != old_flags)
>                         goto retry;
>                 ...
>                 br_port_flags_change(p, changed_mask);
>
> Can this retry actually be taken?  br_setport() runs under RTNL, but
> several other writers modify p->flags without RTNL via set_bit() and
> clear_bit(), for example BR_MRP_LOST_CONT_BIT and BR_MRP_LOST_IN_CONT_BIT
> toggled from work-queue and softirq context in net/bridge/br_mrp_netlink.c
> (br_mrp_ring_port_open() and br_mrp_in_port_open(), called from
> br_mrp_test_work_expired() and br_mrp_mrm_process()).
>
> Because cmpxchg compares the entire word, a concurrent set_bit() or
> clear_bit() on any unrelated bit (such as the MRP bits above) makes the
> cmpxchg fail and trigger goto retry.
>
> When that happens, what is the effect on switchdev?
>
>         err = br_switchdev_set_port_flag(p, flags, changed_mask, extack);
>
> br_switchdev_set_port_flag() first calls call_switchdev_notifiers() with
> SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, then queues a SWITCHDEV_F_DEFER
> attribute set for SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
>
>         attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
>         attr.flags = SWITCHDEV_F_DEFER;
>         err = switchdev_port_attr_set(p->dev, &attr, extack);
>
> The deferred update from a first iteration is already queued and cannot
> be cancelled when the retry loop queues a second one with a different
> (val, mask) pair.  Drivers will see notifier callbacks for both
> iterations.
>
> For br_fdb_flush(), the BR_PORT_MAB transition
>
>         if (!(flags & BR_PORT_MAB) && (old_flags & BR_PORT_MAB))
>                 br_fdb_flush(p->br, &desc);
>
> can re-evaluate to true on a retry and flush FDB entries again.
>
> Finally, br_port_flags_change() runs only once after the loop, with the
> final iteration's changed_mask:
>
>         br_port_flags_change(p, changed_mask);
>
> If a bit such as BR_AUTO_MASK or BR_NEIGH_SUPPRESS was set in an
> earlier iteration's changed_mask but not in the last one (because a
> concurrent writer altered an unrelated bit between iterations), would
> the switchdev driver have already been told about that transition while
> the bridge software-side recompute in br_port_flags_change() is skipped?
>
> The original code performed each of these side effects exactly once per
> br_setport() call.  Should the side effects be moved outside the cmpxchg
> loop so only the pure flags computation is retried?
>
> >       if (br_vlan_tunnel_old && !(flags & BR_VLAN_TUNNEL))
> >               nbp_vlan_tunnel_info_flush(p);
> >
> >       br_port_flags_change(p, changed_mask);
> --
> pw-bot: cr

OK, I will perform two atomic operations, one OR and one AND.