Re: [PATCH net] dpaa2-switch: fix VLAN upper check not rejecting bridge join

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
dpaa2-switch: fix VLAN upper check not rejecting bridge join

The blamed commit refactored the prechangeupper event handling but
failed to actually return an error in case
dpaa2_switch_prevent_bridging_with_8021q_upper() detected a 802.1q upper
on a port which tries to join a bridge. Fix this by returning err
instead of 0.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index 52c1cb9cb7e02..46ae81c2fa013 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -2177,7 +2177,7 @@ dpaa2_switch_prechangeupper_sanity_checks(struct net_device *netdev,
>  	if (err) {
>  		NL_SET_ERR_MSG_MOD(extack,
>  				   "Cannot join a bridge while VLAN uppers are present");
> -		return 0;
> +		return err;

[Severity: High]
If a user adds an 802.1q VLAN upper device to a port after it has already
successfully joined a bridge, won't this check trigger when the port leaves
the bridge?

In drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c,
dpaa2_switch_prechangeupper_sanity_checks() is called for both bridge joins
and unlinks from dpaa2_switch_port_prechangeupper():

    err = dpaa2_switch_prechangeupper_sanity_checks(netdev, upper_dev, extack);
    if (err)
        return err;

    if (!info->linking)
        dpaa2_switch_port_pre_bridge_leave(netdev);

Since returning an error aborts the notifier chain, an unlink notification
would fail this check, preventing dpaa2_switch_port_pre_bridge_leave() from
running. Does this lead to state inconsistency and skipped switchdev
unoffloads?

Could this check be guarded so it only runs when info->linking is true?

>  	}
>  
>  	netdev_for_each_lower_dev(upper_dev, other_dev, iter) {