Re: [PATCH net-next 2/2] net: dsa: fix host fdb refcount when a bridge fdb entry roams

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 7 · Low: 2

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] The fix does not cover cross-tree bridges, although the commit
  message says it does.
- [High] The join rollback path out_rollback_unbridge now dereferences
  NULL in queued work.
- [Medium] The commit message says "Addresses local to the bridge do not
  roam and are still counted." That is wrong for `bridge fdb replace`.
- [Medium] dsa_user_bridge_host_fdb_sync() sends SWITCHDEV_FDB_OFFLOADED
  for every ADD where want is true, even when the host address was not…
- [Medium] Retrying a failed host delete drops the conduit's unicast-
  filter reference twice.
- [Medium] Every dynamically learned ADD on a DSA user port now costs a
  GFP_ATOMIC allocation and a work item, on every DSA switch.
- [Medium] On a "bridge fdb replace" of an address from a foreign port
  onto a DSA user port, the user's new static entry is erased from…
- [Medium] net: dsa: dsa_user_bridge_host_fdb_wanted() ignores
  assisted_learning_on_cpu_port
- [Medium] net: dsa: dsa_user_fdb_event() skips host FDB release on LAG
  without lag_fdb_add
- [Low] The patch fixes a user-visible bug (`port N failed to delete ...
  from fdb: -2` and host addresses left on the CPU port) but has no…
- [Low] The patch turns dp->bridge into a lockless clear-then-flush
  protocol: WRITE_ONCE(dp->bridge, NULL), then dsa_flush_workqueue(),…

--

net: dsa: fix host fdb refcount when a bridge fdb entry roams

This replaces the counted host address handling for bridge FDB entries
on foreign ports with a per-user-port list of held addresses. The list
is re-evaluated against the bridge FDB on every event. Additions on a
user port schedule a release of the address on the other ports of the
bridge, and dsa_port_bridge_destroy() now flushes the workqueue and
purges the held records.

> are released. Addresses local to the bridge do not roam and are still
> counted. The software bridge is not changed.

[ ... ]

> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>

[Severity: Low]
This fixes a user-visible error ("port 1 failed to delete ... from fdb:
-2") and a host address left on the CPU port, but there is no Fixes:
tag. Could one be added so stable and backporters can tell where this
applies?

The fix also depends on br_switchdev_fdb_find_port(), which
dsa_user_bridge_host_fdb_wanted() calls. That helper is added by the
preceding bridge commit ("net: bridge: switchdev: add
br_switchdev_fdb_find_port()").

Given "The software bridge is not changed" in the message, could that
dependency be stated explicitly?

> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 1f5536c0dffc2..e270a0c5e6641 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -457,7 +457,13 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp,
>  {
>  	struct dsa_bridge *bridge = dp->bridge;
>  
> -	dp->bridge = NULL;
> +	WRITE_ONCE(dp->bridge, NULL);
> +
> +	/* Wait for the FDB events that still see the bridge, then release the
> +	 * host addresses the port holds, before the drivers are told it left.
> +	 */
> +	dsa_flush_workqueue();
> +	dsa_user_bridge_host_fdb_purge(dp, bridge);
>  
>  	if (!refcount_dec_and_test(&bridge->refcount))
>  		return;
> @@ -528,7 +534,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>  					&dsa_user_switchdev_blocking_notifier);
>  	dsa_flush_workqueue();
>  out_rollback_unbridge:
> +	dsa_port_bridge_destroy(dp, br);
>  	dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
> +	return err;
>  out_rollback:
>  	dsa_port_bridge_destroy(dp, br);
>  	return err;

[Severity: High]
Can the pending FDB works dereference a NULL bridge on this rollback
path?

out_rollback_unbridge is reached when switchdev_bridge_port_offload()
fails, and nothing unoffloads or flushes first. Its FDB replay
(br_switchdev_fdb_replay()) delivers every bridge FDB entry with
ctx = dp. Local entries such as the bridge and port MACs reach
dsa_user_fdb_event() with is_local set, so they get host_addr = true
and sync = false. A plain dsa_user_switchdev_event_work() item is
queued for each one.

If a later replayed entry fails, the replay stops and the error comes
back here. One way this happens is the GFP_ATOMIC allocation in
dsa_user_fdb_event(). Another is the new
dsa_user_bridge_host_fdb_release(), which returns -ENOMEM after a work
item has already been queued.

dsa_port_bridge_destroy() now does WRITE_ONCE(dp->bridge, NULL) before
dsa_flush_workqueue(). So every work still pending runs with
dp->bridge == NULL, and only the new sync branch checks for that. The
non-sync ADD branch does:

dsa_user_switchdev_event_work()
  dsa_port_bridge_host_fdb_add(dp, addr, vid)
    __dsa_port_bridge_host_fdb_add(dp, dp->bridge, addr, vid)
      .bridge = *bridge,

dsa_port_fdb_add() also evaluates *dp->bridge.

The commit message says the workqueue is flushed when the port "fails
to join" the bridge. Does the non-sync path need a NULL bridge check
too, or should the flush happen before dp->bridge is cleared?

[ ... ]

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 041f9060c8ef0..7f18c7885b9c5 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c

[ ... ]

> @@ -3639,6 +3658,171 @@ dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
>  				 switchdev_work->orig_dev, &info.info, NULL);
>  }
>  

