Re: [PATCH net] net/mlx5: Attach late uplink netdev to loaded representors

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] New error-path use-after-free in the attach path.
- [Medium] mlx5_ib_vport_uplink_rep_attach_netdev() always sets the
  uplink IB port's netdev to the rep's own ETH netdev, via…
- [Medium] The attach path now writes the uplink ETH rep's rpriv->netdev
  while the rep is already REP_LOADED and VF reps may be live.
- [Low] mlx5_eswitch_attach_uplink_netdev() runs on every pass of
  mlx5_eswitch_reload_reps_blocked(), not only after…
- [Low] _mlx5e_probe() calls mlx5_core_uplink_netdev_set(mdev, netdev),
  which now queues the reload-reps work.

--

net/mlx5: Attach late uplink netdev to loaded representors

This commit adds an attach_uplink_netdev rep op that runs from the
reload-reps work. It covers the case where the uplink netdev appears only
after the uplink representor was loaded. ETH re-runs the uplink load to
switch profile and set rpriv->netdev, and IB redoes ib_device_set_netdev().

> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> index 65d8767d1830c..f9931a94157aa 100644
> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> @@ -273,10 +273,34 @@ mlx5_ib_vport_rep_unload(struct mlx5_eswitch_rep *rep)
>  	}
>  }
>  
> +static int
> +mlx5_ib_vport_uplink_rep_attach_netdev(struct mlx5_core_dev *mdev,
> +				       struct mlx5_eswitch_rep *rep)
> +{
> +	struct mlx5_ib_dev *dev = mlx5_ib_rep_to_dev(rep);
> +	struct net_device *ndev;
> +	int i;
> +
> +	/* Shared FDB slave uplinks share the master's IB device. */
> +	if (!dev)
> +		return 0;
> +
> +	ndev = mlx5_ib_get_rep_netdev(rep->esw, rep->vport);
> +	if (!ndev)
> +		return -ENODEV;

[Severity: Low]
Should this return -ENODEV in the scenario the patch targets?

mlx5_eswitch_attach_uplink_netdev() runs on every pass of
mlx5_eswitch_reload_reps_blocked(). That work is also queued by
mlx5_eswitch_register_vport_reps_locked() whenever ETH or IB rep ops
register, not only after mlx5_core_uplink_netdev_set() reports a netdev.

Suppose eth is unbound in legacy mode and switchdev is then entered. The
ETH uplink rep is REP_LOADED with rpriv->netdev == NULL. When the IB rep
registers, the reload work loads the IB uplink rep and calls this op.
mlx5_ib_get_rep_netdev()->mlx5_eswitch_get_proto_dev()->
mlx5e_vport_rep_get_proto_dev() returns NULL, and the core logs:

    E-Switch: Failed to attach uplink netdev to rep type 1, err(-19)

This repeats on each reload-reps pass until eth is bound again. Would
returning 0 for a missing netdev be more appropriate here?

The commit message says the op is "run from the existing reload-reps work
once mlx5_core_uplink_netdev_set() reports a netdev". The comment in
struct mlx5_eswitch_rep_ops says something similar. The call in
mlx5_eswitch_reload_reps_blocked() does not depend on that condition,
though.

> +
> +	for (i = 0; i < dev->num_ports; i++)
> +		if (dev->port[i].rep == rep)
> +			return ib_device_set_netdev(&dev->ib_dev, ndev, i + 1);

[Severity: Medium]
Can this overwrite a netdev that LAG picked for this port?

This always sets the uplink IB port to the rep's own ETH netdev and does
not check for an existing association. For SR-IOV active-backup LAG,
lag_event() in drivers/infiniband/hw/mlx5/main.c points the uplink port
at the netdev carried by MLX5_DRIVER_EVENT_ACTIVE_BACKUP_LAG_CHANGE_LOWERSTATE.
That can be the peer's lower netdev:

    old_ndev = ib_device_get_netdev(ibdev, portnum + 1);
    ret = ib_device_set_netdev(ibdev, ndev, portnum + 1);

mlx5_eswitch_attach_uplink_netdev() calls this op for any REP_LOADED
uplink on every reload-reps pass. Those passes are queued at eth probe
and at rep ops registration, and the devl_trylock() retry in
esw_wq_handler() can delay them.

Suppose a pass runs after LAG has picked a different lower netdev. Would
it replace that choice, so RDMA reports the wrong netdev until the next
lower-state event?

> +
> +	return 0;
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> index 88a170e40bd93..19812bf6a8306 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> @@ -1490,10 +1490,24 @@ mlx5e_vport_uplink_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *
>  	rpriv->netdev = netdev;
>  	err = mlx5e_netdev_change_profile(netdev, dev,
>  					  &mlx5e_uplink_rep_profile, rpriv);
> +	if (err)
> +		rpriv->netdev = NULL;

[Severity: High]
Can this leave the netdev pointing at a freed rpriv?

An error from mlx5e_netdev_change_profile() does not always mean the
profile change was rolled back. When mdev->state is
MLX5_DEVICE_STATE_INTERNAL_ERROR:

    if (mdev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR) {
        mlx5e_netdev_init_profile(netdev, mdev, new_profile, new_ppriv);
        set_bit(MLX5E_STATE_DESTROYING, &priv->state);
        return -EIO;
    }

The netdev is left with priv->profile = &mlx5e_uplink_rep_profile and
priv->ppriv = rpriv, while rpriv->netdev is now cleared.

On the path through mlx5e_vport_uplink_rep_attach_netdev(),
mlx5_eswitch_attach_uplink_netdev() only logs the failure. The ETH rep
stays REP_LOADED. On the next unload, such as health recovery removing
the eth-rep aux device, mlx5e_vport_rep_unload() takes this branch:

    if (!netdev) {
        ppriv = rpriv;
        goto free_ppriv;
    }
    ...
free_ppriv:
    kvfree(ppriv); /* mlx5e_rep_priv */

That frees rpriv without calling mlx5e_vport_uplink_rep_unload(), so
mlx5e_netdev_attach_nic_profile() never runs.

On recovery, mlx5e_resume()->_mlx5e_resume()->mlx5e_attach_netdev()
calls profile->init_tx, which is mlx5e_init_rep_tx():

    struct mlx5e_rep_priv *rpriv = priv->ppriv;
    ...
    err = mlx5e_rep_neigh_init(rpriv);
    ...
    if (rpriv->rep->vport == MLX5_VPORT_UPLINK) {

Would this read and write the freed mlx5e_rep_priv? If rpriv->netdev had
stayed set, the unload path would have restored the NIC profile instead.

mlx5_enter_error_state() can set INTERNAL_ERROR asynchronously while the
attach work runs. The load path already had a similar kvfree-after-EIO
pattern, but the new attach caller and the new NULL assignment add
another way to reach it.

>  	mlx5_uplink_netdev_put(dev, netdev);
>  	return err;
>  }
>  
> +static int
> +mlx5e_vport_uplink_rep_attach_netdev(struct mlx5_core_dev *dev,
> +				     struct mlx5_eswitch_rep *rep)
> +{
> +	struct mlx5e_rep_priv *rpriv = mlx5e_rep_to_rep_priv(rep);
> +
> +	if (rpriv->netdev)
> +		return 0;
> +
> +	return mlx5e_vport_uplink_rep_load(dev, rep);

[Severity: Medium]
Is it safe to rewrite rpriv->netdev here while the uplink rep is already
REP_LOADED and VF reps may be live?

Before this patch, rpriv->netdev was only set inside ops->load. That
happened before the REP_LOADED transition that
mlx5_eswitch_get_proto_dev() checks.

On this path, mlx5e_vport_uplink_rep_load() publishes rpriv->netdev
first. mlx5e_netdev_change_profile() then detaches, cleans up and memsets
netdev_priv() before re-initializing it. On error the pointer goes back
to NULL.

The writer holds devlink and reps_lock. TC offload on VF reps holds
neither when it reads the uplink proto dev, and it dereferences the
result without a NULL check. For example, in get_route_and_out_devs():

    uplink_dev = mlx5_eswitch_uplink_get_proto_dev(esw, REP_ETH);

    rcu_read_lock();
    uplink_upper = netdev_master_upper_dev_get_rcu(uplink_dev);

and in mlx5e_route_lookup_ipv4_get():

    attr->fl.fl4.flowi4_oif = uplink_dev->ifindex;

parse_mirred() in en/tc/act/mirred.c does the same.

Could a flower rule added on a VF rep while the attach work runs see a
netdev whose priv is zeroed mid-change (priv->mdev == NULL in
mlx5e_is_uplink_rep())? Could it see the NULL written after a failed
change? Nothing such as READ_ONCE() or WRITE_ONCE() orders this pointer.

The NULL dereference in the window before eth is rebound comes from
6b4be64fd9fe and is pre-existing. What this patch adds is the uplink
profile changing while VF reps are loaded and handling TC.

> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 5f28d906c35b6..ab93c807b58ce 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -278,6 +278,9 @@ void mlx5_core_uplink_netdev_set(struct mlx5_core_dev *dev, struct net_device *n
>  	mlx5_blocking_notifier_call_chain(dev, MLX5_DRIVER_EVENT_UPLINK_NETDEV,
>  					  netdev);
>  	mutex_unlock(&dev->mlx5e_res.uplink_netdev_lock);
> +
> +	if (netdev)
> +		mlx5_esw_offloads_uplink_netdev_attach(dev);

[Severity: Low]
This queues the reload-reps work from inside _mlx5e_probe(), and probe
keeps using priv afterwards:

    mlx5_core_uplink_netdev_set(mdev, netdev);
    mlx5e_params_print_info(mdev, &priv->channels.params);

When probe comes from a sysfs bind, the core devlink lock is not held.
esw_wq_handler() can then devl_trylock() and run
mlx5e_netdev_change_profile() on another CPU. That runs
mlx5e_detach_netdev(), profile->cleanup() and mlx5e_priv_cleanup() before
re-initializing priv.

Does the read of priv->channels.params race with that teardown? The
visible effect seems limited to wrong values in the informational log
line, but it is still a data race on priv.

>  }
>  
>  void mlx5_core_uplink_netdev_event_replay(struct mlx5_core_dev *dev)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006110219.257714-1-tariqt%40nvidia.com