Re: [PATCH net-next v3 4/5] fbnic: Support larger memory-provider RX pages

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

- [High] fbnic_alloc_qt_page_pools() and fbnic_alloc_rx_qt_resources()
  use two non-equivalent predicates for the same decision, which lets…

Pre-existing issues:
- [High] In fbnic_alloc_qt_page_pools() the error label err_destroy_sub0
  calls page_pool_destroy(pp) after `pp` has been overwritten with the…

--

fbnic: Support larger memory-provider RX pages

Renders the per-queue rx_page_size and uses it for the PPQ page-pool
allocation order and the PPQ BDQ page geometry, so one page-pool
allocation is described by consecutive 4 KiB device-page descriptors.
Also adds ndo_default_qcfg/ndo_validate_qcfg and advertises
QCFG_RX_PAGE_SIZE.

A couple of questions about the two predicates used for the page-pool
geometry, and about the existing error label in
fbnic_alloc_qt_page_pools().

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but the error label reached by
the second page_pool_create() in this function looks broken, and the new
order/max_len parameters add failure modes to that call:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_alloc_qt_page_pools() {
	...
		pp = page_pool_create(&pp_params);
		if (IS_ERR(pp))
			goto err_destroy_sub0;
	...
err_destroy_sub0:
	page_pool_destroy(pp);
	return PTR_ERR(pp);
}

At the label, pp is the ERR_PTR from the failed create, not
qt->sub0.page_pool. page_pool_destroy() only guards against NULL:

net/core/page_pool.c:page_pool_destroy() {
	if (!pool)
		return;

	if (!page_pool_put(pool))
		return;
	...
}

so page_pool_put() does refcount_dec_and_test(&pool->user_cnt) on an error
pointer. Can this oops, and does it also leak the sub0 page_pool together
with its DMA mappings, given the caller propagates the error and
netdev_rx_queue_reconfig() goes to err_free_old_mem without calling
ndo_queue_mem_free?

Should the label be something like:

err_destroy_sub0:
	page_pool_destroy(qt->sub0.page_pool);
	return PTR_ERR(pp);

The reason this comes up here is that pp_params.order is now derived from
a user-supplied value, and the memory provider validates it:

net/core/devmem.c:mp_dmabuf_devmem_init() {
	...
	if (pool->p.order != binding->niov_shift - PAGE_SHIFT)
		return -E2BIG;
	...
}

fbnic_validate_qcfg() accepts any power-of-two rx_page_size >= 4 KiB, so on
a host with PAGE_SIZE larger than 4 KiB get_order(rx_page_size) can differ
from the provider's chunk order and this create fails deterministically,
not just on -ENOMEM. The label exists verbatim at b8e9e7d82e7e and is
still present at dadc755bdd0c.

[Severity: High]
Can the sub1 BDQ geometry end up describing more memory than the pool
actually allocates here?

The pool order is only raised inside the memory-provider branch of
fbnic_alloc_qt_page_pools():

	if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
		pp_params.order = get_order(rx_page_size);
		pp_params.max_len = rx_page_size;

but fbnic_bdq_set_page_size(&qt->sub1, rx_page_size) above runs
unconditionally. The two are not the same test:

net/core/netdev_rx_queue.c:netif_rxq_has_unreadable_mp() {
	if (rxq_idx < dev->real_num_rx_queues)
		return __netif_get_rx_queue(dev, rxq_idx)->mp_params.mp_ops;
	return false;
}

net/core/netdev_config.c:__netdev_queue_config() {
	...
	mpp = &__netif_get_rx_queue(dev, rxq_idx)->mp_params;
	if (mpp->rx_page_size)
		qcfg->rx_page_size = mpp->rx_page_size;
	...
}

So for rxq_idx >= dev->real_num_rx_queues the rendered rx_page_size still
reflects the installed mp_params while netif_rxq_has_unreadable_mp() is
false. sub1 then shares sub0's order-0, PAGE_SIZE, DMA_BIDIRECTIONAL pool
while bd_page_shift claims rx_page_size / 4096 device pages, and
fbnic_bd_prep() posts that many consecutive addresses off one netmem:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_bd_prep() {
	do {
		*bdq_desc = cpu_to_le64(bd);
		bd += FIELD_PREP(FBNIC_BD_DESC_ADDR_MASK, 1) |
		      FIELD_PREP(FBNIC_BD_DESC_ID_MASK, 1);
		bdq_desc++;
	} while (--i);
}

Does that hand the NIC up to rx_page_size - PAGE_SIZE bytes of addresses
past the end of the allocation?

The sequence I think reaches this, all with CAP_NET_ADMIN:

  - with the interface down, dev->real_num_rx_queues is FBNIC_MAX_RXQS,
    since alloc_etherdev_mq() sets it and only fbnic_set_netif_queues()
    (called from __fbnic_open()) narrows it
  - bind a dmabuf with rx-page-size 32768 to a high queue index, say 64;
    netif_mp_open_rxq() only checks rxq_idx < real_num_rx_queues, and
    the !netif_running() path of fbnic_queue_mem_alloc() just creates and
    frees page pools, leaving mp_params installed
  - bring the link up; fbnic_set_netif_queues() calls
    netif_set_real_num_rx_queues(8), and rx_queue_release() does not clear
    mp_params at index 64
  - ethtool -L combined 128 while up; growing the queue count is not
    guarded, and fbnic_alloc_nv_resources() then does

	netdev_queue_config(fbn->netdev, nv->qt[i].cmpl.q_idx, &qcfg);

    which returns rx_page_size 32768 for index 64 while
    netif_rxq_has_unreadable_mp(dev, 64) is false

Would keying both the page-pool parameters and fbnic_bdq_set_page_size()
off the same condition (or clamping rx_page_size to PAGE_SIZE when the
queue has no memory provider) close this?

[ ... ]

[ ... ]

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