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);
