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().
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index fef51d4541026..615b74fc7bef4 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
[ ... ]
> @@ -1600,6 +1604,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;
>
[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.
> @@ -2029,15 +2035,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 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?
> @@ -2098,7 +2107,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;
> }
[ ... ]
> @@ -2997,4 +3072,7 @@ const struct netdev_queue_mgmt_ops fbnic_queue_mgmt_ops = {
> .ndo_queue_mem_free = fbnic_queue_mem_free,
> .ndo_queue_start = fbnic_queue_start,
> .ndo_queue_stop = fbnic_queue_stop,
> + .ndo_default_qcfg = fbnic_default_qcfg,
> + .ndo_validate_qcfg = fbnic_validate_qcfg,
> + .supported_params = QCFG_RX_PAGE_SIZE,
> };
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915182105.4162425-1-bjorn%40kernel.org
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.