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

[Ioana Ciornei]

On Wed, May 06, 2026 at 06:15:36PM +0300, Ioana Ciornei wrote:
> This patch adds support for offloading FDB entries added on upper bond
> devices.
> 
> First of all, the call to switchdev_bridge_port_offload() is updated so
> that the notifier blocks needed for FDB events replay are available to
> the bridge core.
> 
> Using switchdev_handle_*() helpers is also necessary because each FDB
> event needs to be fanned out to any DPAA2 switch lower device. This
> triggers another change in the return type used by the
> dpaa2_switch_port_fdb_event() - from notifier types to regular errno
> types.
> 
> Handling of the SWITCHDEV_FDB_ADD_TO_DEVICE/SWITCHDEV_FDB_DEL_TO_DEVICE
> events is updated so that the newly dpaa2_switch_lag_fdb_add() /
> dpaa2_switch_lag_fdb_del() functions are called anytime a port is under
> a bond device. This will allow us to manage refcounting on FDB entries
> which are added on the upper bond devices.
> 
> The DPAA2 switch uses shared-VLAN learning which means that the vid
> parameter is not used when adding an FDB entry to HW. The current
> behavior when dealing with FDB entries with the same MAC address but
> different VLANs is to add the entry to HW every time while removal will
> get done on the first 'bridge fdb del' command issued by the user.
> 
> The same behavior is kept also for FDBs added on bond devices by keeping
> the refcount on the {vid, addr} pair while the HW operation disregards
> entirely the vid parameter.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> ---
>  .../ethernet/freescale/dpaa2/dpaa2-switch.c   | 190 ++++++++++++++++--
>  .../ethernet/freescale/dpaa2/dpaa2-switch.h   |  22 ++
>  2 files changed, 193 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index bced37335a9e..cce0af47ca07 100644

(...)

> +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);
> +		goto out;
> +	}

Sashiko noted:

	Could this refcounting logic cause FDB entries to leak when
	ports are removed from a LAG?

	When an FDB entry is added to a bond,
	switchdev_handle_fdb_event_to_device() fans out the
	SWITCHDEV_FDB_ADD_TO_DEVICE event to all N lower switch ports,
	which calls dpaa2_switch_lag_fdb_add() N times and sets the
	refcount to N.

	If a port leaves the bond, switchdev does not emit delete events
	for existing FDBs, so the refcount remains at N. If the FDB
	entry is subsequently deleted from the bond, the delete event is
	fanned out to only the remaining N-1 ports. This would drop the
	refcount to 1 instead of 0, meaning the MAC is never removed
	from hardware and the software entry is permanently leaked.

I forgot to pass the proper context and the notifier blocks to the
switchdev_bridge_port_unoffload() call. I think this will fix the issue
raised above.

> +static bool dpaa2_switch_foreign_dev_check(const struct net_device *dev,
> +					   const struct net_device *foreign_dev)
> +{
> +	struct ethsw_port_priv *port_priv = netdev_priv(dev);
> +
> +	if (netif_is_bridge_master(foreign_dev))
> +		if (port_priv->fdb->bridge_dev == foreign_dev)
> +			return false;
> +
> +	if (netif_is_bridge_port(foreign_dev))
> +		return !dpaa2_switch_port_offloads_bridge_port(port_priv,
> +							       foreign_dev);
> +
> +	return true;
> +}

Sashiko notes:

	Does this change inadvertently break FDB offloading for
	standalone ports?

	If a user adds an FDB entry directly to a standalone switch port
	or a standalone bond, orig_dev is the standalone interface
	itself, which is neither a bridge master nor a bridge port.

	Because of this, dpaa2_switch_foreign_dev_check() will evaluate
	to true and cause dpaa2_switch_port_fdb_event() to return 0,
	silently ignoring the FDB addition. Prior to this patch, these
	entries were accepted.

No, there is no such think as a standalone switch port or a standalone
bond that also somehow handle switchdev events.

>  	INIT_WORK(&switchdev_work->work, dpaa2_switch_event_work);
>  	switchdev_work->dev = dev;
>  	switchdev_work->event = event;
> +	switchdev_work->orig_dev = orig_dev;
> +	switchdev_work->vid = fdb_info->vid;

Sashiko notes:

	Does orig_dev need a reference count here to prevent a
	use-after-free?

	Since the pointer to orig_dev is stored for the asynchronous
	switchdev work, and the work queue later uses it in
	call_switchdev_notifiers(), could the network stack unregister
	and free the upper bond or bridge device before
	dpaa2_switch_event_work() executes?

	If so, taking a reference with dev_hold(orig_dev) and releasing
	it with dev_put(orig_dev) in the worker might be necessary.

Sure, will fix it.

(...)

> @@ -3917,6 +4067,8 @@ static int dpaa2_switch_probe(struct fsl_mc_device *sw_dev)
>  		ethsw->lags[i].ethsw = ethsw;
>  		ethsw->lags[i].id = i + 1;
>  		ethsw->lags[i].in_use = 0;
> +		mutex_init(&ethsw->lags[i].fdb_lock);
> +		INIT_LIST_HEAD(&ethsw->lags[i].fdbs);
>  	}

Sashiko notes:

	Will these FDB lists be properly cleaned up when a LAG is
	destroyed?

	When the last port leaves a bond device, the LAG is marked as
	unused (lag->in_use = false), but it doesn't appear that the
	driver iterates over lag->fdbs to free the remaining FDB
	entries, which could cause a memory leak.

I think switchdev will remove the FDB entries from the ports any time a
bridge port is unoffloaded.