[PATCH net] net: bridge: fdb: hold hash_lock when an entry roams

[PATCH net] net: bridge: fdb: hold hash_lock when an entry roams

[Julius Bairaktaris]

br_fdb_update() lets an entry roam to a new port without holding
hash_lock. It notifies switchdev that the entry left the old port,
writes the new port, then notifies the addition. When two CPUs receive
the same source address on different ports, these steps interleave: a
driver sees two deletions for one addition, or an addition for the port
the other CPU wrote.

DSA counts references to a host address on the CPU port. The extra
deletion fails and the extra addition is never released:

  qca-ppe 3a000000.ppe: port 5 failed to delete 02:5a:0b:a2:1a:46 vid 0 from fdb: -2

With one address roaming between a DSA user port and a Wi-Fi AP port of
the same bridge, the error appears 3-6 times per address when the two
ports receive on different CPUs, and not at all when they share one CPU
(4 runs each). With this change it does not appear (6 runs, different
CPUs).

Take hash_lock when the entry roams or its flags change, and send both
notifications under it. The common case, where the entry neither roams
nor changes, stays lockless.

Fixes: 90dc8fd36078 ("net: bridge: notify switchdev of disappearance of old FDB entry upon migration")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---

Notes:
    net-next 941056f91907 ("net: bridge: fdb: factor out existing entry updates")
    moves this code into __fdb_update(); the same change applies there.

 net/bridge/br_fdb.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e4570bbed854..c8680ae3ef08 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -995,8 +995,17 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 				fdb_modified = __fdb_mark_active(fdb);
 			}
 
