Re: [PATCH net-next v3 04/12] dpaa2-switch: extend dpaa2_switch_port_set_fdb() to cover bond scenarios

[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: extend dpaa2_switch_port_set_fdb() to cover bond scenarios

The dpaa2_switch_port_set_fdb() function is responsible with determining
what FDB should be used by a port as a consequence of changing its upper
device. This patch extends the function to also cover the circumstances
in which a DPAA2 switch port offloads a bond device.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index 8026a50141055..b851376b8e1b8 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -68,15 +68,45 @@ static void dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv,

[ ... ]

> -		/* Since we are not the last port which leaves a bridge,
> -		 * acquire a new FDB and use it. The number of FDBs is sized to
> -		 * accommodate all switch ports as standalone, each with its
> -		 * private FDB, which means that dpaa2_switch_fdb_get_unused()
> -		 * must succeed here. WARN if not.
> -		 */
> -		fdb = dpaa2_switch_fdb_get_unused(port_priv->ethsw_data);
> -		if (WARN_ON(!fdb))
> -			return;
> +		if (fdb) {
> +			/* Switching to the FDB of another port which is under
> +			 * the same bond device. Release the previous FDB in
> +			 * case we were the last to use it.
> +			 */
> +			if (last_fdb_user) {
> +				port_priv->fdb->in_use = false;
> +				port_priv->fdb->bridge_dev = NULL;
> +			}
> +			port_priv->fdb = fdb;
> +		} else if (last_fdb_user) {
> +			/* No other bond lowers to share the FDB with and we
> +			 * are its last user, just keep it.
> +			 */
> +		} else {
> +			fdb = dpaa2_switch_fdb_get_unused(port_priv->ethsw_data);
> +			if (WARN_ON(!fdb))
> +				return;
>  
> -		port_priv->fdb = fdb;
> -		port_priv->fdb->in_use = true;
> -		port_priv->fdb->bridge_dev = NULL;
> +			port_priv->fdb = fdb;
> +			port_priv->fdb->in_use = true;
>  		}

[Severity: High]
Does this code leak a hardware FDB structure on the error recovery path of
dpaa2_switch_port_bond_join()?

If dpaa2_switch_port_bridge_join() fails inside dpaa2_switch_port_bond_join(),
the error recovery calls dpaa2_switch_port_set_fdb(port_priv, upper_dev, false).

Since the port is not the last user of the shared bridge FDB, this dynamically
allocates a new, unused FDB via dpaa2_switch_fdb_get_unused() and marks it
in_use = true.

Control then returns to dpaa2_switch_port_bond_join(), which continues its own
error recovery and explicitly reverts port_priv->fdb back to its original
pre-bond private FDB (old_fdb):

    if (port_priv->fdb != old_fdb) {
        new_fdb = port_priv->fdb;
        old_fdb->in_use = true;
        old_fdb->bridge_dev = NULL;
        port_priv->fdb = old_fdb;
        ...
    }

However, the newly allocated FDB (new_fdb) from the bridge_join failure
path is ignored and left marked as in_use = true. Because this FDB is no
longer referenced by any port, it seems to be permanently leaked.

Could this deplete the highly limited pool of hardware FDB slots, eventually
preventing any new standalone ports from functioning?

[ ... ]

>  	/* The current port is about to change its FDB to the one used by the
> @@ -143,7 +203,8 @@ static void dpaa2_switch_port_set_fdb(struct ethsw_port_priv *port_priv,
>  	}
>  
>  	/* Keep track of the new upper bridge device */
> -	port_priv->fdb->bridge_dev = upper_dev;
> +	if (netif_is_bridge_master(upper_dev))
> +		port_priv->fdb->bridge_dev = upper_dev;
>  }

[Severity: Critical]
Does this prematurely mark actively shared FDBs as unused?

When a port joins a bond that is already in a bridge alongside another
standalone port:

1. dpaa2_switch_port_bond_join() first gives the port the bond's shared FDB
   (which is the bridge FDB).
2. Then it calls dpaa2_switch_port_bridge_join(), which calls
   dpaa2_switch_port_set_fdb(..., true).
3. This searches the bridge's lowers and finds the standalone port, skipping
   the bond's ports.

Because both the bond and the standalone port are in the same bridge, they
share the exact same FDB.

However, the existing code just above this diff:

	if (other_port_priv) {
		port_priv->fdb->in_use = false;
		port_priv->fdb->bridge_dev = NULL;
		port_priv->fdb = other_port_priv->fdb;
	}

executes without checking if port_priv->fdb == other_port_priv->fdb. Since
port_priv->fdb was already the bridge's FDB, this unconditionally marks the
actively shared bridge FDB as unused.

Once the RTNL lock is released, could subsequent calls to
dpaa2_switch_fdb_get_unused() by unrelated ports allocate this actively used
bridge FDB, causing L2 isolation breakdown and silent data corruption across
the entire bridge?