Re: [PATCH net v3] net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (1) · Medium: 1 (4) · 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 close the bug class its commit message
  describes, 'Dynamic (hardware-learned) ALE entries are not removed on…
- [High] Remote L2 traffic can now force many full ALE MMIO scans while
  RTNL is held.
- [Medium] The patch removes the `!fdb->added_by_user` gate from the
  SWITCHDEV_FDB_DEL_TO_DEVICE path in am65_cpsw_switchdev_event_work()…
- [Low] The runtime PM reference added to the FDB event work only matters
  when no port is open, and in that state the ALE has already been…
- [Low] The submission justifies a full ALE walk on every dynamic delete
  with the claim that 'The ALE size is small (max 512 entries on the…

Pre-existing issues:
- [High] Nothing serializes ALE table accesses, and this patch adds a
  frequent new full-table walker that races with existing non-RTNL ALE…
- [Medium] This problem wasn't introduced by this patch, but the patch
  makes it more visible.
- [Medium] This problem wasn't introduced by this patch.
- [Medium] This problem wasn't introduced by this patch.
- [Medium] This problem wasn't introduced by this patch.

--

net: ti: am65-cpsw-switchdev: flush dynamic FDB entries by port on delete

This patch lets dynamic (not user-added) FDB deletes reach the ALE in the
am65-cpsw and cpsw switchdev event work. It does this through a new helper,
cpsw_ale_del_ucast_dynamic_by_port(), which matches on (addr, port) and
treats vid 0 as a wildcard. It also takes a runtime PM reference around
the ALE accesses in the FDB event work.

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> index 53cdac272b583..bc4e19e2373f5 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c

[ ... ]

> @@ -383,12 +385,20 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
>  
>  		if (!fdb->added_by_user || fdb->is_local)
>  			break;
> +
> +		ret = pm_runtime_resume_and_get(cpsw->dev);
> +		if (ret < 0) {
> +			netdev_err(port->ndev, "%s: failed to resume device: %d\n", __func__, ret);
> +			break;
> +		}

[Severity: Medium]
This isn't a bug introduced by this patch, but the patch makes it more
visible. These are the only pm_runtime calls in am65-cpsw-switchdev.c and
cpsw_switchdev.c. The other switchdev handlers write the same ALE without
a runtime PM reference:

am65_cpsw_port_stp_state_set()
	ret = cpsw_ale_control_set(cpsw->ale, port->port_id,
				   ALE_PORT_STATE, cpsw_state);

The same is true of am65_cpsw_port_attr_br_flags_set() ->
cpsw_ale_set_unreg_mcast(), the vlan_add/vlan_del and mdb_add/mdb_del
handlers, and their counterparts in cpsw_switchdev.c.

The port-down sequence named in the commit message also reaches the STP
handler, through deferred switchdev work:

NETDEV_DOWN -> br_stp_disable_port() -> br_set_state(BR_STATE_DISABLED)
    (SWITCHDEV_F_DEFER) -> am65_cpsw_port_stp_state_set()
        -> cpsw_ale_control_set()

That can run after the last port's asynchronous pm_runtime_put() has
already suspended the device.

If the new references are needed, do the sibling handlers need them too?
If they are not needed, are the new ones redundant?

> +
>  		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
>  			port_id = HOST_PORT_NUM;
>  
>  		cpsw_ale_add_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
>  				   fdb->vid ? ALE_VLAN : 0, fdb->vid);
>  		am65_cpsw_fdb_offload_notify(port->ndev, fdb);

[Severity: Medium]
This is a pre-existing issue, but the return value of cpsw_ale_add_ucast()
is ignored here, and SWITCHDEV_FDB_OFFLOADED is always sent.

If cpsw_ale_add_ucast() finds no matching, free or ageable row, it returns
-ENOMEM without writing anything:

cpsw_ale_add_ucast()
	if (idx < 0)
		idx = cpsw_ale_find_ageable(ale);
	if (idx < 0)
		return -ENOMEM;

