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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] xsk: UAF of pool and UMEM on xsk_pp_bind() failure with active
  page_pool
- [High] xsk: unconditional DMA unmap and UMEM release in
  xsk_pp_unbind()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] net: null pointer dereference in __netdev_queue_config()
  [1]
--

commit 34e15c1f4828114e9aaedcfa55376bd7c379c9a3
Author: Björn Töpel <bjorn@kernel.org>

xsk: Install UMEM page pool providers in the core

This commit installs AF_XDP UMEM memory providers through the netdev RX
queue API for drivers setting netdev_queue_mgmt_ops.xsk_page_pool, and
defers allocating pool->heads to copy mode and driver zero-copy.

> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
> index 1975de42a60d9..97e33764737bd 100644
> --- a/net/core/netdev_config.c
> +++ b/net/core/netdev_config.c
> @@ -88,6 +88,8 @@ static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
>  	mpp = &__netif_get_rx_queue(dev, rxq_idx)->mp_params;
>  	if (mpp->rx_page_size)
>  		qcfg->rx_page_size = mpp->rx_page_size;
> +	if (mpp->rx_headroom)
> +		qcfg->rx_headroom = mpp->rx_headroom;
>  	err = validate_cb(dev, qcfg, extack);
>  	if (err)
>  		return err;

[Severity: Critical]
This problem wasn't introduced by this patch, but can __netdev_queue_config()
dereference a NULL dev->queue_mgmt_ops pointer [1] when called on a
device without queue management ops?

Earlier in __netdev_queue_config(), dev->queue_mgmt_ops is dereferenced
without a NULL check:

net/core/netdev_config.c:__netdev_queue_config() {
	...
	validate_cb = netdev_nop_validate_qcfg;
	if (validate && dev->queue_mgmt_ops->ndo_validate_qcfg)
		validate_cb = dev->queue_mgmt_ops->ndo_validate_qcfg;

	memset(qcfg, 0, sizeof(*qcfg));

	/* Get defaults from the driver, in case user config not set */
	if (dev->queue_mgmt_ops->ndo_default_qcfg)
		dev->queue_mgmt_ops->ndo_default_qcfg(dev, qcfg);
	...
}

Calling netdev_queue_config() or netdev_queue_config_validate() on a device
where dev->queue_mgmt_ops is NULL will panic.

> 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

[ ... ]

> +	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;
> +}

[Severity: High]
If netif_mp_open_rxq() fails after ndo_queue_mem_alloc() has created a
page_pool that attached to pp, can this error path cause a use-after-free of
pool and UMEM?

When page_pool_destroy() defers destruction because chunks are still held
(pp->nr_pools > 0), or when xp_alloc_heads(pool) fails:

1. xp_dma_unmap() runs unconditionally before xsk_pp_abandon(), unmapping
   DMA pages while the lingering page_pool still owns chunks.
2. xsk_pp_bind() sets pool->pp = pp without clearing pp->fq, and
   xp_assign_dev() skips the copy-mode fallback because pool->pp is set,
   returning err to xsk_bind().
3. xsk_bind() calls xp_destroy(xs->pool), which drops only pool's reference
   on pp via xsk_pp_put(pool->pp) and immediately kvfree()s pool:

net/xdp/xsk.c:xsk_bind() {
	...
		err = xp_assign_dev(xs->pool, dev, qid, flags);
		if (err) {
			xp_destroy(xs->pool);
			xs->pool = NULL;
			goto out_unlock;
		}
	...
}

The lingering page_pool still holds its reference to pp, leaving pp->pool
dangling (dereferenced in xsk_pp_alloc_fill() and xsk_pp_refill_done()) and
pp->fq pointing to xs->fq_tmp. Because xs->pool is now NULL and xs->state
remains XSK_READY, closing the socket frees xs->fq_tmp and calls
xdp_put_umem(xs->umem, true), which vunmaps umem->addrs and unpins umem->pgs
while the page_pool still holds chunks.

> +/* 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]
Can xsk_pp_unbind() and xp_release_deferred() unmap DMA and free the pool
and UMEM while the RX queue or page_pool is still using them?

xsk_pp_unbind() ignores the return value of netif_mp_close_rxq() and
unconditionally calls xsk_pp_detach(pp) -> xp_dma_unmap(). If
netif_mp_close_rxq() fails (for example, -ENOMEM when allocating replacement
queue memory in netdev_rx_queue_reconfig()), the hardware RX queue remains
active on the unmapped UMEM pages.

Furthermore, right after xp_clear_dev() returns in xp_release_deferred(),
xdp_put_umem(pool->umem, false) unpins and vunmaps the UMEM and
xp_destroy(pool) frees pool, even if netif_mp_close_rxq() failed or
page_pool_destroy() deferred releasing in-flight netmems (pp->refs > 0).

[1] https://sashiko.dev/#/bug/linux-47dd1618-68d7-4419-9441-df54e8f32d4d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110102039.1002605-1-bjorn@kernel.org?part=5