[PATCH net-next] bridge: Add missing READ_ONCE() annotations around FDB destination port

[PATCH net-next] bridge: Add missing READ_ONCE() annotations around FDB destination port

[Ido Schimmel]

When roaming, the FDB destination port can change without holding the
bridge's hash lock. Therefore, add missing READ_ONCE() annotations in
both RCU readers and readers that hold the lock. In the latter case, the
annotation is not needed in places where the FDB entry was already
validated to be a local entry since such entries cannot roam.

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/bridge/br_device.c | 2 +-
 net/bridge/br_fdb.c    | 7 ++++---
 net/bridge/br_input.c  | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index a35ceae0a6f2..e7f343ab22d3 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -107,7 +107,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		else
 			br_flood(br, skb, BR_PKT_MULTICAST, false, true, vid);
 	} else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
-		br_forward(dst->dst, skb, false, true);
+		br_forward(READ_ONCE(dst->dst), skb, false, true);
 	} else {
 		br_flood(br, skb, BR_PKT_UNICAST, false, true, vid);
 	}
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index ac81e58d5f70..a114373c9816 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -470,7 +470,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 	spin_lock_bh(&br->hash_lock);
 	vg = nbp_vlan_group(p);
 	hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
-		if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
+		if (READ_ONCE(f->dst) == p &&
+		    test_bit(BR_FDB_LOCAL, &f->flags) &&
 		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) {
 			/* delete old one */
 			fdb_delete_local(br, p, f);
@@ -878,7 +879,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 
 	spin_lock_bh(&br->hash_lock);
 	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (f->dst != p)
+		if (READ_ONCE(f->dst) != p)
 			continue;
 
 		if (!do_all)
@@ -1631,7 +1632,7 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
 
 	spin_lock_bh(&p->br->hash_lock);
 	hlist_for_each_entry(f, &p->br->fdb_list, fdb_node) {
-		if (f->dst == p && f->key.vlan_id == vid)
+		if (READ_ONCE(f->dst) == p && f->key.vlan_id == vid)
 			clear_bit(BR_FDB_OFFLOADED, &f->flags);
 	}
 	spin_unlock_bh(&p->br->hash_lock);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 2cbae0f9ae1f..470615675bdc 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -223,7 +223,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 		if (now != READ_ONCE(dst->used))
 			WRITE_ONCE(dst->used, now);
-		br_forward(dst->dst, skb, local_rcv, false);
+		br_forward(READ_ONCE(dst->dst), skb, local_rcv, false);
 	} else {
 		if (!mcast_hit)
 			br_flood(br, skb, pkt_type, local_rcv, false, vid);
-- 
2.54.0

Re: [PATCH net-next] bridge: Add missing READ_ONCE() annotations around FDB destination port

[Ido Schimmel]

On Sun, May 17, 2026 at 02:50:09PM +0300, Ido Schimmel wrote:
> When roaming, the FDB destination port can change without holding the
> bridge's hash lock. Therefore, add missing READ_ONCE() annotations in
> both RCU readers and readers that hold the lock. In the latter case, the
> annotation is not needed in places where the FDB entry was already
> validated to be a local entry since such entries cannot roam.
> 
> Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/bridge/br_device.c | 2 +-
>  net/bridge/br_fdb.c    | 7 ++++---
>  net/bridge/br_input.c  | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index a35ceae0a6f2..e7f343ab22d3 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -107,7 +107,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		else
>  			br_flood(br, skb, BR_PKT_MULTICAST, false, true, vid);
>  	} else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
> -		br_forward(dst->dst, skb, false, true);
> +		br_forward(READ_ONCE(dst->dst), skb, false, true);

Sashiko complains about a race condition between br_fdb_update() and
del_nbp(), but it's a pre-existing issue. I will add it to my list.

>  	} else {
>  		br_flood(br, skb, BR_PKT_UNICAST, false, true, vid);
>  	}
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index ac81e58d5f70..a114373c9816 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -470,7 +470,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  	spin_lock_bh(&br->hash_lock);
>  	vg = nbp_vlan_group(p);
>  	hlist_for_each_entry(f, &br->fdb_list, fdb_node) {

Sashiko complains that this needs to be hlist_for_each_entry_safe() for
PREEMPT_RT kernels, but I am not sure it's correct and it's not a
problem that was introduced by this patch:

"
Does this loop need to use hlist_for_each_entry_safe()?

If fdb_delete_local() unlinks f and the loop continues, the loop will
dereference f->fdb_node.next. Because spin_lock_bh() is a sleepable lock on
PREEMPT_RT and does not disable preemption, an RCU grace period could complete
concurrently.

Could this result in f being freed before the loop attempts to read its next
pointer, leading to a use-after-free?
"

AFAICT, on PREEMPT_RT spin_lock_bh() calls local_bh_disable() which
calls rcu_read_lock(), so I don't see how an RCU grace period can pass.

> -		if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
> +		if (READ_ONCE(f->dst) == p &&
> +		    test_bit(BR_FDB_LOCAL, &f->flags) &&
>  		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) {
>  			/* delete old one */
>  			fdb_delete_local(br, p, f);