Can the bridge then set BR_FDB_OFFLOADED on an entry that is not in
hardware? cpsw_switchdev_event_work() does the same thing with
cpsw_fdb_offload_notify().

> +		pm_runtime_put(cpsw->dev);
>  		break;
>  	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>  		fdb = &switchdev_work->fdb_info;
> @@ -397,13 +407,27 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
>  			   fdb->addr, fdb->vid, fdb->added_by_user,
>  			   fdb->offloaded, port_id);
>  
> -		if (!fdb->added_by_user || fdb->is_local)
> +		if (fdb->is_local)
>  			break;

[Severity: Medium]
Flushes are not the only source of dynamic deletes. With this gate gone,
does bridge software ageing now free live ALE rows too?

br_fdb_cleanup() expires ordinary learned entries with
fdb_delete(br, f, true), which leads to:

fdb_delete() -> fdb_notify(RTM_DELNEIGH) -> br_switchdev_fdb_notify()
    -> SWITCHDEV_FDB_DEL_TO_DEVICE (added_by_user=0, is_local=0)
        -> am65_cpsw_switchdev_event_work()
            -> cpsw_ale_del_ucast_dynamic_by_port()

Neither TI driver handles SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME, and
neither sends SWITCHDEV_FDB_ADD_TO_BRIDGE. So the software FDB entry is
refreshed only by frames that reach the host port. Known unicast that the
hardware forwards port-to-port never refreshes it.

The bridge entry expires after ageing_time (300s by default), or after
forward_delay during a topology change. The DEL then frees the ALE row,
even if the hardware has it marked as touched and in active use. With
vid=0 this happens in every VLAN. Unicast to that station then floods
until the station transmits again.

The v3 changelog says the ageing trade-off "has been documented in the
commit message". The commit message doesn't mention that bridge software
ageing now removes hardware FDB entries. Could that be documented, or
could the driver avoid this behaviour?

The same applies to cpsw_switchdev_event_work().

[Severity: High]
Does this fully fix the problem the commit message describes?

  Dynamic (hardware-learned) ALE entries are not removed on FDB flush and
  sit stale until the ALE hardware ages them out ~30 seconds later.

The new code only reacts to per-entry SWITCHDEV_FDB_DEL_TO_DEVICE
notifications. The bridge sends those only for entries in its own
software FDB, and it only learns from frames that reach the CPU. The ALE
learns from all ingress traffic.

For example, after a flush, A sends a frame to an unknown B. The frame
floods to the host, so the bridge learns A. B replies to the now-known A,
and the switch forwards the reply in hardware. The ALE learns B, but the
bridge does not. On the next flush or port disable no DEL is sent for B,
so B's ALE row stays stale until hardware ageout.

The commit message also says:

  Dynamic deletes are the only way to remove a hardware-learned entry
  early, so drop that gate for the delete path.

Is that accurate? The driver could fast-age a port's dynamic unicast
entries itself, the way DSA's port_fast_age does. For example, it could
do this from am65_cpsw_port_stp_state_set() and cpsw_port_stp_state_set()
on DISABLED, BLOCKING and LISTENING transitions.
cpsw_ale_flush_multicast() already walks the table by port mask in a
similar way. Today the STP handlers only write ALE_PORT_STATE:

am65_cpsw_port_stp_state_set()
	ret = cpsw_ale_control_set(cpsw->ale, port->port_id,
				   ALE_PORT_STATE, cpsw_state);

A per-port fast-age would also avoid the ageing side effect described
above.

> -		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
> -			port_id = HOST_PORT_NUM;
>  
> -		cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
> -				   fdb->vid ? ALE_VLAN : 0, fdb->vid);
> +		ret = pm_runtime_resume_and_get(cpsw->dev);
> +		if (ret < 0) {
> +			netdev_err(port->ndev, "%s: failed to resume device: %d\n", __func__, ret);
> +			break;
> +		}

