Re: [PATCH net-next v1 05/11] xsk: Install UMEM page pool providers in the core

[netdev-bot+sashiko@kernel.org]

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

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

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] xsk_pp_unbind() in net/xdp/xsk_pp.c ignores the return value of
  netif_mp_close_rxq() and always calls xsk_pp_detach().
- [High] xsk: UAF of pool and UMEM on xsk_pp_bind() failure with active
  page_pool

--

xsk: Install UMEM page pool providers in the core

This commit installs the AF_XDP UMEM as a page pool memory provider
through the queue management API, for drivers that set
netdev_queue_mgmt_ops.xsk_page_pool. Drivers check the chunk size,
headroom and multi-buffer setup in XDP_VALIDATE_XSK_POOL. The core
then verifies, using a per-queue mp_gen, that the new page pools picked
up the provider.

> diff --git a/net/xdp/xsk_pp.c b/net/xdp/xsk_pp.c
> index fdd55265a6cdc..c2214e718cdb3 100644
> --- a/net/xdp/xsk_pp.c
> +++ b/net/xdp/xsk_pp.c

[ ... ]

> @@ -236,11 +243,225 @@ static int xsk_pp_nl_fill(void *mp_priv, struct sk_buff *rsp,
>  	return 0;
>  }
>  
> +/* The queue no longer uses the provider, and its DMA stopped */
> +static void xsk_pp_detach(struct xsk_pp *pp)
> +{
> +	pp->rxq = NULL;
> +	spin_lock_bh(&pp->lock);
> +	pp->fq = NULL;
> +	spin_unlock_bh(&pp->lock);
> +	xp_dma_unmap(pp->pool, XSK_PP_DMA_ATTR);
> +}

[ ... ]

> +/* Called with RTNL and the instance lock of the device */
> +void xsk_pp_unbind(struct xsk_buff_pool *pool)
> +{
> +	struct xsk_pp *pp = pool->pp;
> +	struct pp_memory_provider_params mp = {
> +		.mp_ops = &xsk_pp_ops,
> +		.mp_priv = pp,
> +	};
> +
> +	/* Unregister removed the provider already */
> +	if (!pp->rxq)
> +		return;
> +
> +	netif_mp_close_rxq(pool->netdev, pool->queue_id, &mp);
> +	xsk_pp_detach(pp);
> +}

[Severity: High]
What happens here if netif_mp_close_rxq() fails? xsk_pp_unbind() drops
its return value and always calls xsk_pp_detach(), and that unmaps the
UMEM DMA.

netif_mp_close_rxq() can return an error after the reconfig fails:

net/core/netdev_rx_queue.c:__netif_mp_close_rxq() {
    ...
	memset(&rxq->mp_params, 0, sizeof(rxq->mp_params));
	netdev_queue_config(dev, rxq_idx, &qcfg[1]);

	err = netdev_rx_queue_reconfig(dev, rxq_idx, &qcfg[0], &qcfg[1]);
	if (err == -ENETDOWN)
		return 0;
	WARN_ON(err);
	return err;
}

netdev_rx_queue_reconfig() can fail in several places:

  - kvzalloc() of new_mem or old_mem
  - ndo_queue_mem_alloc()
  - ndo_queue_stop()
  - ndo_queue_start(), which falls back to restarting the old queue

In each case the old queue keeps running. Its page pools still have
mp_priv == pp and mp_ops == &xsk_pp_ops, and the hardware RX ring still
holds UMEM chunk DMA addresses.

The teardown then continues:

xp_release_deferred()
  xp_clear_dev()
    __xp_clear_dev()
      xsk_pp_unbind()
        netif_mp_close_rxq()   <- fails, result dropped
        xsk_pp_detach()
          xp_dma_unmap()
  xdp_put_umem()               <- vunmap(umem->addrs), unpin pages
  xp_destroy(pool)             <- frees the xsk_buff_pool

The surviving page pools hold a reference on the xsk_pp only, taken in
xsk_pp_init(). They hold none on the DMA mapping, the UMEM pages or the
xsk_buff_pool.

Can the NIC then DMA into unmapped IOVAs? Without an IOMMU, can it DMA
into unpinned pages that may already be reused?

