Re: [PATCH net-next 09/10] net: devmem: add netdev_has_dmabuf_binding() helper

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] netdev_has_dmabuf_binding() matches only on `dev ==
  READ_ONCE(binding->dev)`.
- [Medium] netdev_has_dmabuf_binding() compares the caller's net_device
  pointer with binding->dev for every binding on…
- [Low] The kernel-doc for netdev_has_dmabuf_binding() in
  include/net/netmem.h says without qualification that 'A binding is…

--

net: devmem: add netdev_has_dmabuf_binding() helper

This adds netdev_has_dmabuf_binding(). It reports whether a devmem dmabuf
binding exists for a given netdev, and can also filter on the DMA device
the dmabuf was attached to. Bindings can outlive their xarray slot, so each
one now stays on a new global live list until it is freed.

> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 0cc8572f62caf..8bcf44f43d175 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -387,16 +387,39 @@ static inline unsigned long netmem_get_dma_addr(netmem_ref netmem)

[ ... ]

> +/**
> + * netdev_has_dmabuf_binding - is there a dmabuf binding for a
> + * given @dev and @dma_dev
> + *
> + * @dev: netdev to check bindings
> + * @dma_dev: dma device to check, when NULL it will not be checked
> + *
> + * A binding is reported until its last reference is dropped, which can be
> + * long after the user unbound it: net_iovs of a TX binding can still be
> + * queued in flight, with DMA mappings that are only valid for @dma_dev.

[Severity: Low]
Is this also true for RX bindings?

netdev_has_dmabuf_binding() matches on binding->dev. For RX bindings,
mp_dmabuf_devmem_uninstall() clears that field when the last bound queue
goes away:

net/core/devmem.c:mp_dmabuf_devmem_uninstall() {
    ...
			if (xa_empty(&binding->bound_rxqs)) {
				mutex_lock(&binding->lock);
				ASSERT_EXCLUSIVE_WRITER(binding->dev);
				WRITE_ONCE(binding->dev, NULL);
				mutex_unlock(&binding->lock);
			}
    ...
}

This can happen on netdev unregister through dev_memory_provider_uninstall().
It can also happen on lease teardown through netif_rxq_cleanup_unlease().

After that, the netlink socket, page pools or user-held devmem tokens can
still hold references to the binding. Its sgt is also still mapped against
attachment->dev. Even so, the helper no longer reports the binding for any
netdev.

The helper also walks RX bindings without checking binding->direction.

Should the kernel-doc, and the commit message's "track bindings for their
whole lifetime", say that this only applies to TX bindings?

> + *
> + * Return: true if a binding is found under @dev and possibly @dma_dev
> + */
> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev);

[ ... ]

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 07c62e2e3088c..173d66d1cb6e2 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -357,6 +368,25 @@ struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id)
>  	return binding;
>  }
>  
> +bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	bool found = false;
> +
> +	spin_lock(&net_devmem_live_bindings_lock);
> +	list_for_each_entry(binding, &net_devmem_live_bindings, live_list) {
> +		if (dev == READ_ONCE(binding->dev) &&
> +		    (!dma_dev || binding->attachment->dev == dma_dev)) {

[Severity: High]
Can this miss RX bindings made on a leased queue?

netdev_nl_bind_rx_doit() stores the netdev the user named by ifindex as
binding->dev:

net/core/netdev-genl.c:netdev_nl_bind_rx_doit() {
    ...
	binding = net_devmem_bind_dmabuf(netdev, NULL, dma_dev, DMA_FROM_DEVICE,
					 dmabuf_fd, niov_shift, priv,
					 info->extack);
    ...
}

When that netdev is a virtual leasee such as netkit, the rest of the setup
still points at the physical NIC:

  - netdev_queue_get_dma_dev() follows the lease and returns the physical
    queue's DMA device. The dmabuf is attached and mapped against it.

  - netif_mp_open_rxq() installs the memory provider on the physical RX
    queue through __netif_get_rx_queue_lease(..., NETIF_VIRT_TO_PHYS).

The physical driver then fills its RX rings with net_iov DMA addresses
that are only valid for that dma_dev. But
netdev_has_dmabuf_binding(phys_netdev, ...) returns false, because
binding->dev is the netkit device.

TX doesn't have this problem, because netdev_nl_bind_tx_doit() stores the
physical bind_dev in binding->dev and the virtual device in vdev.

The only caller in the series is set_pflag_data_direct() in
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:

    if (netdev_has_dmabuf_binding(netdev, NULL)) {

For a leased RX binding this check doesn't fire, so
mlx5e_safe_switch_params() goes ahead and reopens the channels. The queue
still has its mp installed, so this code in mlx5e_open_channel():

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

would use the other DMA device with IOVAs that were mapped for the old one.

Could that cause IOMMU faults? Or could RX DMA land wherever those IOVAs
point in the other domain?

For RX bindings, should the lookup also match the physical device behind
a lease?

[Severity: Medium]
For TX bindings, could this pointer comparison match a stale binding->dev?

net_devmem_bind_dmabuf() sets binding->dev = dev without taking a netdev
reference, and netdev_nl_bind_tx_doit() doesn't take one either. The only
later write to the field is WRITE_ONCE(binding->dev, NULL) in
mp_dmabuf_devmem_uninstall(). That only runs for RX queues with an
installed memory provider.

With this patch, TX bindings stay on net_devmem_live_bindings until their
last percpu ref is dropped. That only happens once the owning netlink
socket is closed and all in-flight skbs are freed.

If the netdev is unregistered and freed before then, the list entry keeps
a dangling net_device pointer. An mlx5 driver rebind or a devlink reload
could do that.

If a new net_device is later allocated at the same address,
netdev_has_dmabuf_binding(new_netdev, NULL) would return true for a
binding that belongs to the old device. set_pflag_data_direct() would then
keep returning -EBUSY for as long as the old netlink socket stays open.

The stale TX binding->dev was already there before this patch. What this
patch adds is using it as a lookup key for the whole binding lifetime.

> +			found = true;
> +			break;
> +		}
> +	}
> +	spin_unlock(&net_devmem_live_bindings_lock);
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(netdev_has_dmabuf_binding);

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