[Severity: Low]
When does this reference actually make a difference? Every open port
holds a PM reference from am65_cpsw_nuss_ndo_slave_open() or
cpsw_ndo_open(). When the last port stops, the ALE is cleared and
disabled before that reference is dropped:

am65_cpsw_nuss_common_stop()  (usage_count == 1)
cpsw_ndo_stop()               (usage_count <= 1)
    cpsw_ale_stop()
	cpsw_ale_control_set(ale, 0, ALE_CLEAR, 1);
	cpsw_ale_control_set(ale, 0, ALE_ENABLE, 0);

So the resume only matters when the ALE is already empty. In that case
cpsw_ale_del_ucast_dynamic_by_port() scans an empty table and cannot
delete anything. Each dynamic DEL queued when the last port goes down
(one per learned MAC) powers the device up just to do that scan.

On the ADD side, the entry is written into a cleared, disabled ALE, and
SWITCHDEV_FDB_OFFLOADED is still sent. cpsw_ale_start() then clears it
again on the next open.

The commit message says "an ADD/DEL that reaches the ALE needs its own
reference". The reference only avoids MMIO to a suspended device; the ALE
update itself has no effect. Is the commit message misleading here?
Would pm_runtime_get_if_active() be a better fit, or skipping the ALE
access when no port is open?

> +
> +		if (!fdb->added_by_user) {
> +			cpsw_ale_del_ucast_dynamic_by_port(cpsw->ale,
> +							   (u8 *)fdb->addr,
> +							   port_id,
> +							   fdb->vid);

[Severity: High]
Can remote L2 traffic now force many full ALE scans while RTNL is held?

Before this patch, dynamic DEL work items returned right after
rtnl_lock(). Now each one walks ale->params.ale_entries rows: 512 on
am64-cpswxg and 1024 on cpsw_new. Each row costs one MMIO write and three
MMIO reads. The whole table is scanned when vid is 0, and also when
nothing matches.

Untrusted traffic can generate dynamic DELs. br_fdb_update() sends one
every time a MAC moves:

br_fdb_update()
	if (unlikely(source != READ_ONCE(fdb->dst) &&
		     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
		br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);

So a source MAC that alternates between two ports causes a DEL on every
move.

Flooding random source MACs from a single port also works, because
fdb_max_learned is unlimited by default. When those entries age out,
br_fdb_cleanup() sends one DEL per entry, and each DEL is a full scan
that finds nothing.

Every notification also allocates a GFP_ATOMIC work item, with no bound
on how many. Could this cause long RTNL contention and a growing backlog
of queued work on small SoCs?

The same applies to cpsw_switchdev_event_work().

> +		} else {
> +			if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
> +				port_id = HOST_PORT_NUM;
> +			cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
> +					   fdb->vid ? ALE_VLAN : 0, fdb->vid);

[Severity: Medium]
This is a pre-existing issue, but am65_cpsw_switchdev_event() and
cpsw_switchdev_event() queue a separate work item for each FDB
notification:

	queue_work(system_long_wq, &switchdev_work->work);

system_long_wq is created with alloc_workqueue("events_long", WQ_PERCPU, 0)
and is not ordered. The workers serialize on rtnl_lock but can acquire it
in any order.

If a user-added ADD is followed by a DEL for the same entry, can they run
as DEL then ADD? That would leave a stale ALE_UCAST_PERSISTANT entry in
hardware after the user has deleted it.

> +		}
> +		pm_runtime_put(cpsw->dev);
>  		break;
>  	default:
>  		break;
> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index e202bba494807..d3d6a1ef894ac 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c

[ ... ]