Also, xsk_pp_alloc_reuse() would keep recycling chunks with stale DMA
addresses. CPU reads through pp->area.vaddr would hit the vunmapped
umem->addrs, and pp->pool would point to the freed xsk_buff_pool.

Devmem doesn't seem to have this problem, because its page pools pin
the binding and the DMA unmap happens on the last reference drop. Here
the unmap happens no matter what pp->nr_pools is.

netif_rxq_cleanup_unlease() seems to follow the same pattern:

	__netif_mp_close_rxq(phys_rxq->dev, rxq_idx, &p);
	__netif_mp_uninstall_rxq(virt_rxq, &p);

It ignores the close result and then reaches xsk_pp_uninstall(), which
calls xsk_pp_detach().

No driver sets xsk_page_pool at this commit. fbnic opts in later in the
series, and its queue mem_alloc can fail with -ENOMEM.

A later patch in the series adds synchronize_net() before
xp_dma_unmap() in this path. That doesn't seem to help when the old
queue is still running.

Should xsk_pp_detach() be skipped, or the DMA unmap and UMEM release be
deferred until pp->nr_pools drops to zero, when the close fails?

[ ... ]

> @@ -236,11 +243,225 @@ static int xsk_pp_nl_fill(void *mp_priv, struct sk_buff *rsp,
>  	return 0;
>  }
>  
> +/* The queue no longer uses the provider, and its DMA stopped */
> +static void xsk_pp_detach(struct xsk_pp *pp)
> +{
> +	pp->rxq = NULL;
> +	spin_lock_bh(&pp->lock);
> +	pp->fq = NULL;
> +	spin_unlock_bh(&pp->lock);
> +	xp_dma_unmap(pp->pool, XSK_PP_DMA_ATTR);
> +}
> +
> +/* The device unregisters, and is closed */
> +static void xsk_pp_uninstall(void *mp_priv, struct netdev_rx_queue *rxq)
> +{
> +	xsk_pp_detach(mp_priv);
> +}
> +
>  const struct memory_provider_ops xsk_pp_ops = {
>  	.init		= xsk_pp_init,
>  	.destroy	= xsk_pp_destroy,
>  	.alloc_netmems	= xsk_pp_alloc_netmems,
>  	.release_netmem	= xsk_pp_release_netmem,
>  	.nl_fill	= xsk_pp_nl_fill,
> +	.uninstall	= xsk_pp_uninstall,
>  	.caps		= MP_CAP_READABLE,
>  };
> +
> +static bool xsk_pp_abandon(struct xsk_pp *pp)
> +{
> +	struct xsk_buff_pool *pool = pp->pool;
> +	bool busy;
> +	u32 idx;
> +
> +	spin_lock_bh(&pp->lock);
> +	busy = pp->nr_pools;
> +	while (!busy && pp->reuse_cnt) {
> +		idx = pp->reuse[--pp->reuse_cnt];
> +		pp->state[idx] = XSK_PP_USER;
> +		xp_free(&pool->heads[idx]);
> +	}
> +	spin_unlock_bh(&pp->lock);
> +
> +	return !busy;
> +}
> +
> +static int xsk_pp_validate(struct net_device *dev, struct xsk_buff_pool *pool,
> +			   unsigned int mtu, struct bpf_prog *prog,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct xsk_rx_check check = {
> +		.buf_size = pool->chunk_size,
> +		.headroom = xsk_pool_get_headroom(pool),
> +		.mtu = mtu,
> +		.sg = xsk_pool_uses_sg(pool),
> +		.prog = prog,
> +	};
> +	struct netdev_bpf bpf = {
> +		.command = XDP_VALIDATE_XSK_POOL,
> +		.xsk_check.check = &check,
> +		.xsk_check.extack = extack,
> +	};
> +
> +	return dev->netdev_ops->ndo_bpf(dev, &bpf);
> +}
> +
> +/**
> + * xsk_validate_dev - check AF_XDP page pool queues against a new setting
> + * @dev: device of the queues
> + * @mtu: MTU to check
> + * @prog: XDP program to check, or NULL for the current one
> + * @mode: XDP mode of @prog
> + * @extack: netlink extended ack
> + *
> + * Context: holds the instance lock of @dev, or RTNL for a device without it.
> + * Return: 0 if the driver accepts all queues of @dev that have an AF_XDP
> + * memory provider, or its error.
> + */
> +int xsk_validate_dev(struct net_device *dev, unsigned int mtu,
> +		     struct bpf_prog *prog, enum bpf_xdp_mode mode,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct netdev_rx_queue *rxq;
> +	struct xsk_pp *pp;
> +	unsigned int i;
> +	int err;
> +
> +	netdev_assert_locked_ops_compat(dev);
> +
> +	for (i = 0; i < dev->real_num_rx_queues; i++) {
> +		rxq = __netif_get_rx_queue(dev, i);
> +		if (rxq->mp_params.mp_ops != &xsk_pp_ops)
> +			continue;
> +
> +		/* Generic XDP runs outside the queue NAPI */
> +		if (mode == XDP_MODE_SKB) {
> +			NL_SET_ERR_MSG(extack,
> +				       "generic XDP cannot feed an AF_XDP page pool socket");
> +			return -EBUSY;
> +		}
> +
> +		pp = rxq->mp_params.mp_priv;
> +		err = xsk_pp_validate(dev, pp->pool, mtu, prog, extack);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xsk_validate_dev);
> +
> +/**
> + * xsk_rxq_pool - get the AF_XDP pool that provides memory to an RX queue
> + * @dev: device of the queue
> + * @queue_id: RX queue
> + *
> + * The pool stays valid while the queue that the caller allocates runs.
> + *
> + * Context: holds the instance lock of @dev.
> + * Return: the buffer pool, or NULL.
> + */
> +struct xsk_buff_pool *xsk_rxq_pool(struct net_device *dev, u16 queue_id)
> +{
> +	struct netdev_rx_queue *rxq;
> +	struct xsk_pp *pp;
> +
> +	netdev_assert_locked(dev);
> +
> +	if (queue_id >= dev->real_num_rx_queues)
> +		return NULL;
> +
> +	rxq = __netif_get_rx_queue(dev, queue_id);
> +	if (rxq->mp_params.mp_ops != &xsk_pp_ops)
> +		return NULL;
> +
> +	pp = rxq->mp_params.mp_priv;
> +	return pp->pool;
> +}
> +EXPORT_SYMBOL_GPL(xsk_rxq_pool);
> +
> +bool xsk_pp_supported(struct net_device *dev)
> +{
> +	const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
> +
> +	return qops && qops->xsk_page_pool;
> +}
> +
> +/* Called with RTNL and the instance lock of @dev. On error, @pool->pp stays
> + * set if an old page pool still owns chunks; copy mode must not take the pool.
> + */
> +int xsk_pp_bind(struct xsk_buff_pool *pool, struct net_device *dev,
> +		u16 queue_id)
> +{
> +	struct pp_memory_provider_params mp = { .mp_ops = &xsk_pp_ops };
> +	struct device *dma_dev;
> +	struct xsk_pp *pp;
> +	int err;
> +
> +	if (queue_id >= dev->real_num_rx_queues || pool->unaligned)
> +		return -EOPNOTSUPP;
> +	if ((dev->xdp_features & NETDEV_XDP_ACT_XSK) != NETDEV_XDP_ACT_XSK ||
> +	    (xsk_pool_uses_sg(pool) && dev->xdp_zc_max_segs == 1))
> +		return -EOPNOTSUPP;
> +	if (dev_xdp_prog_id(dev, XDP_MODE_SKB))
> +		return -EOPNOTSUPP;
> +	if (!netif_all_mps_have_cap(dev, MP_CAP_READABLE))
> +		return -EBUSY;
> +
> +	err = xsk_pp_validate(dev, pool, dev->mtu, NULL, NULL);
> +	if (err)
> +		return err;
> +
> +	dma_dev = netdev_queue_get_dma_dev(dev, queue_id, NETDEV_QUEUE_TYPE_RX);
> +	if (!dma_dev)
> +		return -EOPNOTSUPP;
> +
> +	err = xp_dma_map(pool, dma_dev, XSK_PP_DMA_ATTR, pool->umem->pgs,
> +			 pool->umem->npgs);
> +	if (err)
> +		return err;
> +
> +	pp = xsk_pp_create(pool);
> +	if (IS_ERR(pp)) {
> +		xp_dma_unmap(pool, XSK_PP_DMA_ATTR);
> +		return PTR_ERR(pp);
> +	}
> +
> +	mp.mp_priv = pp;
> +	mp.rx_page_size = pool->chunk_size;
> +	mp.rx_headroom = xsk_pool_get_headroom(pool);
> +	err = netif_mp_open_rxq(dev, queue_id, &mp, NULL);
> +	if (!err) {
> +		pp->rxq = __netif_get_rx_queue(dev, queue_id);
> +		pool->pp = pp;
> +		pool->xdp_zc_max_segs = dev->xdp_zc_max_segs;
> +		return 0;
> +	}
> +
> +	xp_dma_unmap(pool, XSK_PP_DMA_ATTR);
> +	/* Copy mode takes the chunks that the provider holds */
> +	if (!xp_alloc_heads(pool) && xsk_pp_abandon(pp))
> +		xsk_pp_put(pp);
> +	else
> +		pool->pp = pp;
> +
> +	return err;
> +}
> +
> +/* Called with RTNL and the instance lock of the device */
> +void xsk_pp_unbind(struct xsk_buff_pool *pool)
> +{
> +	struct xsk_pp *pp = pool->pp;
> +	struct pp_memory_provider_params mp = {
> +		.mp_ops = &xsk_pp_ops,
> +		.mp_priv = pp,
> +	};
> +
> +	/* Unregister removed the provider already */
> +	if (!pp->rxq)
> +		return;
> +
> +	netif_mp_close_rxq(pool->netdev, pool->queue_id, &mp);
> +	xsk_pp_detach(pp);
> +}

[Severity: High]
What keeps the buffer pool and the UMEM alive when this error path
leaves pool->pp set?

netif_mp_open_rxq() can fail after ndo_queue_mem_alloc() has already
created page pools that selected the provider. That happens when
page_pool_check_memory_provider() returns -ENODATA, and when
ndo_queue_stop() or ndo_queue_start() fails. Each of those page pools
took a reference and bumped pp->nr_pools in xsk_pp_init().

If ndo_queue_mem_free() leaves one of them in deferred destruction
because chunks are still out, xsk_pp_abandon() returns false and
pool->pp is set. The same happens when xp_alloc_heads() fails with
-ENOMEM.

By that point xp_dma_unmap() has already run unconditionally. The
lingering page pool may still own chunks whose DMA addresses came from
that mapping. pp->fq is also left pointing at the fill queue, unlike
in xsk_pp_detach().

xp_assign_dev() then sees pool->pp set, skips the copy mode fallback
and returns the error. xsk_bind() destroys the pool right away:

	err = xp_assign_dev(xs->pool, dev, qid, flags);
	if (err) {
		xp_destroy(xs->pool);
		xs->pool = NULL;
		goto out_unlock;
	}

xp_destroy() drops only the buffer pool's reference on the xsk_pp.
The xsk_pp survives on the page pool's reference, and then the
xsk_buff_pool is freed, which leaves pp->pool dangling.

The socket stays in XSK_READY, so on close xsk_release() frees
xs->fq_tmp, which pp->fq still points to. xsk_destruct() then calls
xdp_put_umem(), which vunmaps umem->addrs (pp->area.vaddr) and unpins
the UMEM pages. The lingering page pool still holds chunks backed by
those pages.

When that page pool finally returns its chunks, won't the provider
ops reach pp->pool, pp->fq or pp->area.vaddr after they are gone?

The comment above xsk_pp_bind() says pool->pp stays set so that copy
mode does not take the pool. But the caller frees the pool anyway, so
what does keeping pool->pp set protect? A second bind() on the same
socket also gets a fresh pool with pool->pp == NULL for the same UMEM,
so the pool->pp check in xp_assign_dev() does not help there either.

Should the xsk_pp pin the buffer pool and the UMEM, and do the DMA
unmap in xsk_pp_free(), so they outlive the last page pool? Or should
xsk_bind() defer releasing the pool in this case?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org