Re: [PATCH net-next V2 11/13] net: add netdev_unbind_dma_dev() helper

[netdev-bot+sashiko@kernel.org]

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

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

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_unbind_dma_dev() (net/core/netdev_queues.c:84) refuses
  only when the netdev is both running and present (`netif_running(dev)…
- [Medium] The two halves of netdev_unbind_dma_dev() use different rules
  for which mappings belong to @dma_dev.
- [Medium] __netif_mp_close_rxq() (net/core/netdev_rx_queue.c:291)
  quietly accepts an already-uninstalled provider only when…
- [Low] The commit message says "A detached netdev also works because ...
  new bindings can't be added", but the core does not enforce this for…
- [Low] Several in-tree comments that list binding lifecycle rules were
  not updated for the new site that clears and unmaps bindings, and the…
- [Low] netdev_unbind_dma_dev() treats a NULL result from
  __netdev_queue_get_dma_dev() as "this queue maps against the departing…
- [Low] net_devmem_unbind_dma_dev() (devmem.c:575-596) clears
  NET_DEVMEM_BINDING_LIVE to "hide the binding from TX lookups while it…

--

net: add netdev_unbind_dma_dev() helper

This adds an exported helper for a closed or detached netdev. It
uninstalls the memory providers of RX queues mapped against a departing
DMA device and unmaps the devmem bindings that use that device. It is
meant for mlx5 data direct, where the DMA device is not the netdev's own
PCI device.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index dd3987bbd9f6c..bc8ae12b2318a 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -556,6 +556,48 @@ bool netdev_has_dmabuf_binding(struct net_device *dev, struct device *dma_dev)
>  }
>  EXPORT_SYMBOL_GPL(netdev_has_dmabuf_binding);
>  
> +/* Unmap the bindings of a closed @dev that use @dma_dev, the way netdev
> + * unregister does. Senders that raced the detach drop their skbs at xmit.
> + */
> +void net_devmem_unbind_dma_dev(struct net_device *dev, struct device *dma_dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +	bool match, live;
> +
> +	netdev_assert_locked(dev);
> +
> +	xa_lock(&net_devmem_dmabuf_bindings);
> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
> +		if (READ_ONCE(binding->dev) != dev)
> +			continue;
> +
> +		/* Hide the binding from TX lookups while it is checked. */
> +		live = xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +				   NET_DEVMEM_BINDING_LIVE);
> +		if (live)
> +			__xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +					NET_DEVMEM_BINDING_LIVE);
> +		xa_unlock(&net_devmem_dmabuf_bindings);

[Severity: Low]
Does clearing the mark here actually hide the binding from TX lookups?
net_devmem_lookup_dmabuf() runs under rcu_read_lock() and does:

    binding = xa_find(&net_devmem_dmabuf_bindings, &xa_idx, id,
                      NET_DEVMEM_BINDING_LIVE);

net_devmem_unbind_dmabuf() needs a grace period after clearing the mark
for exactly this reason:

    /* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
     * mark is cleared.
     */
    synchronize_net();

There is no synchronize_net() here. A lookup already in flight can still
find a matching binding and take a reference. The safety of matching
bindings then rests only on clearing binding->dev and the check at xmit
time.

There is also a side effect for bindings that don't match. A binding on
@dev that doesn't use @dma_dev is invisible while xa_lock is dropped. A
concurrent SO_DEVMEM_TX sendmsg on it could get a spurious -EINVAL.

The mark is cleared on entry and restored on exit to guard this region,
rather than to express state. Is the clear and restore needed at all?

> +
> +		mutex_lock(&binding->lock);
> +		match = net_devmem_binding_mapped_to(binding, dma_dev);
> +		mutex_unlock(&binding->lock);
> +		if (match)
> +			net_devmem_dmabuf_binding_unmap(binding);

[Severity: Medium]
The commit message calls this half "devmem TX", and the comment says it
works "the way netdev unregister does". However, nothing here filters on
binding->direction or LIVE. net_devmem_dev_unregister() skips LIVE RX
bindings:

    if (binding->direction == DMA_FROM_DEVICE) {
        if (READ_ONCE(binding->dev) != dev ||
            xa_get_mark(..., NET_DEVMEM_BINDING_LIVE))
            continue;

The two halves of netdev_unbind_dma_dev() also pick mappings by
different rules. The RX loop uses the driver's current per-queue report
and skips leased queues. Here, net_devmem_binding_mapped_to() treats a
NULL @dma_dev as a wildcard:

    return binding->attachment &&
           (!dma_dev || binding->attachment->dev == dma_dev);

Take an RX binding whose queue the RX loop skipped but which matches
here. That can happen with a NULL @dma_dev, with a driver that already
reports a different device, or on the leased-queue path. Such a binding
is unmapped and gets binding->dev cleared while rxq->mp_params and
binding->bound_rxqs still point to it. mp_dmabuf_devmem_uninstall()
avoids this by clearing dev only once bound_rxqs is empty.

A later netlink socket close then reaches net_devmem_unbind_dmabuf()
with a NULL binding->dev:

    if (netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params))

Wouldn't netif_mp_close_rxq() then dereference dev->real_num_rx_queues
through NULL? If the netdev is reopened first, the page pool would be
built from the provider that is still installed, over an sgt that has
already been unmapped.

This doesn't look reachable from the in-tree mlx5 caller. But the helper
is exported, and netdev_has_dmabuf_binding() documents a NULL dma_dev as
"any". Should this half filter on DMA_TO_DEVICE, or should a NULL
@dma_dev be rejected?

> +
> +		xa_lock(&net_devmem_dmabuf_bindings);
> +		if (match) {
> +			WRITE_ONCE(binding->vdev, NULL);
> +			WRITE_ONCE(binding->dev, NULL);

[Severity: Low]
This isn't a bug, but some comments about the binding lifecycle don't
cover this new site.

The net_devmem_lock_dev() kernel-doc lists where binding->dev is
cleared:

 * NULL while holding netdev_lock(dev) (in mp_dmabuf_devmem_uninstall(),
 * net_devmem_unbind_dmabuf(), or net_devmem_dev_unregister() during

In struct net_devmem_dmabuf_binding, the vdev comment says "cleared with
WRITE_ONCE() on unregistration". The ref comment says "TX bindings are
also unmapped earlier if their device is unregistered". Both of these now
also happen when the DMA device goes away.

The new netdev_unbind_dma_dev() kernel-doc says it uninstalls queues
"that map against @dma_dev". It also uninstalls queues for which the
driver reports no DMA device at all.

Should these comments be updated?

> +		} else if (live) {
> +			__xa_set_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +				      NET_DEVMEM_BINDING_LIVE);
> +		}
> +	}
> +	xa_unlock(&net_devmem_dmabuf_bindings);
> +}

[ ... ]

> diff --git a/net/core/netdev_queues.c b/net/core/netdev_queues.c
> index 27cb6397bdd4e..71cc7bfc087dd 100644
> --- a/net/core/netdev_queues.c
> +++ b/net/core/netdev_queues.c

[ ... ]

> @@ -58,6 +60,55 @@ struct device *netdev_queue_get_dma_dev(struct net_device *dev,
>  	return dma_dev;
>  }
>  
> +/**
> + * netdev_unbind_dma_dev() - drop the memory providers of a departing DMA device
> + * @dev:	net_device, closed or detached by the caller
> + * @dma_dev:	DMA device the driver is about to lose

[Severity: Low]
The commit message says:

  A detached netdev also works because the driver has already torn down
  the queues and new bindings can't be added.

Is that enforced for io_uring zcrx? The devmem netlink bind paths check
netif_device_present(). zcrx_register_netdev() only needs
netdev_queue_get_dma_dev() to return non-NULL before it calls
netif_mp_open_rxq(). Neither netif_mp_open_rxq() nor
__netif_mp_open_rxq() checks whether the device is present.

So once this returns and the instance lock is dropped, the only thing
stopping a new zcrx area from being mapped against the departing device
is the driver's ndo_queue_get_dma_dev() no longer reporting it. mlx5
does stop reporting it: channels->num is 0 when closed, and dd.dev is
cleared before the notifier runs.

The kernel-doc only says "net_device, closed or detached by the caller".
Should it also state that the driver must stop reporting @dma_dev?

> + *
> + * Uninstalls the memory provider of every RX queue of @dev that maps
> + * against @dma_dev. Unmaps the devmem bindings of @dev that use @dma_dev.
> + * The bindings stay allocated until their users release them.

[ ... ]

> + * Return: 0 on success, -EBUSY if @dev is running and present.
> + */
> +int netdev_unbind_dma_dev(struct net_device *dev, struct device *dma_dev)
> +{
> +	unsigned int i;
> +
> +	netdev_assert_locked(dev);
> +
> +	if (netif_running(dev) && netif_device_present(dev))
> +		return -EBUSY;

[Severity: High]
Can a netdev that is running but detached get past this check while
validated devmem TX skbs are still sitting in a qdisc?

The commit message says:

  The netdev must be closed or detached so that the hardware no longer
  accesses the mappings and the qdiscs hold no validated skbs.

netif_device_detach() only clears __LINK_STATE_PRESENT and stops the TX
queues. Only dev_close()->dev_deactivate()->qdisc_reset() empties the
qdiscs.

The mlx5 caller later in this series seems to reach exactly this state
in mlx5e_nic_disable():

    if (netif_running(priv->netdev))
        mlx5e_close(priv->netdev);
    netif_device_detach(priv->netdev);
    ...
    mlx5e_data_direct_unbind_dev(priv);

mlx5e_close() is the ndo_stop, so __LINK_STATE_START stays set. The
check above then passes because the device is not present.

Consider this sequence:

sch_direct_xmit()
  validate_xmit_skb_list()
    validate_xmit_unreadable_skb()   <- binding->dev still set, passes
                                     mlx5e_close() stops the txq
  dev_requeue_skb(skb, q)            <- skb parked in q->gso_skb

netdev_unbind_dma_dev()
  net_devmem_unbind_dma_dev()
    net_devmem_dmabuf_binding_unmap()
      dma_buf_unmap_attachment()
      dma_buf_unpin()

On resume, mlx5e_open() and netif_device_attach() wake the queue.
dequeue_skb() then returns the gso_skb with:

    /* skb in gso_skb were already validated */
    *validate = false;

The binding->dev check in validate_xmit_unreadable_skb() never runs
again for that skb. Would the NIC then be given net_iov DMA addresses
that are no longer mapped for the DMA device?

The comment above net_devmem_unbind_dma_dev() says "Senders that raced
the detach drop their skbs at xmit". That doesn't seem to hold for these
requeued skbs.

For RX, the check relies on an unenforced convention that a detached
driver has already torn down its RX rings. mlx5 meets it because it
closes before detaching. A caller that only detaches would have its
dmabuf and zcrx mappings revoked while RX descriptors still point at
them. Should the exported helper enforce or document this?

> +
> +	for (i = 0; i < dev->real_num_rx_queues; i++) {
> +		struct netdev_rx_queue *rxq = __netif_get_rx_queue(dev, i);
> +		struct pp_memory_provider_params p = rxq->mp_params;
> +		struct device *queue_dev;
> +
> +		if (!p.mp_ops)
> +			continue;
> +
> +		queue_dev = __netdev_queue_get_dma_dev(dev, i);
> +		if (queue_dev && queue_dev != dma_dev)
> +			continue;

[Severity: Low]
Here a NULL from __netdev_queue_get_dma_dev() counts as a match. Is that
intended for a closed netdev?

mlx5e_queue_get_dma_dev() returns NULL whenever:

    /* Allow only opened channels. */
    if (queue_index < 0 || queue_index >= channels->num)
        goto out;

mlx5e_close_channels() sets chs->num = 0. So in the closed state this
helper requires, the @dma_dev filter appears to do nothing for the only
in-tree caller. Every RX queue with a provider is uninstalled, whichever
device it is actually mapped to.

If a driver's queues can map against more than one device, wouldn't
providers on a device that isn't going away be torn down as well? A
leased queue with a provider would also get past this filter and hit
the WARN_ON_ONCE(rxq->lease). The TX half filters on
binding->attachment->dev instead.

mlx5 doesn't allow data direct to be toggled while providers exist, so
there's no in-tree effect today. The semantics still look fragile.

> +
> +		if (WARN_ON_ONCE(rxq->lease))
> +			continue;
> +
> +		memset(&rxq->mp_params, 0, sizeof(rxq->mp_params));
> +		__netif_mp_uninstall_rxq(rxq, &p);

[Severity: Medium]
Can this race with io_close_queue() in io_uring/zcrx.c and trigger the
WARN_ON_ONCE() in __netif_mp_close_rxq()?

__netif_mp_close_rxq() only accepts an already-uninstalled provider on
the unregister path:

    if (dev->reg_state > NETREG_REGISTERED &&
        !rxq->mp_params.mp_ops)
        return 0;

    if (WARN_ON_ONCE(rxq->mp_params.mp_ops != old_p->mp_ops ||

Here mp_params is cleared on a device that is still registered. One
possible sequence:

io_close_queue()
  copies ifq->netdev, sets ifq->netdev = NULL under pp_lock
  netdev_lock(netdev)   <- blocks, held by the unbind caller
                                  netdev_unbind_dma_dev()
                                    memset(&rxq->mp_params, 0, ...)
                                    __netif_mp_uninstall_rxq()
                                      io_pp_uninstall()
                                        io_zcrx_unmap_areas()
                                        io_zcrx_drop_netdev()
                                        (ifq->if_rxq left unchanged)
  if (ifq->if_rxq != -1)
      netif_mp_close_rxq(netdev, ifq->if_rxq, &p);
        __netif_mp_close_rxq()
          reg_state == NETREG_REGISTERED, mp_ops == NULL
          WARN_ON_ONCE(...) -> -EINVAL

The areas are already unmapped, so cleanup still completes. Would a
zcrx teardown that races with a data direct unbind or a suspend produce
a WARN splat (fatal with panic_on_warn) plus a spurious -EINVAL?

> +	}
> +
> +	net_devmem_unbind_dma_dev(dev, dma_dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_unbind_dma_dev);

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