Re: [PATCH net] net: page_pool: bound queue_idx before indexing dev->_rx[]

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] The new check in page_pool_init() (net/core/page_pool.c) bounds
  queue_idx by netdev->real_num_rx_queues.
- [Medium] The commit message says "A queue_idx at or beyond
  real_num_rx_queues reads past the dev->_rx[] allocation".

--

net: page_pool: bound queue_idx before indexing dev->_rx[]

This patch adds a check to page_pool_init() for pools created with
PP_FLAG_ALLOW_UNREADABLE_NETMEM. The check rejects a queue_idx at or
above netdev->real_num_rx_queues, and it runs before
__netif_get_rx_queue() is used to look up the queue's memory provider.

> [ ... ]  A
> queue_idx at or beyond real_num_rx_queues reads past the dev->_rx[]
> allocation:

[Severity: Medium]

Is this description accurate? dev->_rx[] is sized by num_rx_queues, not
by real_num_rx_queues:

net/core/dev.c:netif_alloc_rx_queues() {
	unsigned int i, count = dev->num_rx_queues;
	struct netdev_rx_queue *rx;
	size_t sz = count * sizeof(*rx);
    ...
}

alloc_netdev_mqs() sets both num_rx_queues and real_num_rx_queues to
rxqs. Drivers can then lower real_num_rx_queues.

An index in [real_num_rx_queues, num_rx_queues) is still inside the
allocation. The quoted KASAN report needs an index >= num_rx_queues.

Could the commit message be corrected? It could also say that the new
check rejects indices that are inside the allocation.

> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index d7c88c0b67e53..87f14ae855e32 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -274,6 +274,15 @@ static int page_pool_init(struct page_pool *pool,
>  
>  	if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
>  		netdev_assert_locked(pool->slow.netdev);
> +
> +		/* queue_idx indexes dev->_rx[]; reject a value the device
> +		 * cannot serve before dereferencing the queue.
> +		 */
> +		if (pool->slow.queue_idx >= pool->slow.netdev->real_num_rx_queues) {
> +			err = -EINVAL;
> +			goto free_ptr_ring;
> +		}

[Severity: High]

Can this check reject valid queue indices when a driver increases its
number of rx queues?

Before this patch, an index between real_num_rx_queues and num_rx_queues
was in bounds and just found NULL mp_params. Several drivers that set
PP_FLAG_ALLOW_UNREADABLE_NETMEM create page pools for new rx queues
before they raise real_num_rx_queues.

For bnxt, here is the path when ethtool -L increases the combined count:

bnxt_set_channels()
  bnxt_close_nic()
  bp->rx_nr_rings = channel->combined_count;
  bnxt_open_nic()
    __bnxt_open_nic()
      bnxt_alloc_mem()
        bnxt_alloc_rx_rings()
          bnxt_alloc_rx_page_pool()
            page_pool_create()
              page_pool_init()  <-- queue_idx >= stale real_num_rx_queues
      ...
      bnxt_request_irq()
        bnxt_setup_int_mode()
          bnxt_set_real_num_queues()
            netif_set_real_num_rx_queues(dev, bp->rx_nr_rings)

bnxt_alloc_rx_page_pool() always sets the flag, with queue_idx =
rxr->bnapi->index. So this would affect every bnxt device, even without
devmem. bnxt_alloc_mem() would fail and the interface would stay down.

The same ordering seems to apply to:

  bnge: bnge_open_core() -> bnge_alloc_core() -> ... ->
  bnge_alloc_rx_page_pool() runs before bnge_request_irq() ->
  bnge_setup_interrupts() -> netif_set_real_num_queues()

  mlx5 with SHAMPO: mlx5e_safe_switch_params() -> mlx5e_open_channels()
  creates the pools before the preactivate hook
  mlx5e_num_channels_changed() calls netif_set_real_num_rx_queues()

  gve with header split: gve_adjust_config() -> gve_queues_mem_alloc()
  runs before gve_queues_start() calls netif_set_real_num_rx_queues()

The existing helpers netif_rxq_has_unreadable_mp() and netif_rxq_has_mp()
in net/core/netdev_rx_queue.c treat such indices as having no memory
provider, not as an error:

	if (rxq_idx < dev->real_num_rx_queues)
		return __netif_get_rx_queue(dev, rxq_idx)->mp_params.mp_ops;
	return false;

Would it work to bound queue_idx by num_rx_queues instead? Another option
is to skip the mp lookup above real_num_rx_queues rather than return
-EINVAL. Because of the Fixes: tag, this change would also go to stable
trees.

>  		rxq = __netif_get_rx_queue(pool->slow.netdev,
>  					   pool->slow.queue_idx);
>  		pool->mp_priv = rxq->mp_params.mp_priv;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155611.68473-1-phx0fer%40gmail.com