Re: [PATCH net-next v3 10/12] dpaa2-switch: offload port objects on an upper bond device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpaa2-switch: offload port objects on an upper bond device

This patch adds support for offloading port objects, VLANs and MDBs,
added on upper bond devices. It introduces switchdev_handle_*() replication
helpers for port objects and a refcount mechanism for handling MDBs in bond
devices.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index fadebf6a758d1..9aba5f348abc0 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -2096,30 +2096,46 @@ static int dpaa2_switch_port_mdb_add(struct net_device *netdev,
>  				     const struct switchdev_obj_port_mdb *mdb)
>  {
>  	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> +	struct dpaa2_switch_lag *lag;
>  	int err;
>  
>  	/* Check if address is already set on this port */
>  	if (dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr))
>  		return -EEXIST;

[Severity: High]
Does this incorrectly block valid MDB additions for the same MAC address but
different VLAN IDs?

Looking at dpaa2_switch_port_lookup_address(), it checks the VLAN-unaware
netdev->mc list. If an MDB with the same MAC but different VID is added,
it returns -EEXIST.

Later, if the user attempts to delete the un-added MDB,
dpaa2_switch_port_mdb_del() finds the MAC from the first MDB, skips hardware
removal due to VID mismatch, and successfully calls dev_mc_del(). 

When the legitimate MDB is later deleted, the lookup fails with -ENOENT,
skipping dpaa2_switch_lag_fdb_del() and permanently leaking the dynamically
allocated struct dpaa2_mac_addr and the hardware FDB entry.

[ ... ]

> @@ -2215,14 +2231,17 @@ static int dpaa2_switch_port_mdb_del(struct net_device *netdev,
>  				     const struct switchdev_obj_port_mdb *mdb)
>  {
>  	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> +	struct dpaa2_switch_lag *lag;
>  	int err;
>  
>  	if (!dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr))
>  		return -ENOENT;

[Severity: High]
Does returning an error here abort the switchdev_handle_port_obj_del()
replication loop for the remaining bond slaves?

This patch uses switchdev_handle_port_obj_del() to replicate object
deletions across all slave devices. If a driver callback returns an error
other than -EOPNOTSUPP on one of the slaves, the replication loop aborts,
skipping all remaining slaves.

For example, dpaa2_switch_port_mdb_del() returns -ENOENT if the MAC is not
found, and dpaa2_switch_port_vlans_del() propagates hardware deletion errors.

Will this leave the remaining slave ports with stale VLANs and MDBs fully
active in software and hardware, causing silent data leaks and inconsistent
state?

[ ... ]

> @@ -3128,37 +3154,23 @@ static int dpaa2_switch_port_event(struct notifier_block *nb,
>  	}
>  }
>  
> -static int dpaa2_switch_port_obj_event(unsigned long event,
[ ... ]
>  static int dpaa2_switch_port_blocking_event(struct notifier_block *nb,
>  					    unsigned long event, void *ptr)
>  {
>  	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> +	int err;
>  
>  	switch (event) {
>  	case SWITCHDEV_PORT_OBJ_ADD:
> +		err = switchdev_handle_port_obj_add(dev, ptr,
> +						    dpaa2_switch_port_dev_check,
> +						    dpaa2_switch_port_obj_add);
> +		return notifier_from_errno(err);

[Severity: High]
Can this code leak memory and hardware MDBs when a slave leaves a bond?

When switchdev_handle_port_obj_add() replicates MDB additions across all
bond slaves, it increments the a->refcount in lag->fdbs for each slave. 

If a slave leaves the bond (e.g., via ip link set eth1 nomaster), shouldn't
dpaa2_switch_port_bond_leave() be updated to decrement the slave's share
of the MDB refcounts?

Without this, when the bond is later removed from the bridge,
SWITCHDEV_PORT_OBJ_DEL replicates only over the remaining slaves. The MDB
refcounts will not reach zero because the decrements from the departed
slaves are permanently lost. This would leak struct dpaa2_mac_addr memory
and leave the hardware MDB permanently programmed on the LAG.