-			/* fastpath: update of existing entry */
-			if (unlikely(source != READ_ONCE(fdb->dst) &&
+			if (likely(!fdb_modified &&
+				   (source == READ_ONCE(fdb->dst) ||
+				    test_bit(BR_FDB_STICKY, &fdb->flags)) &&
+				   !test_bit(BR_FDB_ADDED_BY_USER, &flags)))
+				return;
+
+			/* keep a roam and its two switchdev notifications
+			 * atomic against a roam on another CPU
+			 */
+			spin_lock(&br->hash_lock);
+			if (unlikely(source != fdb->dst &&
 				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
 				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
 				WRITE_ONCE(fdb->dst, source);
@@ -1023,6 +1032,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 				trace_br_fdb_update(br, source, addr, vid, flags);
 				fdb_notify(br, fdb, RTM_NEWNEIGH, true);
 			}
+			spin_unlock(&br->hash_lock);
 		}
 	} else {
 		spin_lock(&br->hash_lock);
-- 
2.53.0

Re: [PATCH net] net: bridge: fdb: hold hash_lock when an entry roams

[Nikolay Aleksandrov]

On 04/10/2026 15:42, Julius Bairaktaris wrote:
> br_fdb_update() lets an entry roam to a new port without holding
> hash_lock. It notifies switchdev that the entry left the old port,
> writes the new port, then notifies the addition. When two CPUs receive
> the same source address on different ports, these steps interleave: a
> driver sees two deletions for one addition, or an addition for the port
> the other CPU wrote.
> 
> DSA counts references to a host address on the CPU port. The extra
> deletion fails and the extra addition is never released:
> 
>    qca-ppe 3a000000.ppe: port 5 failed to delete 02:5a:0b:a2:1a:46 vid 0 from fdb: -2
> 
> With one address roaming between a DSA user port and a Wi-Fi AP port of
> the same bridge, the error appears 3-6 times per address when the two
> ports receive on different CPUs, and not at all when they share one CPU
> (4 runs each). With this change it does not appear (6 runs, different
> CPUs).
> 
> Take hash_lock when the entry roams or its flags change, and send both
> notifications under it. The common case, where the entry neither roams
> nor changes, stays lockless.
> 
> Fixes: 90dc8fd36078 ("net: bridge: notify switchdev of disappearance of old FDB entry upon migration")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
> ---
> 
> Notes:
>      net-next 941056f91907 ("net: bridge: fdb: factor out existing entry updates")
>      moves this code into __fdb_update(); the same change applies there.
> 
>   net/bridge/br_fdb.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 

Absolutely not, this was made intentionally. Taking the hash_lock would further kill learning
and roaming scaling. Surely switchdev drivers must have dealt with this for some time
now, if you'd like to fix it do it so the software path isn't affected.

Cheers,
  Nik

Re: [PATCH net] net: bridge: fdb: hold hash_lock when an entry roams

[Julius Bairaktaris]

Hi Nik,

Thanks for the review. Sounds reasonable to me and I will look into it.

Julius

Am So., 4. Okt. 2026 um 13:44 Uhr schrieb Nikolay Aleksandrov
<razor@blackwall.org>:
>
> On 04/10/2026 15:42, Julius Bairaktaris wrote:
> > br_fdb_update() lets an entry roam to a new port without holding
> > hash_lock. It notifies switchdev that the entry left the old port,
> > writes the new port, then notifies the addition. When two CPUs receive
> > the same source address on different ports, these steps interleave: a
> > driver sees two deletions for one addition, or an addition for the port
> > the other CPU wrote.
> >
> > DSA counts references to a host address on the CPU port. The extra
> > deletion fails and the extra addition is never released:
> >
> >    qca-ppe 3a000000.ppe: port 5 failed to delete 02:5a:0b:a2:1a:46 vid 0 from fdb: -2
> >
> > With one address roaming between a DSA user port and a Wi-Fi AP port of
> > the same bridge, the error appears 3-6 times per address when the two
> > ports receive on different CPUs, and not at all when they share one CPU
> > (4 runs each). With this change it does not appear (6 runs, different
> > CPUs).
> >
> > Take hash_lock when the entry roams or its flags change, and send both
> > notifications under it. The common case, where the entry neither roams
> > nor changes, stays lockless.
> >
> > Fixes: 90dc8fd36078 ("net: bridge: notify switchdev of disappearance of old FDB entry upon migration")
> > Assisted-by: Claude:claude-opus-5-5
> > Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
> > ---
> >
> > Notes:
> >      net-next 941056f91907 ("net: bridge: fdb: factor out existing entry updates")
> >      moves this code into __fdb_update(); the same change applies there.
> >
> >   net/bridge/br_fdb.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
>
> Absolutely not, this was made intentionally. Taking the hash_lock would further kill learning
> and roaming scaling. Surely switchdev drivers must have dealt with this for some time
> now, if you'd like to fix it do it so the software path isn't affected.
>
> Cheers,
>   Nik
>
>
>

Re: [PATCH net] net: bridge: fdb: hold hash_lock when an entry roams

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] br_fdb_update() looks up the entry with fdb_find_rcu() and
  checks it without a lock, then takes br->hash_lock…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but on a MAB-
  enabled locked port, br_fdb_update() can roam an existing entry onto…
- [Medium] Peer-site inconsistency, not introduced by this patch.
- [Medium] TOCTOU on BR_FDB_LOCAL, not introduced by this patch.

--

net: bridge: fdb: hold hash_lock when an entry roams

With this patch, br_fdb_update() takes br->hash_lock when an existing
entry roams to a new port or its flags change. The switchdev delete and
add notifications for the roam are then sent under that lock. The common
case, where the entry neither roams nor changes, stays lockless.

> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e4570bbed8544..c8680ae3ef08b 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -995,8 +995,17 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  				fdb_modified = __fdb_mark_active(fdb);
>  			}
>  
> -			/* fastpath: update of existing entry */
> -			if (unlikely(source != READ_ONCE(fdb->dst) &&
> +			if (likely(!fdb_modified &&
> +				   (source == READ_ONCE(fdb->dst) ||
> +				    test_bit(BR_FDB_STICKY, &fdb->flags)) &&
> +				   !test_bit(BR_FDB_ADDED_BY_USER, &flags)))
> +				return;
> +
> +			/* keep a roam and its two switchdev notifications
> +			 * atomic against a roam on another CPU
> +			 */
> +			spin_lock(&br->hash_lock);
> +			if (unlikely(source != fdb->dst &&
>  				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
>  				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
>  				WRITE_ONCE(fdb->dst, source);

[Severity: High]
Is fdb still in the table by the time hash_lock is taken here?

The entry was found with fdb_find_rcu() before the lock was taken. In
that window, any hash_lock holder can run fdb_delete() on it:

net/bridge/br_fdb.c:fdb_delete() {
    ...
	hlist_del_init_rcu(&f->fdb_node);
	rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode,
			       br_fdb_rht_params);
    ...
	fdb_notify(br, f, RTM_DELNEIGH, swdev_notify);
	kfree_rcu(f, rcu);
}

fdb_delete() does not touch fdb->dst. So when this CPU gets the lock,
source != fdb->dst is still true and the following happens:

- a second switchdev RTM_DELNEIGH goes out for the old port
- fdb->dst is rewritten
- a switchdev add and a netlink RTM_NEWNEIGH go out for the new port

At that point the entry is in neither fdb_list nor the rhashtable, so
nothing will ever send the matching delete. The next frame from that MAC
then goes through fdb_create() and sends another add.

Isn't this the same "two deletions for one addition" imbalance the commit
message describes, with the DSA CPU port host address refcount leaking?

Possible deleters include br_fdb_delete_by_port() (port disable or
removal), br_fdb_cleanup() (ageing and STP fast ageing), br_fdb_flush(),
and user or external-learn deletes. br_fdb_delete_by_port() holds
hash_lock for its whole walk. A roaming CPU spinning on the lock would
only get it after the entry has already been removed.

Other code in this file that finds an entry locklessly re-checks it after
taking the lock. For example, br_fdb_cleanup():

			spin_lock_bh(&br->hash_lock);
			if (!hlist_unhashed(&f->fdb_node))
				fdb_delete(br, f, true);
			spin_unlock_bh(&br->hash_lock);

Would a similar check right after spin_lock() be needed here?

	if (unlikely(hlist_unhashed(&fdb->fdb_node))) {
		spin_unlock(&br->hash_lock);
		return;
	}

[Severity: Medium]
This isn't a bug introduced by this patch, but br_fdb_update() tests
BR_FDB_LOCAL before taking the lock and does not test it again inside the
new hash_lock section:

		if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) {

A concurrent "bridge fdb replace ... permanent" runs fdb_add_entry() under
hash_lock. In that window it can set the local and static bits on the
entry:

		if (state & NUD_PERMANENT) {
			set_bit(BR_FDB_LOCAL, &fdb->flags);
			if (!test_and_set_bit(BR_FDB_STATIC, &fdb->flags))
				fdb_add_hw_addr(br, addr);

Could the roam then move a newly permanent entry onto the receiving port?
Since the lock is now held here anyway, could BR_FDB_LOCAL be re-checked
under it?

The impact looks limited. The switchdev notifications stay balanced
because both target br->dev for local entries, and frames to a local
entry are still delivered locally. Hitting this needs CAP_NET_ADMIN and
the right timing.

[Severity: High]
This is a pre-existing issue, but can this roam move an entry onto a
locked MAB port and leave it unlocked?

br_handle_frame_finish() decides about the locked port from a lockless
lookup. On a mismatch it drops "without roaming":

net/bridge/br_input.c:br_handle_frame_finish() {
    ...
		if (!fdb_src) {
			...
			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;
		} else if (READ_ONCE(fdb_src->dst) != p ||
			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
			/* FDB mismatch. Drop the packet without roaming. */
			goto drop;
		} else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
			...
			br_fdb_update(br, p, eth_hdr(skb)->h_source, vid,
				      BIT(BR_FDB_LOCKED));
			goto drop;
		}
    ...
}

br_fdb_update() then does the lookup again. On the existing-entry path it
ignores BIT(BR_FDB_LOCKED) in flags. It roams whenever source != fdb->dst
and BR_FDB_STICKY is clear, and it clears BR_FDB_LOCKED on the assumption
that it is "roaming to an unlocked port".

Two interleavings seem possible.

In the first, the lookup in br_handle_frame_finish() misses. Before
br_fdb_update() does its own lookup, a legitimate host on unlocked port q
creates the entry. The entry is then roamed onto locked port p with
BR_FDB_LOCKED cleared.

In the second, a locked entry for MAC M sits on p. The legitimate host on
q roams it to q, which clears BR_FDB_LOCKED. A concurrent frame on p has
already passed the dst == p && BR_FDB_LOCKED check. It calls
br_fdb_update(br, p, ..., BIT(BR_FDB_LOCKED)), sees dst == q, and roams
the entry back to p, unlocked.

In both cases, later frames from p with that source MAC pass the locked
port check and are forwarded. Traffic for M also goes to p until the
host roams the entry back.

The roam now runs under hash_lock. Would that be a good place to refuse
the roam when BIT(BR_FDB_LOCKED) is set in flags, or when the source port
is locked?

[ ... ]

> @@ -1023,6 +1032,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  				trace_br_fdb_update(br, source, addr, vid, flags);
>  				fdb_notify(br, fdb, RTM_NEWNEIGH, true);
>  			}
> +			spin_unlock(&br->hash_lock);
>  		}

[Severity: Medium]
This isn't a bug introduced by this patch, but the other places that
change fdb->dst don't use the order this path uses: delete for the old
port, then add for the new one.

fdb_add_entry() does:

		if (READ_ONCE(fdb->dst) != source) {
			WRITE_ONCE(fdb->dst, source);
			modified = true;
		}

br_fdb_external_learn_add() does:

		if (READ_ONCE(fdb->dst) != p) {
			WRITE_ONCE(fdb->dst, p);
			modified = true;
		}

Afterwards both only call fdb_notify() with RTM_NEWNEIGH. Neither sends a
switchdev RTM_DELNEIGH for the old port.

Can a "bridge fdb replace" that moves a static or ext-learned entry
between a foreign port and a DSA user port unbalance the DSA CPU port
host address refcount in the same way? This needs CAP_NET_ADMIN, and it
is separate from the race this patch addresses.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004124254.3525496-1-julius%40bairaktaris.de