Re: [PATCH net-next v3 09/12] dpaa2-switch: offload FDBs added on an upper bond device

[Jakub Kicinski <kuba@kernel.org>]

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 FDBs added on an upper bond device

This patch adds support for offloading FDB entries added on upper bond
devices.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index fb754dae80697..fadebf6a758d1 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
[ ... ]
> @@ -616,6 +640,91 @@ static int dpaa2_switch_port_fdb_del(struct ethsw_port_priv *port_priv,
>  		return dpaa2_switch_port_fdb_del_mc(port_priv, addr);
>  }
>  
> +static struct dpaa2_mac_addr *
> +dpaa2_switch_mac_addr_find(struct list_head *addr_list,
> +			   const unsigned char *addr, u16 vid)
> +{
> +	struct dpaa2_mac_addr *a;
> +
> +	list_for_each_entry(a, addr_list, list)
> +		if (ether_addr_equal(a->addr, addr) && a->vid == vid)
> +			return a;
> +
> +	return NULL;
> +}
> +
> +static int dpaa2_switch_lag_fdb_add(struct dpaa2_switch_lag *lag,
> +				    const unsigned char *addr, u16 vid)
> +{
> +	struct ethsw_port_priv *port_priv;
> +	struct dpaa2_mac_addr *a;
> +	int err = 0;
> +
> +	mutex_lock(&lag->fdb_lock);
> +
> +	a = dpaa2_switch_mac_addr_find(&lag->fdbs, addr, vid);
> +	if (a) {
> +		refcount_inc(&a->refcount);

[Severity: High]
If a port leaves the LAG, the fan-out for the FDB delete event will only
reach the remaining ports. Could this result in the refcount decrementing
fewer times than it incremented, preventing it from reaching zero and
permanently leaking the struct dpaa2_mac_addr?

> +		goto out;
> +	}
> +
> +	port_priv = dpaa2_switch_lag_get_primary(lag);

[Severity: High]
The FDB entry is programmed into hardware using this primary port's physical
interface ID. If this primary port leaves the LAG, is there a mechanism to
migrate the existing FDB entries to one of the remaining ports? Without
migration, could traffic be misrouted, and would subsequent deletions fail
because they target the new primary port's ID?

> +	if (!port_priv) {
> +		err = -ENOENT;
> +		goto out;
> +	}
> +
> +	a = kzalloc(sizeof(*a), GFP_KERNEL);
[ ... ]
> @@ -2856,32 +2996,46 @@ struct ethsw_switchdev_event_work {
>  	struct work_struct work;
>  	struct switchdev_notifier_fdb_info fdb_info;
>  	struct net_device *dev;
> +	struct net_device *orig_dev;
>  	unsigned long event;
> +	u16 vid;
>  };
>  
>  static void dpaa2_switch_event_work(struct work_struct *work)
>  {
>  	struct ethsw_switchdev_event_work *switchdev_work =
>  		container_of(work, struct ethsw_switchdev_event_work, work);
> +	struct net_device *orig_dev = switchdev_work->orig_dev;
>  	struct net_device *dev = switchdev_work->dev;
> +	struct ethsw_port_priv *port_priv = netdev_priv(dev);
>  	struct switchdev_notifier_fdb_info *fdb_info;
> +	struct dpaa2_switch_lag *lag;
>  	int err;
>  
>  	rtnl_lock();
>  	fdb_info = &switchdev_work->fdb_info;
> +	lag = rtnl_dereference(port_priv->lag);
>  
>  	switch (switchdev_work->event) {
>  	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> -		err = dpaa2_switch_port_fdb_add(netdev_priv(dev),
> -						fdb_info->addr);
> +		if (lag)
> +			err = dpaa2_switch_lag_fdb_add(lag, fdb_info->addr,
> +						       switchdev_work->vid);
> +		else
> +			err = dpaa2_switch_port_fdb_add(netdev_priv(dev),
> +							fdb_info->addr);
>  		if (err)
>  			break;
>  		fdb_info->offloaded = true;
> -		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, orig_dev,
>  					 &fdb_info->info, NULL);
>  		break;
>  	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> -		dpaa2_switch_port_fdb_del(netdev_priv(dev), fdb_info->addr);
> +		if (lag)
> +			dpaa2_switch_lag_fdb_del(lag, fdb_info->addr,
> +						 switchdev_work->vid);
> +		else
> +			dpaa2_switch_port_fdb_del(port_priv, fdb_info->addr);

[Severity: High]
Is it possible for a port to be removed from a bond between the queuing of
this event and its execution? If port_priv->lag becomes NULL during this
window, the worker would route the deletion to the standalone path instead
of the LAG path. Could this bypass the LAG refcount decrement and leak the
MAC address?

>  		break;
>  	}
[ ... ]
> @@ -3903,8 +4073,10 @@ static void dpaa2_switch_remove(struct fsl_mc_device *sw_dev)
>  	for (i = 0; i < DPAA2_SWITCH_RX_NUM_FQS; i++)
>  		netif_napi_del(&ethsw->fq[i].napi);
>  
> -	for (i = 0; i < ethsw->sw_attr.num_ifs; i++)
> +	for (i = 0; i < ethsw->sw_attr.num_ifs; i++) {
>  		dpaa2_switch_remove_port(ethsw, i);
> +		mutex_destroy(&ethsw->lags[i].fdb_lock);
> +	}

[Severity: High]
Are the dynamically allocated struct dpaa2_mac_addr entries in the lag->fdbs
list freed during driver teardown? It appears ethsw->lags is freed shortly
after this without iterating through the fdbs lists, which might cause a
memory leak of all active LAG FDB entries. Could this also happen if all
ports leave a LAG and it becomes unused, inheriting stale entries upon reuse?

>  
>  	kfree(ethsw->fdbs);
>  	kfree(ethsw->filter_blocks);