Re: [PATCH net v2] 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 2 (1) · 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).

- [Medium] The patch removes the `!fdb->added_by_user` gate from the
  SWITCHDEV_FDB_DEL_TO_DEVICE branch of am65_cpsw_switchdev_event_work().
- [Medium] Before this patch, dynamic DEL_TO_DEVICE events hit the
  `!fdb->added_by_user` break and never touched hardware.

Pre-existing issues:
- [High] cpsw_ale has no internal lock (no spinlock, mutex or lockdep
  assert in cpsw_ale.c).
- [Medium] This problem wasn't introduced by this patch, but the older
  CPSW switchdev driver has the same flush defect.

--

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

This patch drops the added_by_user gate on SWITCHDEV_FDB_DEL_TO_DEVICE in
am65-cpsw-switchdev, so dynamic FDB deletes now reach the hardware. It adds
cpsw_ale_del_ucast_dynamic_by_port(), which walks the ALE and frees
non-persistent unicast rows that match (addr, port). When vid is 0 it frees
matching rows in every vid.

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> index 53cdac272b58..0dc681748b0e 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
> @@ -397,13 +397,19 @@ 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]

With the added_by_user gate removed, does routine bridge software ageing
now remove live ALE rows as well?

br_fdb_cleanup() ages out dynamic entries like this:

br_fdb_cleanup()
  fdb_delete(br, f, true)
    fdb_notify(br, f, RTM_DELNEIGH, true)
      br_switchdev_fdb_notify()
        -> SWITCHDEV_FDB_DEL_TO_DEVICE, added_by_user = 0

This driver never reports hardware-learned entries to the bridge (there is
no SWITCHDEV_FDB_ADD_TO_BRIDGE). Also, am65_cpsw_port_attr_set() does not
handle SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME. As a result, the bridge only
refreshes an entry when a frame from that station reaches the CPU.

Take a station whose unicast traffic is switched only in hardware. Its
bridge entry expires after the default 300 seconds, while traffic keeps
refreshing its ALE row. The ALE ages out untouched rows after about 30
seconds (AM65_CPSW_ALE_AGEOUT_DEFAULT with CPSW_ALE_F_HW_AUTOAGING), so any
row still present at that point is in use.

cpsw_ale_del_ucast_dynamic_by_port() does not check touched/untouched
state. With vid == 0 it frees every matching row for that MAC on the port.

Would traffic to that station then be flooded as unknown unicast until the
station transmits again? That would happen once per bridge ageing interval,
with a full ALE walk each time.

The commit message only describes this as a flush fix. Should it mention
the change in ageing behaviour, or should the ageing path be filtered out?

>  		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);
> +		if (!fdb->added_by_user)
> +			cpsw_ale_del_ucast_dynamic_by_port(cpsw->ale,
> +							   (u8 *)fdb->addr,
> +							   port_id,
> +							   fdb->vid);

[Severity: Medium]

Before this patch, dynamic DEL_TO_DEVICE events hit the break before any
hardware access. Now each one does a full ALE MMIO walk. Does this path
need a runtime PM reference?

On port-down the sequence looks like this:

am65_cpsw_nuss_ndo_slave_stop()
  am65_cpsw_nuss_common_stop()
    cpsw_ale_stop()
  pm_runtime_put(common->dev)

NETDEV_DOWN then triggers:

br_stp_disable_port()
  br_fdb_delete_by_port()
    -> one SWITCHDEV_FDB_DEL_TO_DEVICE per dynamic entry, added_by_user = 0

am65_cpsw_switchdev_event_work()
  cpsw_ale_del_ucast_dynamic_by_port()
    cpsw_ale_read()   /* writel_relaxed/readl_relaxed of ALE regs */

When the last port goes down, these work items access the ALE registers
after the last runtime PM reference has been dropped. Other ALE users in
am65-cpsw-nuss.c take pm_runtime_resume_and_get() first, for example the
VLAN add/kill and set_mac_address paths.

