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

[Dragos Tatulea <dtatulea@nvidia.com>]

On 12.11.26 10:40, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 
v3 doesn't to the mark restore trick anymore.

>> +
>> +		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?
> 
Ack. Will fix in v3 to filter by DMA_TO_DEVICE and rename
net_devmem_unbind_dma_dev() to net_devmem_unbind_tx_dma_dev().

>> +
>> +		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?
> 
Ok. Will update.

>> +		} 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?
>
Driver does have to stop reporting the unbound dma_dev. Will add
to doc.

>> + *
>> + * 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?
> 
I did this to prevent another issue in mlx5e: mlx5e_suspend() disables
the data direct notifier so if the data direct device is unbound after
suspend, the DMA dev ptr would be left dangling. So I wanted to call
netdev_unbind_dma_dev() during suspend. Seems like this is not
the right way.

I will add one more mlx5 patch that closes the netdev on suspend
if it is in use. It will be ugly, but I can't see a better way.

>> +
>> +	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?
Yes, that is the point.

> 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.
>
I chose to keep leased queues as unsupported to keep this series
reasonably sized.

>> +
>> +		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?
> 
Ack. I will add a patch in io_pp_uninstall() to set ifq->if_rxq to -1.

Thanks,
Dragos