Re: [PATCH net-next] net: dsa: felix: validate switch in .netdev_to_port() conversion

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@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.
Full review at: https://sashiko.dev
---
net: dsa: felix: validate switch in .netdev_to_port() conversion

This commit changes the .netdev_to_port() callback to take a struct
ocelot pointer and adds a dp->ds mismatch check in felix_netdev_to_port()
so that a netdev belonging to a different switch is rejected before the
port index is consumed by flower rule parsing.

> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 1b8269320464..d2e96d065dc4 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -984,7 +984,7 @@ static bool ocelot_netdevice_dev_check(const struct net_device *dev)
>  	return dev->netdev_ops == &ocelot_port_netdev_ops;
>  }
>  
> -int ocelot_netdev_to_port(struct net_device *dev)
> +int ocelot_netdev_to_port(struct ocelot *ocelot, struct net_device *dev)
>  {
>  	struct ocelot_port_private *priv;

[Medium]
The new ocelot parameter is added here to match the updated
ocelot_ops::netdev_to_port signature, but is it actually used in the
body of ocelot_netdev_to_port()?

The function still does only:

    if (!dev || !ocelot_netdevice_dev_check(dev))
            return -EINVAL;

    priv = netdev_priv(dev);

    return priv->port.index;

ocelot_netdevice_dev_check() compares dev->netdev_ops against the
file-scope ocelot_port_netdev_ops vtable, which is shared across all
ocelot instances, so a netdev belonging to a sibling ocelot switch
passes this check and priv->port.index is returned as a port number
on the local hardware.

Isn't this the same silent wrong-port resolution the commit message
describes for the felix path?

The corresponding switch-ownership check the commit adds for felix:

    if (IS_ERR(dp) || dp->ds != ds)
            return -EINVAL;

has no analogue in the ocelot variant. struct ocelot_port carries a
back-pointer to its owning struct ocelot (used elsewhere in this
file), so a symmetric check such as:

    if (priv->port.ocelot != ocelot)
            return -EINVAL;

after the netdev_ops check would mirror the felix change and consume
the otherwise unused ocelot argument. Without it, the parameter is
dead and the validation promised by the commit message is only
half-applied.

The two call sites in ocelot_flower.c, ocelot_flower_parse_egress_port()
consuming a->dev and ocelot_flower_parse_indev() resolving a netdev
from a user-supplied ifindex via __dev_get_by_index(), both feed
externally-controlled netdevs into ocelot->ops->netdev_to_port(ocelot, dev),
so the standalone ocelot path used by ocelot_vsc7514 still accepts a
foreign switch's netdev when more than one ocelot instance is present.