Re: [PATCH net-next 08/10] net/mlx5e: Recreate netdev channels on data direct device unbind

[netdev-bot+sashiko@kernel.org]

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

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

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] On DD unbind, mlx5e_data_direct_event() clears
  MLX5E_PFLAG_DATA_DIRECT and rebuilds the channels, but it never…
- [Medium] mlx5e_data_direct_event()
  (drivers/net/ethernet/mellanox/mlx5/core/en_main.c) ignores the return…

--

net/mlx5e: Recreate netdev channels on data direct device unbind

On a data direct unbind event, this commit makes the mlx5e notifier clear
MLX5E_PFLAG_DATA_DIRECT and recreate the channels with
mlx5e_safe_switch_params(). It runs under rtnl, the netdev lock and
state_lock. The commit message notes that dmabuf bindings attached to the
data direct device are not torn down.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 6476baf71bdee..fb3e0bd00f04e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6144,9 +6144,28 @@ static int mlx5e_data_direct_event(struct notifier_block *nb,
>  {
>  	struct mlx5e_priv *priv = container_of(nb, struct mlx5e_priv,
>  					       data_direct_nb);
> +	struct mlx5e_params new_params;
> +
> +	if (action != MLX5_DATA_DIRECT_UNBIND)
> +		return NOTIFY_DONE;
> +
> +	rtnl_lock();
> +	netdev_lock(priv->netdev);
> +	mutex_lock(&priv->state_lock);
>  
> -	if (action == MLX5_DATA_DIRECT_UNBIND)
> -		mlx5_core_dbg(priv->mdev, "data direct device unbound\n");
> +	if (!MLX5E_GET_PFLAG(&priv->channels.params, MLX5E_PFLAG_DATA_DIRECT))
> +		goto out;
> +
> +	new_params = priv->channels.params;
> +	MLX5E_SET_PFLAG(&new_params, MLX5E_PFLAG_DATA_DIRECT, false);
> +	mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);

[Severity: High]
What happens to devmem bindings that were mapped against the data direct
device when this switch runs?

While the pflag is set, mlx5e_queue_get_dma_dev() returns the data direct
PCI device. net_devmem_bind_dmabuf() therefore maps the dmabuf in the data
direct function's IOMMU domain, for the RX queues and for TX queue 0.

After the switch, mlx5e_open_channel() computes data_direct as false for
the recreated channels:

    data_direct = MLX5E_GET_PFLAG(params, MLX5E_PFLAG_DATA_DIRECT) &&
                  netif_rxq_has_unreadable_mp(netdev, ix);

and mlx5e_alloc_rq() then uses the ordinary PF UMR mkey:

    wqe->data[0].lkey = data_direct ?
                        cpu_to_be32(rq->mpwqe.crossing_mkey) :
                        rq->mpwqe.umr_mkey_be;

page_pool_init() picks up the same binding again from rxq->mp_params.
mp_dmabuf_devmem_init() does not check or remap the DMA device.

Won't the NIC then write received payload to data direct domain IOVAs as
the PF requester?

On TX, the new SQs no longer have MLX5E_SQ_STATE_DATA_DIRECT, so
mlx5e_txwqe_build_dsegs() uses sq->mkey_be for net_iov frags:

    dseg->lkey = dd && skb_frag_is_net_iov(frag) ?
                 sq->crossing_mkey_be : sq->mkey_be;

So the NIC would read PF domain memory at data direct domain IOVAs.

The commit message mentions IOMMU faults on RX. If an IOVA is also mapped
in the PF domain, could the result be silent memory corruption on RX,
with network-controlled payload, or an information leak on TX?

The later commit "net/mlx5e: Enable the data direct netdev feature" makes
the pflag settable. It also adds a netdev_has_dmabuf_binding() -EBUSY
check, but only in set_pflag_data_direct(). This unbind path still
switches unconditionally, and the series never adds a revoke hook.

At the end of the series, don't the queues still use data direct resources
after unbind, which is what this commit says it prevents?

Do the bindings need to be revoked or quiesced before this switch, or
should the switch be skipped while bindings exist?

> +	netdev_info(priv->netdev,
> +		    "data_direct device unbound, disabled data_direct\n");

[Severity: Medium]
Should the return value of mlx5e_safe_switch_params() be checked here?

When the interface is open, mlx5e_safe_switch_params() can fail if
kzalloc_obj() fails. It can also fail if mlx5e_open_channels() hits an
allocation or firmware command error:

    err = mlx5e_open_channels(priv, new_chs);
    if (err)
        goto err_cancel_selq;

Those error paths free only the new allocations and leave priv->channels
as it was. The old channels stay active:

- The SQs keep MLX5E_SQ_STATE_DATA_DIRECT and the crossing_mkey_be set in
  mlx5e_alloc_txqsq().
- The RQs keep the data direct mkeys.
- priv->channels.params still has MLX5E_PFLAG_DATA_DIRECT set.

Even so, the netdev_info() above reports that data_direct was disabled,
and NOTIFY_OK is returned.

mlx5_data_direct_do_unbind() sends this notification once, after it
clears reg->dd.dev, and never retries. mlx5_data_direct_remove() then
disables and frees the device without checking the result:

    mlx5_data_direct_dev_unreg(dev);
    pci_disable_device(pdev);
    kfree(dev->vuid);
    kfree(dev);

Could devmem-backed queues then keep sending DMA through a PCI function
that has been disabled, or rebound to another driver?

There is also a side effect: with the pflag still set and dd.dev NULL,
mlx5e_queue_get_dma_dev() still takes the data direct branch. But
mlx5_data_direct_get_dev() now returns NULL there, so later devmem or zcrx
binds would fail with no clear reason. Ethtool would also still show the
flag as enabled.

The later commit "net/mlx5e: Enable the data direct netdev feature" makes
this path reachable, and it does not change this callback.

> +
> +out:

[ ... ]

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