> +int cpsw_ale_del_ucast_dynamic_by_port(struct cpsw_ale *ale, const u8 *addr,
> +				       int port, u16 vid)
> +{
> +	u32 ale_entry[ALE_ENTRY_WORDS];
> +	int type, ucast_type, idx;
> +	u8 entry_addr[6];
> +	int deleted = 0;
> +	int entry_vid;
> +
> +	for (idx = 0; idx < ale->params.ale_entries; idx++) {

[Severity: Low]
The v2 changelog justifies a full walk on every dynamic delete with:

  The ALE size is small (max 512 entries on the largest supported device)

Is that right for cpsw_switchdev.c? cpsw_init_common() in cpsw_priv.c
sets ale_params.dev_id = "cpsw", and the "cpsw" entry in
cpsw_ale_id_match has .tbl_entries = 1024. On that driver, every vid=0
delete and every delete that finds no match walks 1024 rows.

> +		cpsw_ale_read(ale, idx, ale_entry);

[Severity: High]
This is a pre-existing issue, but this patch adds a new full-table walker
that runs often. Nothing serializes ALE table accesses.

cpsw_ale_read() and cpsw_ale_write() share one indirect window: write the
index to ALE_TABLE_CONTROL, then access the ALE_TABLE words. There is no
ALE lock.

This walk runs from the switchdev work, holding only rtnl_mutex.
am65_cpsw_nuss_ndo_slave_set_rx_mode() calls cpsw_ale_set_allmulti(),
cpsw_ale_flush_multicast() and cpsw_ale_add_mcast() holding only
netif_addr_lock_bh. An unprivileged user can reach that path without
RTNL:

IPV6_ADD_MEMBERSHIP -> ipv6_sock_mc_join() -> igmp6_group_added()
    -> dev_mc_add() -> am65_cpsw_nuss_ndo_slave_set_rx_mode()
        -> cpsw_ale_flush_multicast()

cpsw_ale_flush_multicast() writes back every ADDR/VLAN_ADDR row. The
index write here can land between the other walker's index write and its
word reads or commit. If it does, the other walker reads or commits the
wrong row's contents.

It can also go the other way: this loop could read a row other than idx
and then write ALE_TYPE_FREE to idx.

Separately, am65_cpsw_nuss_suspend() calls cpsw_ale_dump() with no lock,
while this work can still run on the non-freezable system_long_wq.

Can this free or overwrite VLAN, persistent unicast or multicast rows?

> +		type = cpsw_ale_get_entry_type(ale_entry);

[ ... ]

> diff --git a/drivers/net/ethernet/ti/cpsw_switchdev.c b/drivers/net/ethernet/ti/cpsw_switchdev.c
> index 7e06aac388a60..7fbebe666da81 100644
> --- a/drivers/net/ethernet/ti/cpsw_switchdev.c
> +++ b/drivers/net/ethernet/ti/cpsw_switchdev.c

[ ... ]

> @@ -393,12 +395,20 @@ static void cpsw_switchdev_event_work(struct work_struct *work)
>  
>  		if (!fdb->added_by_user || fdb->is_local)
>  			break;
> +
> +		ret = pm_runtime_resume_and_get(cpsw->dev);
> +		if (ret < 0) {
> +			dev_err(cpsw->dev, "%s: failed to resume device: %d\n", __func__, ret);
> +			break;
> +		}

[Severity: Medium]
This is a pre-existing issue, and it is not related to this change.
cpsw_remove() in cpsw_new.c and am65_cpsw_nuss_remove() in
am65-cpsw-nuss.c both call pm_runtime_resume_and_get() and return early
if it fails:

am65_cpsw_nuss_remove()
	ret = pm_runtime_resume_and_get(&pdev->dev);
	if (ret < 0) {
		/* Note, if this error path is taken, we're leaking some
		 * resources.
		 */
		...
		return;
	}

That return happens before am65_cpsw_unregister_notifiers() or
cpsw_unregister_notifiers(), and before the netdev and devlink
unregistration. If the module is unloaded after that, do the switchdev
and netdevice notifiers stay registered, pointing into freed module text?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001090820.1605711-1-danishanwar%40ti.com