Re: [PATCH net-next 07/10] net/mlx5e: Use the correct DMA dev when data_direct pdev enabled

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch makes mlx5e_queue_get_dma_dev() return
  &dd_dev->pdev->dev, the device of a separate, independently removable…
- [Medium] With MLX5E_PFLAG_DATA_DIRECT set, mlx5e_queue_get_dma_dev()
  (drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5916-5922) returns…
- [Medium] At this commit, mlx5e_queue_get_dma_dev() loads dd_dev =
  READ_ONCE(mdev->data_direct->dev) under priv->state_lock and…
- [Low] The commit message says 'Rename netdev argument to netdev', but
  the diff renames the parameter of mlx5e_queue_get_dma_dev() from 'dev'…

--

net/mlx5e: Use the correct DMA dev when data_direct pdev enabled

When MLX5E_PFLAG_DATA_DIRECT is set, this patch makes
mlx5e_queue_get_dma_dev() return the data direct PCI device instead of
the NIC PF DMA device. It also renames the net_device parameter of the
function.

> When the private flag is set, always return the data direct
> PCI device instead of the NIC PF PCI device in .ndo_queue_get_dma_dev.
>
> Rename netdev argument to netdev because there are too many confusing
> "dev"-based names in this function.

[Severity: Low]
This isn't a bug, but should this read "Rename dev argument to netdev"?
The diff renames the parameter from dev to netdev.

Is "always" accurate? If mlx5_data_direct_get_dev() returns NULL (no
bound dd device, or mdev->data_direct == NULL), the function returns NULL.
It does not fall back to the PF device.

At this commit, set_pflag_data_direct() in en_ethtool.c still returns
-EOPNOTSUPP, so the flag can't be set yet. Earlier patches in the series
say when a change has no runtime effect. Could this message say so too?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index d5282a3016218..6476baf71bdee 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -5913,7 +5913,16 @@ static struct device *mlx5e_queue_get_dma_dev(struct net_device *dev,
>  		goto out;
>  
>  	ch = channels->c[queue_index];
> -	pdev = ch->pdev;
> +	if (MLX5E_GET_PFLAG(&channels->params, MLX5E_PFLAG_DATA_DIRECT)) {
> +		struct mlx5_core_dev *mdev = ch->mdev;
> +		struct mlx5_data_direct_dev *dd_dev;
> +
> +		dd_dev = mlx5_data_direct_get_dev(mdev->data_direct);

[Severity: Medium]
What keeps dd_dev alive between this load and the dd_dev->pdev
dereference below? The reader holds priv->state_lock. At this commit the
unbind path does not take that lock:

mlx5_data_direct_remove()
  mlx5_data_direct_dev_unreg()
    mlx5_data_direct_do_unbind()
      clears the dd pointer, calls the UNBIND notifier
        mlx5e_data_direct_event()   <- only logs
  kfree(dev)

The pflag can't be set at this commit, so this path isn't reachable yet.
The next patch in the series, "net/mlx5e: Recreate netdev channels on data
direct device unbind", makes mlx5e_data_direct_event() take rtnl_lock,
netdev_lock and priv->state_lock before kfree(dev) runs. That closes the
race for priv->mdev.

A gap still seems to remain. mlx5e_nic_enable() registers the notifier
only on priv->mdev:

	mlx5_data_direct_register(mdev, &priv->data_direct_nb);

This reader uses ch->mdev instead. On a Socket-Direct netdev, can an
unbind of a secondary PF's dd device race with this lookup? That case
only arises if SD and data direct are used together.

> +		if (dd_dev)
> +			pdev = &dd_dev->pdev->dev;

[Severity: Medium]
Does the returned device match the device the RQ page_pool is created
with? mlx5e_alloc_rq() still uses:

	pp_params.dev       = rq->pdev;

rq->pdev is set in mlx5e_open_channel():

	c->pdev     = mlx5_core_dma_dev(mdev);

That is the PF device, even when the RQ is opened with data_direct set.

io_uring zcrx stores the ndo result and maps the area against it. It then
checks that device against the page_pool device:

io_register_zcrx_ifq()
	ifq->dev = netdev_queue_get_dma_dev(ifq->netdev, if_rxq, NETDEV_QUEUE_TYPE_RX);

io_pp_zc_init()
	if (WARN_ON_ONCE(ifq->dev != pp->p.dev))
		return -EINVAL;

With the pflag set, would registering a zcrx ifq hit this WARN, so that
page_pool_create() and the queue restart both fail?

Even without that check, zcrx_sync_for_device() syncs through pp->p.dev
(the PF). The area, however, was mapped on the data direct device.

There is also a related case. The final patch in the series guards the
pflag change only with netdev_has_dmabuf_binding(). That covers devmem
but not zcrx.

Suppose a zcrx memory provider is installed before the flag is flipped.
mlx5e_open_channel() then computes data_direct=true, because
netif_rxq_has_unreadable_mp() only checks mp_ops. The area is still mapped
on the PF, so the device check passes, but the RQ uses the data direct
PD/KSM mkey. Is that combination expected to work?

[Severity: High]
Can this device go away while a devmem binding still uses it? It belongs
to a separate PCI function (mlx5_dd) that can be removed on its own, and
no reference is taken on it here.

devmem keeps the pointer for as long as the binding exists:

net_devmem_bind_dmabuf()
  dma_buf_attach(binding->dmabuf, dma_dev)
    dma_buf_dynamic_attach()
      attach->dev = dev;    <- no get_device()

The mapping is only torn down later:

__net_devmem_dmabuf_binding_free()
  dma_buf_unmap_attachment_unlocked(binding->attachment, ...)
  dma_buf_detach(binding->dmabuf, binding->attachment)

mlx5_data_direct_remove() does not revoke these bindings. At this commit
the UNBIND notifier only logs.

Later in the series, "net/mlx5e: Recreate netdev channels on data direct
device unbind" clears the pflag and recreates the channels. Its commit
message says the recreated channels "will still pick up the binding via
rxq->mp_params and post DMA addresses from the data direct IOMMU domain
to the PF, causing IOMMU faults".

After a dd unbind or hot-remove with a binding still active, could the
NIC post dd-domain IOVAs through the PF? That would mean IOMMU faults, or
DMA into the wrong memory if the domain is reassigned, for example to
vfio.

On hot removal, could the struct pci_dev be freed while attach->dev still
points to it? The deferred __net_devmem_dmabuf_binding_free() would then
unmap and detach against freed memory.

netdev_has_dmabuf_binding() in the final patch blocks pflag changes from
ethtool. It doesn't seem to cover the dd device being unbound.

[ ... ]

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