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

[netdev-bot+sashiko@kernel.org]

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_rx_qt_resources() programs the PPQ device-page
  geometry unconditionally (`fbnic_bdq_set_page_size(&qt->sub1,…

Pre-existing issues:
- [High] In fbnic_alloc_qt_page_pools()
  (drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:1611-1623) the local…

--

fbnic: Support larger memory-provider RX pages

The patch lets memory providers pick a larger RX page size for a queue via
QCFG_RX_PAGE_SIZE, uses that rendered rx_page_size for the PPQ page-pool
allocations and completion decoding, and adds ndo_default_qcfg /
ndo_validate_qcfg callbacks to sanity check the requested size.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 28b962307c080..5e22905a63739 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

> @@ -1599,6 +1603,8 @@ fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct fbnic_q_triad *qt,
>  
>  	qt->sub0.page_pool = pp;
>  	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;
>  		pp_params.flags |= PP_FLAG_ALLOW_UNREADABLE_NETMEM;
>  		pp_params.dma_dir = DMA_FROM_DEVICE;
>  
>  		pp = page_pool_create(&pp_params);
>  		if (IS_ERR(pp))
>  			goto err_destroy_sub0;

[Severity: High]
This isn't a bug introduced by this patch, but the patch widens the set of
inputs that can make this page_pool_create() fail, so it seems worth
mentioning.

At the err_destroy_sub0 label, pp has already been reassigned to the
ERR_PTR returned by the second page_pool_create():

	err_destroy_sub0:
		page_pool_destroy(pp);
		return PTR_ERR(pp);

page_pool_destroy() only short-circuits on NULL:

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

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

and page_pool_put() then does refcount_dec_and_test(&pool->user_cnt) on
the error pointer. Should the label destroy qt->sub0.page_pool instead,
and save the error value before overwriting pp?

Along the same path, is qt->sub0.page_pool leaked together with its DMA
mappings? fbnic_alloc_rx_qt_resources() returns the error before reaching
its free_page_pools label, fbnic_alloc_nv_resources()'s "while (i--)"
unwind skips the failing qt, and on the queue-reconfig side
netdev_rx_queue_reconfig() goes to err_free_old_mem (kvfree of the qmem
container only, no ndo_queue_mem_free) when ndo_queue_mem_alloc fails.

The second create can fail for ordinary reasons: page_pool_init() returns
-ENOMEM for the ptr_ring allocation (up to 16384 entries here) and can
also propagate an error from pool->mp_ops->init(pool).

[ ... ]

> @@ -2028,15 +2034,18 @@ static int fbnic_alloc_tx_qt_resources(struct fbnic_net *fbn,
>  
>  static int fbnic_alloc_rx_qt_resources(struct fbnic_net *fbn,
>  				       struct fbnic_napi_vector *nv,
> -				       struct fbnic_q_triad *qt)
> +				       struct fbnic_q_triad *qt,
> +				       u32 rx_page_size)
>  {
>  	struct device *dev = fbn->netdev->dev.parent;
>  	int err;
>  
> -	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx);
> +	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx, rx_page_size);
>  	if (err)
>  		return err;
>  
> +	fbnic_bdq_set_page_size(&qt->sub1, rx_page_size);
> +

[Severity: High]
Can the BDQ geometry here end up describing a page pool that was never
enlarged?

fbnic_bdq_set_page_size(&qt->sub1, rx_page_size) runs unconditionally,
but fbnic_alloc_qt_page_pools() only raises order/max_len when
netif_rxq_has_unreadable_mp() is true; otherwise sub1 just takes another
reference on sub0's order-0, PAGE_SIZE pool. The two predicates are not
the same:

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

The config renderer applies the memory provider's rx_page_size with no
real_num_rx_queues bound check, while netif_rxq_has_unreadable_mp()
silently reports false above real_num_rx_queues.

When they disagree, does the device get descriptors past the end of the
mapping? fbnic_bd_prep() posts fbnic_bd_page_count() descriptors from a
single allocation:

	__le64 *bdq_desc = &bdq->desc[id * fbnic_bd_page_count(bdq)];
	dma_addr_t dma = page_pool_get_dma_addr_netmem(netmem);
	u64 bd, i = fbnic_bd_page_count(bdq);

and on the completion side fbnic_rcd_bd_page_offset() decodes offsets up
to rx_page_size - 1, which fbnic_add_rx_frag() then feeds to
page_pool_dma_sync_netmem_for_cpu() and xdp_buff_add_frag() as payload
inside what is really a PAGE_SIZE buffer.

A path that seems to reach this: fbnic only sets real_num_rx_queues in
fbnic_set_netif_queues() at open, so after probe it equals
FBNIC_MAX_RXQS from alloc_etherdev_mq(). A devmem/io_uring bind with
rx-page-size=8K against a high queue index is accepted then, "ip link set
up" shrinks real_num_rx_queues (netif_set_real_num_rx_queues() has no
memory-provider check), and a later "ethtool -L combined 32" makes
fbnic_alloc_nv_resources() allocate that queue while
netif_rxq_has_unreadable_mp() returns false but qcfg.rx_page_size is
still 8192.

Before this patch bd_page_shift was always derived from PAGE_SIZE and so
always matched the order-0 pool. Would it make sense to either derive the
BDQ page size from the pool that was actually created, or reject
rx_page_size != PAGE_SIZE when no large-order pool was allocated?

>  	err = xdp_rxq_info_reg(&qt->xdp_rxq, fbn->netdev, qt->sub0.q_idx,
>  			       nv->napi.napi_id);
>  	if (err)

[ ... ]

> @@ -2097,7 +2106,11 @@ static int fbnic_alloc_nv_resources(struct fbnic_net *fbn,
>  
>  	/* Allocate Rx Resources */
>  	for (j = 0; j < nv->rxt_count; j++, i++) {
> -		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i]);
> +		struct netdev_queue_config qcfg;
> +
> +		netdev_queue_config(fbn->netdev, nv->qt[i].cmpl.q_idx, &qcfg);
> +		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i],
> +						  qcfg.rx_page_size);
>  		if (err)
>  			goto free_qt_resources;
>  	}

[ ... ]

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