[ ... ]

> +static void dsa_user_bridge_host_fdb_set(struct net_device *dev,
> +					 const struct dsa_bridge *bridge,
> +					 const unsigned char *addr, u16 vid,
> +					 bool want)
> +{

[ ... ]

> +	} else {
> +		err = __dsa_port_bridge_host_fdb_del(dp, bridge, addr, vid);
> +		if (err) {
> +			dev_err(dp->ds->dev,
> +				"port %d failed to delete %pM vid %d from fdb: %d\n",
> +				dp->index, addr, vid, err);
> +			return;
> +		}

[Severity: Medium]
After a failed delete the record is kept. Can that call dev_uc_del()
on the conduit more than once for a single dev_uc_add()?

__dsa_port_bridge_host_fdb_del() does:

	if (conduit->priv_flags & IFF_UNICAST_FLT) {
		err = dev_uc_del(conduit, addr);
		if (err)
			return err;
	}

	return dsa_port_host_fdb_del(dp, addr, vid, db);

Suppose dev_uc_del() succeeds and dsa_port_host_fdb_del() fails, for
example with a driver port_fdb_del() error. The dev_uc_del() is not
undone, but the record stays on bridge_host_fdbs. A later want == false
event, or dsa_user_bridge_host_fdb_purge(), then repeats the whole
delete, including another dev_uc_del().

The conduit uc list is refcounted per MAC and shared across VIDs,
bridges and other host address users. The extra delete can drop
another holder's reference.

When that holder later releases, dev_uc_del() returns -ENOENT, and
__dsa_port_bridge_host_fdb_del() returns before the switch delete.
dsa_user_bridge_host_fdb_purge() then logs the error and frees the
record anyway:

	err = __dsa_port_bridge_host_fdb_del(dp, bridge, a->addr,
					     a->vid);
	if (err)
		dev_err(...);
	list_del(&a->list);
	kfree(a);

Would the CPU port FDB entry then stay programmed, with nothing left
to remove it?

> +
> +		list_del(&a->list);
> +		kfree(a);
> +	}
> +}
> +
> +static void
> +dsa_user_bridge_host_fdb_sync(struct dsa_switchdev_event_work *switchdev_work)
> +{

[ ... ]

> +	dsa_user_bridge_host_fdb_set(dev, bridge, switchdev_work->addr,
> +				     switchdev_work->vid, want);
> +	if (want && !del)
> +		dsa_fdb_offload_notify(switchdev_work);
> +}

[Severity: Medium]
Is SWITCHDEV_FDB_OFFLOADED sent here even when the host address was not
installed?

