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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net: unmapping TX dmabufs on detached netdev in
  netdev_unbind_dma_dev()
- [Medium] net: race between netdev_unbind_dma_dev() and
  io_close_queue()
--

commit 3c773dc0f4402c3fc717479371e244f710ce138c
Author: Dragos Tatulea <dtatulea@nvidia.com>

net: add netdev_unbind_dma_dev() helper

This commit adds netdev_unbind_dma_dev() and net_devmem_unbind_dma_dev() to
uninstall RX memory providers and unmap TX devmem bindings associated with a
departing DMA device on a closed or detached netdev.

> 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
[ ... ]
> +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 allowing netdev_unbind_dma_dev() on a running, detached netdev leave
validated devmem TX skbs queued in q->gso_skb after their dmabuf binding is
unmapped?

Unlike closing a netdev, netif_device_detach() only clears
__LINK_STATE_PRESENT and stops the TX queues without calling
dev_deactivate() or resetting q->gso_skb.  Any devmem TX skb that was
already validated by validate_xmit_unreadable_skb() and requeued onto
q->gso_skb when the queue stopped stays there while
net_devmem_unbind_dma_dev() unmaps the binding and clears binding->dev.

When netif_device_attach() later wakes the queues, dequeue_skb() pulls the
skb from q->gso_skb with *validate = false:

net/sched/sch_generic.c:dequeue_skb() {
    ...
		/* skb in gso_skb were already validated */
		*validate = false;
		if (xfrm_offload(skb))
			*validate = true;
		/* check the reason of requeuing without tx lock first */
		txq = skb_cap_tx_queue(q, skb);
		if (!netif_xmit_frozen_or_stopped(txq)) {
			skb = __skb_dequeue(&q->gso_skb);
    ...
}

sch_direct_xmit() then skips validate_xmit_skb_list() and passes the skb
with unmapped DMA addresses to ndo_start_xmit(), causing DMA to unmapped
memory or IOMMU faults.

> +
> +	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;
> +
> +		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 trigger the WARN_ON_ONCE() in __netif_mp_close_rxq() when racing
with io_close_queue()?

CPU0                                  CPU1
io_close_queue()
  mutex_lock(&ifq->pp_lock)
  netdev = ifq->netdev
  ifq->netdev = NULL
  mutex_unlock(&ifq->pp_lock)
                                      netdev_lock(netdev)
                                      netdev_unbind_dma_dev()
                                        memset(&rxq->mp_params, 0, ...)
                                        __netif_mp_uninstall_rxq()
                                          io_pp_uninstall()
                                            io_zcrx_drop_netdev()
                                      netdev_unlock(netdev)
  netdev_lock(netdev)
  if (ifq->if_rxq != -1)
    netif_mp_close_rxq()
      __netif_mp_close_rxq()
        WARN_ON_ONCE(...)  <-- hits warning

io_close_queue() clears ifq->netdev and drops ifq->pp_lock before taking
netdev_lock(netdev).  If netdev_unbind_dma_dev() runs in that window,
io_zcrx_drop_netdev() sees ifq->netdev == NULL and leaves ifq->if_rxq
unchanged, so io_close_queue() proceeds to call netif_mp_close_rxq().

In __netif_mp_close_rxq(), a cleared rxq->mp_params is only ignored when
dev->reg_state > NETREG_REGISTERED:

net/core/netdev_rx_queue.c:__netif_mp_close_rxq() {
    ...
	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 ||
			 rxq->mp_params.mp_priv != old_p->mp_priv))
		return -EINVAL;
    ...
}

Because netdev_unbind_dma_dev() runs while dev->reg_state is still
NETREG_REGISTERED, __netif_mp_close_rxq() hits the WARN_ON_ONCE().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110183826.2927274-1-tariqt@nvidia.com?part=11