Re: [PATCH net-next v3 02/12] dpaa2-switch: add support for LAG offload

[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: add support for LAG offload

This patch adds the bulk of the changes needed in order to support
offloading of an upper bond device.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index a0bf5b50aae5f..d082ecf9b1252 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c

[ ... ]

> @@ -2210,19 +2300,275 @@ dpaa2_switch_prechangeupper_sanity_checks(struct net_device *netdev,
> +static int dpaa2_switch_port_bond_join(struct net_device *netdev,
> +				       struct net_device *bond_dev,
> +				       struct netdev_lag_upper_info *info,
> +				       struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	/* If the bond device is a switch port, join the bridge as well */
> +	bridge_dev = netdev_master_upper_dev_get(bond_dev);
> +	if (!bridge_dev || !netif_is_bridge_master(bridge_dev))
> +		return 0;
> +
> +	err = dpaa2_switch_port_bridge_join(netdev, bridge_dev, extack);

[Severity: High]
Should switchdev_bridge_port_offload() inside dpaa2_switch_port_bridge_join()
receive the actual bridge port (the bond device) rather than the lower
physical port?

For ports enslaved to a LAG, passing the lower physical port might cause
switchdev to immediately abort the offload with -EINVAL, since the netdev
is not directly enslaved to the bridge. I see this was addressed in a later
commit adding a dpaa2_switch_port_to_bridge_port() helper.

[Severity: High]
When a port joins a LAG that is already enslaved to a bridge, is there a risk
of dpaa2_switch_port_set_fdb() corrupting the in_use flag of active FDBs?

Since the port first acquires the bond's shared FDB, and then
dpaa2_switch_port_bridge_join() finds a port already using that same FDB,
the code in dpaa2_switch_port_set_fdb() seems to lack a self-assignment
check. This could mark the shared FDB as unused before reassigning it,
allowing another standalone port to reallocate it. I note this was fixed in
a later commit covering bond scenarios.

[Severity: High]
Will dpaa2_switch_port_set_fdb() skip bonds when iterating through lower
devices, breaking cross-LAG forwarding?

When a port joins a bridge, dpaa2_switch_port_set_fdb() uses a non-recursive
iterator (netdev_for_each_lower_dev). If the other lower device is a bond,
dpaa2_switch_port_dev_check() will return false, skipping nested ports and
failing to share the hardware FDB. I note this was also fixed in a later
commit.

> +	if (err)
> +		goto err_bridge_join;
> +
> +	return err;
> +
> +err_bridge_join:
> +	dpaa2_switch_port_del_vlan(port_priv, DEFAULT_VLAN_ID);
> +err_vlan_add:
> +err_lag_cfg:
> +	rcu_assign_pointer(port_priv->lag, NULL);
> +	dpaa2_switch_set_lag_cfg(bond_dev, lag_id, ethsw);
> +err_egress_flood:
> +	if (port_priv->fdb != old_fdb) {
> +		new_fdb = port_priv->fdb;
> +
> +		/* Explicitly move the port back into the old private FDB */
> +		old_fdb->in_use = true;
> +		old_fdb->bridge_dev = NULL;
> +		port_priv->fdb = old_fdb;
> +
> +		/* Reprogram the bond FDB to no longer include this port */
> +		dpaa2_switch_fdb_set_egress_flood(ethsw, new_fdb->fdb_id);

[Severity: High]
Are we permanently leaking a hardware FDB resource in this error path?

If dpaa2_switch_port_bridge_join() fails, it will have allocated a new
intermediate FDB and marked it in_use. The cleanup block here restores
the original old_fdb, but does not appear to set new_fdb->in_use back to
false. Repeated failures might exhaust the fixed number of FDBs.

> +	} else {
> +		/* Same FDB means that this was the first port joining the
> +		 * bond. Keep using the same FDB.
> +		 */
> +		old_fdb->bridge_dev = NULL;
> +	}

[ ... ]

> +static int dpaa2_switch_port_bond_leave(struct net_device *netdev,
> +					struct net_device *bond_dev)
> +{

[ ... ]

> +lag_cleanup:
> +	/* Recreate the LAG configuration for the LAG group that we left. In
> +	 * case any step failed, at least we free up a LAG resource.
> +	 */
> +	rcu_assign_pointer(port_priv->lag, NULL);
> +	dpaa2_switch_set_lag_cfg(bond_dev, lag->id, ethsw);

[Severity: Critical]
Could this trigger a kernel panic if lag is NULL on the rollback path?

If dpaa2_switch_port_bond_join() fails and sets port_priv->lag to NULL, a
subsequent NETDEV_CHANGEUPPER rollback calls this function. The lag variable
is initialized via rtnl_dereference(port_priv->lag), which would be NULL,
leading to a dereference when accessing lag->id here.

> +
> +	return err;
> +}
> +
>  static int dpaa2_switch_port_prechangeupper(struct net_device *netdev,
>  					    struct netdev_notifier_changeupper_info *info)
>  {
> +	struct net_device *upper_dev, *br;
>  	struct netlink_ext_ack *extack;
> -	struct net_device *upper_dev;
>  	int err;
>  
>  	if (!dpaa2_switch_port_dev_check(netdev))
> @@ -2239,6 +2585,24 @@ static int dpaa2_switch_port_prechangeupper(struct net_device *netdev,
>  
>  		if (!info->linking)
>  			dpaa2_switch_port_pre_bridge_leave(netdev);
> +	} else if (netif_is_lag_master(upper_dev)) {
> +		if (!info->linking) {
> +			if (netif_is_bridge_port(upper_dev))
> +				dpaa2_switch_port_pre_bridge_leave(netdev);
> +			return 0;
> +		}
> +
> +		if (netif_is_bridge_port(upper_dev)) {
> +			br = netdev_master_upper_dev_get(upper_dev);
> +			err = dpaa2_switch_prechangeupper_sanity_checks(netdev,
> +									br,
> +									extack);
> +			if (err)
> +				return err;
> +		}
> +
> +		return dpaa2_switch_pre_lag_join(netdev, upper_dev,
> +						 info->upper_info, extack);
>  	}
>  
>  	return 0;

[Severity: High]
Might this bypass topological checks and allow VLAN uppers on offloaded LAGs?

If a port joins a LAG first, and then a VLAN is added to the LAG, the
event propagates here with the VLAN device as upper_dev. Since it is
neither a bridge nor a LAG master, this function returns 0 without checking
vlan_uses_dev(), inadvertently allowing an unsupported hardware configuration.

> @@ -2263,6 +2627,80 @@ static int dpaa2_switch_port_changeupper(struct net_device *netdev,
>  							     extack);
>  		else
>  			return dpaa2_switch_port_bridge_leave(netdev);

[Severity: High]
Does calling dpaa2_switch_port_bridge_leave() wrongly split LAG member ports
into isolated private FDBs?

When a LAG leaves a bridge, this is invoked for each member port. It will
unconditionally call dpaa2_switch_port_set_fdb(port_priv, NULL), which forces
the port to drop the shared FDB and isolates it from the other LAG members.
I note this was also fixed in a later commit covering bond scenarios.

> +	} else if (netif_is_lag_master(upper_dev)) {
> +		if (info->linking)
> +			return dpaa2_switch_port_bond_join(netdev, upper_dev,
> +							   info->upper_info,
> +							   extack);