dsa_user_bridge_host_fdb_set() returns void. It can return early
without installing anything:

	if (want) {
		a = kzalloc_obj(*a);
		if (!a)
			return;

It does the same when __dsa_port_bridge_host_fdb_add() fails.

The caller still calls dsa_fdb_offload_notify() whenever want is true,
so the bridge marks the entry offloaded. Before this patch, the error
path in dsa_user_switchdev_event_work() did a break and skipped the
notification.

Could dsa_user_bridge_host_fdb_set() report whether the address ended
up held, so the notification is only sent then?

> +
> +/* Only the user port an entry is on gets its events, so when the entry roams
> + * there, release it on the ports of the bridge that still hold it.
> + */
> +static void dsa_user_bridge_host_fdb_release_work(struct work_struct *work)
> +{
> +	struct dsa_switchdev_event_work *switchdev_work =
> +		container_of(work, struct dsa_switchdev_event_work, work);
> +	struct dsa_port *dp = dsa_user_to_port(switchdev_work->dev);
> +	const unsigned char *addr = switchdev_work->addr;
> +	const struct dsa_bridge *bridge = READ_ONCE(dp->bridge);
> +	u16 vid = switchdev_work->vid;
> +	struct dsa_port *other;
> +
> +	if (bridge) {
> +		dsa_tree_for_each_user_port(other, dp->ds->dst) {

[Severity: High]
Does this release reach the user ports of other trees in the same
bridge?

The commit message says an entry on a user port, "also one of another
tree", is not held. It also says "an addition there releases the
address on the other ports of the bridge". This loop only walks
dp->ds->dst, which is the tree of the port that got the ADD.

For an entry on a DSA user port,
__switchdev_handle_fdb_event_to_device() only delivers the event to
that port:

	if (check_cb(dev))
		return mod_cb(dev, orig_dev, event, info->ctx, fdb_info);

Take trees A and B in one bridge, with ADDR on a foreign port such as
a veth. Every user port of both trees holds ADDR. Then run this, where
swpB is a tree B user port:

  bridge fdb replace ADDR dev swpB master static

fdb_add_entry() only notifies RTM_NEWNEIGH. Only swpB gets the event,
and the release work only visits tree B. Tree A's ports never call
dsa_user_bridge_host_fdb_set(..., false).

Would ADDR stay on tree A's CPU port until those ports leave the
bridge, even after the entry is deleted? Should this walk every tree
in dsa_tree_list instead?

> +			if (dsa_port_offloads_bridge_dev(other, bridge->dev) &&
> +			    !dsa_user_bridge_host_fdb_wanted(other->user, bridge,
> +							     addr, vid, NULL))
> +				dsa_user_bridge_host_fdb_set(other->user, bridge,
> +							     addr, vid, false);

[Severity: Low]
Do these reads of the other ports' dp->bridge race with
dsa_port_bridge_destroy() on those ports?

dsa_port_bridge_destroy() clears dp->bridge with WRITE_ONCE() before it
flushes the workqueue. So this work can run while port "other" is
leaving the bridge.

dsa_port_offloads_bridge_dev() goes through dsa_port_bridge_dev_get():

	return dp->bridge ? dp->bridge->dev : NULL;

That is two plain loads. dsa_user_bridge_host_fdb_wanted() reaches
similar plain reads through dsa_foreign_dev_check() ->
dsa_tree_offloads_bridge_dev() / dsa_tree_offloads_bridge_port(). On
the writer side, dsa_port_bridge_create() still publishes with a plain
dp->bridge = bridge.

KCSAN would report these races. If the compiler reloads dp->bridge
between the check and the dereference, the dereference can see NULL.

Should the remaining dp->bridge accesses in this scheme be annotated
too?

> +		}
> +	}
> +
> +	kfree(switchdev_work);
> +}

[ ... ]

> @@ -3723,7 +3897,9 @@ static int dsa_user_fdb_event(struct net_device *dev,
>  
>  	if (switchdev_fdb_is_dynamically_learned(fdb_info)) {
>  		if (dsa_port_offloads_bridge_port(dp, orig_dev))
> -			return 0;
> +			return event == SWITCHDEV_FDB_ADD_TO_DEVICE ?
> +			       dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
> +								fdb_info->vid) : 0;

[Severity: Medium]
When ds->assisted_learning_on_cpu_port is not set, can this keep a
host address held that nothing will ever release?

dsa_user_fdb_event() drops dynamically learned entries on foreign
ports, both ADD and DEL, when assisted_learning_on_cpu_port is false.
User-added entries on foreign ports still go through
dsa_user_bridge_host_fdb_sync().

The lookup here does not make that distinction.
br_switchdev_fdb_find_port() returns the port of any non-local entry,
including one the bridge learned dynamically.

Consider a static entry for ADDR on veth0, where veth0 and veth1 are
both foreign ports of the bridge. Then run:

  bridge fdb del ADDR dev veth0 master

Suppose a frame from ADDR arrives on veth1 before the DEL work runs on
dsa_owq, and the bridge learns ADDR there. That ADD is dynamically
learned, so dsa_user_fdb_event() returns 0 for it.

When the DEL work then calls dsa_user_bridge_host_fdb_wanted(), brport
is veth1. veth1 is not leaving, is not a DSA user port, and is
foreign. So want is true and the record on bridge_host_fdbs is kept.

When the dynamic entry on veth1 ages out, its DEL is dropped by the
same assisted_learning_on_cpu_port check, so no sync work is queued.
The address stays on the CPU port until the user ports leave the
bridge.

Should dsa_user_bridge_host_fdb_wanted() apply the same filter as
dsa_user_fdb_event()? For example, the lookup could also report
whether the entry was dynamically learned. Or should DEL events for
addresses a port holds bypass that filter?

[Severity: Medium]
Does every dynamically learned ADD on a user port now cost a GFP_ATOMIC
allocation and a work item, on every DSA switch?

Before this patch, this branch returned 0 right away. It now runs
before the ds->assisted_learning_on_cpu_port check. So it applies even
when no port holds any bridge host address and the work has nothing to
do.

Each release work walks every user port of the tree. For each bridged
port it does an RCU FDB lookup and a linear scan of bridge_host_fdbs.
All of this runs on the single ordered dsa_owq. Non-host static ADDs
also queue two work items, the normal one and the release.

An L2 peer on a user port can send frames with changing source MACs.
The bridge learns each one, since the default fdb_max_learned of 0
means no limit. Each learn then queues work that competes with real
FDB programming on the ordered workqueue.

Is this cost intended for switches that never hold bridge host
addresses?

>  
>  		/* FDB entries learned by the software bridge or by foreign
>  		 * bridge ports should be installed as host addresses only if
> @@ -3768,9 +3944,15 @@ static int dsa_user_fdb_event(struct net_device *dev,
>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>  	switchdev_work->vid = fdb_info->vid;
>  	switchdev_work->host_addr = host_addr;
> +	switchdev_work->sync = host_addr && !fdb_info->is_local;

[Severity: Medium]
The commit message says "Addresses local to the bridge do not roam and
are still counted." Does that hold for bridge fdb replace?

In fdb_add_entry(), an existing entry on a foreign port can be turned
into a local br0 entry in place. fdb_update_dst() changes the
destination, NUD_PERMANENT sets BR_FDB_LOCAL, and only RTM_NEWNEIGH is
notified, with no deletion. The reverse change is also possible.

Here the local ADD has is_local set, so it gets sync = false. It goes
to the counted path, which never touches bridge_host_fdbs. Consider:

  bridge fdb replace ADDR dev br0 self local
  bridge fdb del ADDR dev br0 self

Only the counted reference is dropped. The records held from the
foreign phase keep ADDR on the CPU port until the ports leave the
bridge.

In the other direction, from local to static on a foreign port, the
counted reference from the local phase is never dropped.

Should this replace case be handled like the user port one?

> +	switchdev_work->replay = !!ctx;
>  
>  	dsa_schedule_work(&switchdev_work->work);
>  
> +	if (!host_addr && event == SWITCHDEV_FDB_ADD_TO_DEVICE)
> +		return dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
> +							fdb_info->vid);
> +

[Severity: Medium]
Can the release be skipped when a static entry moves onto a LAG?

Take a user-added entry on a LAG that the port offloads. orig_dev is
the LAG device, dsa_foreign_dev_check() returns false, and host_addr
stays false. dsa_user_fdb_event() then reaches this check, which comes
before the new call to dsa_user_bridge_host_fdb_release():

	if (dp->lag && !host_addr) {
		if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
			return -EOPNOTSUPP;

Drivers that offload LAGs but have no lag_fdb_add/lag_fdb_del, such
as mv88e6xxx and qca8k, return -EOPNOTSUPP there.

Suppose every user port of the bridge holds ADDR as a host address,
because the entry is on a foreign port such as a veth. Then run:

  bridge fdb replace ADDR dev bond0 master static

Here bond0 is an offloaded LAG of user ports. fdb_add_entry() only
notifies RTM_NEWNEIGH, so no DEL for the veth reaches DSA. The commit
message relies on the release for this replace case, but the release
is never scheduled. Nothing later re-evaluates the records either.
The address stays on the CPU port until the user ports leave the
bridge.

Dynamically learned ADDs on the LAG are not affected, since they take
the dsa_port_offloads_bridge_port() branch earlier. Should the release
be scheduled before the lag_fdb_add/lag_fdb_del check, so it does not
depend on whether the driver can install the entry itself?

[Severity: Medium]
On a bridge fdb replace from a foreign port onto a user port, can the
release erase the static entry that was just installed on the user
port?

The normal work queued above installs the entry on the user port
through ds->ops->port_fdb_add(). The release work runs after it on the
ordered dsa_owq. Dropping the last hold reaches ds->ops->port_fdb_del()
on the CPU port for the same MAC and VID.

On switches with a single unicast entry per {MAC, VID}, that delete
does not check which port owns the entry. b53_arl_op() does this for
unicast, with is_valid false on a delete:

	if (!is_multicast_ether_addr(addr)) {
		ent.port = port;
		ent.is_valid = is_valid;

mt7530_port_fdb_del() writes STATIC_EMP for the MAC/VID whatever the
owner:

	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_EMP);

ocelot_fdb_del() takes no port argument:

	return ocelot_mact_forget(ocelot, addr, vid);

Would the static entry then be missing from hardware, even though the
bridge already received SWITCHDEV_FDB_OFFLOADED for it?

>  	return 0;
>  }

[ ... ]

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