With CONFIG_TI_K3_AM65_CPTS disabled, am65_cpsw_init_cpts() returns before
it calls pm_runtime_forbid(), so the device can runtime suspend. The comment
there notes that K3 CPSWxG modules may lose context on ON->OFF transitions.

Could this access a suspended module?

> +		else
> +			cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port_id,
> +					   fdb->vid ? ALE_VLAN : 0, fdb->vid);
>  		break;

[Severity: Medium]

This is a pre-existing issue that was not introduced by this patch. The
older CPSW switchdev driver seems to have the same flush problem.
cpsw_switchdev_event_work() in drivers/net/ethernet/ti/cpsw_switchdev.c
still has:

	if (!fdb->added_by_user || fdb->is_local)
		break;
	...
	cpsw_ale_del_ucast(cpsw->ale, (u8 *)fdb->addr, port,
			   fdb->vid ? ALE_VLAN : 0, fdb->vid);

cpsw_init_host_port() in cpsw_new.c enables ALE_VLAN_AWARE, so the nonzero
vid mismatch described in the commit message applies there too.
cpsw_switchdev_event() queues DEL_TO_DEVICE work without any filtering.

Should cpsw_switchdev.c also use the new
cpsw_ale_del_ucast_dynamic_by_port() helper, or is that planned as a
separate fix?

> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index e202bba49480..86bcea5744e0 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c

[ ... ]

> @@ -441,6 +441,48 @@ static int cpsw_ale_find_ageable(struct cpsw_ale *ale)
>  	return -ENOENT;
>  }
>  
> +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++) {
> +		cpsw_ale_read(ale, idx, ale_entry);

[ ... ]

> +		memset(ale_entry, 0, sizeof(ale_entry));
> +		cpsw_ale_set_entry_type(ale_entry, ALE_TYPE_FREE);
> +		cpsw_ale_write(ale, idx, ale_entry);

[Severity: High]

This is a pre-existing issue: the existing switchdev ucast add/del and VLAN
ops can already race here. This patch adds a new full-table read/write
walker and runs it much more often.

ALE table accesses go through a shared indirect window: the index goes into
ALE_TABLE_CONTROL, then the ALE_TABLE words are read or written.
cpsw_ale.c has no internal lock. The only thing serializing this loop is
rtnl_lock() taken by the switchdev work.

am65_cpsw_nuss_ndo_slave_set_rx_mode() is called on the legacy
ndo_set_rx_mode path in __dev_set_rx_mode(). It runs under
netif_addr_lock_bh() without rtnl. It calls cpsw_ale_flush_multicast(),
which reads every ALE_TYPE_ADDR/ALE_TYPE_VLAN_ADDR row, including unicast
rows, and always writes it back:

	for (idx = 0; idx < ale->params.ale_entries; idx++) {
		cpsw_ale_read(ale, idx, ale_entry);
		...
		cpsw_ale_write(ale, idx, ale_entry);
	}

An unprivileged user can reach that path without rtnl:

setsockopt(IPV6_ADD_MEMBERSHIP)
  __ipv6_sock_mc_join()
    __ipv6_dev_mc_inc()
      igmp6_group_added()
        dev_mc_add()
          __dev_mc_add()
            __dev_set_rx_mode()
              am65_cpsw_nuss_ndo_slave_set_rx_mode()
                cpsw_ale_flush_multicast()

Suppose cpsw_ale_flush_multicast() reads row N, this function then frees
row N, and cpsw_ale_flush_multicast() writes its stale copy back. Doesn't
the deleted dynamic entry come back?

The ALE_TABLE_CONTROL writes from the two paths could also interleave.
Could one path then read another row's contents, or write its words to the
wrong index? That would free or overwrite an unrelated VLAN, persistent or
multicast row.

Should ALE table accesses be serialized by a lock inside cpsw_ale?

> +		deleted++;
> +
> +		if (vid)
> +			return 0;
> +	}
> +
> +	return deleted ? 0 : -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(cpsw_ale_del_ucast_dynamic_by_port);

[ ... ]

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