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

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 <[email protected]>
> Signed-off-by: Ido Schimmel <[email protected]>
> ---
>  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);