Re: [PATCH net-next v1 11/11] eth: fbnic: Support AF_XDP page pool zero copy

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

Critical: 0 · High: 1 · Medium: 1 · Low: 3

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] Stale, unreferenced xsk_buff_pool pointer is used after a failed
  queue restart during AF_XDP unbind.
- [Medium] On an AF_XDP provider queue, an empty FILL ring is normal flow
  control, but fbnic counts it as a buffer allocation failure.
- [Low] The commit message presents the balanced header/payload refill as
  something for the shared provider page pool ("Header and payload rings…
- [Low] The new "Zero-copy through page pool" section of
  Documentation/networking/af_xdp.rst says "MTU and XDP program changes…
- [Low] The new af_xdp.rst text says: "If the device cannot do zero-copy
  for the socket, a bind with XDP_ZEROCOPY fails with EOPNOTSUPP, while…

--

eth: fbnic: Support AF_XDP page pool zero copy

This adds AF_XDP zero-copy support to fbnic through a page pool memory
provider. The header and payload BDQs share one provider page pool and
use a new balanced refill policy, and socket TX descriptors go out on
the XDP TX ring from the RX NAPI after RX processing. It also adds
per-queue rx_headroom support and documents the page pool zero-copy mode
in af_xdp.rst.

[ ... ]

[Severity: Low]
Does the page pool bind path always behave this way?

xsk_pp_bind() in net/xdp/xsk_pp.c returns -EBUSY when another queue has
an unreadable memory provider:

    if (!netif_all_mps_have_cap(dev, MP_CAP_READABLE))
        return -EBUSY;

It also passes on errors from the driver's XDP_VALIDATE_XSK_POOL check,
xp_dma_map(), xsk_pp_create() (-ENOMEM, -ERANGE) and netif_mp_open_rxq()
(for example -EEXIST, -EINVAL, -ENOMEM).

The copy-mode fallback is also conditional. In xp_assign_dev():

    if (!force_zc && !pool->pp)
        err = xp_alloc_heads(pool); /* fallback to copy mode */

Suppose netif_mp_open_rxq() fails, and then either xp_alloc_heads()
fails or xsk_pp_abandon() finds an old page pool still holding chunks.
In that case xsk_pp_bind() leaves pool->pp set, and a bind without
XDP_ZEROCOPY fails instead of falling back to copy mode.

Could the text mention the other errnos, and the case where copy mode is
not available?

[ ... ]

[Severity: Low]
Should this list also say that the driver must call xsk_validate_dev()
from its own ndo_change_mtu?

In net/core/dev.c, the only callers of xsk_validate_dev() are the XDP
program install paths. No core MTU path calls it. fbnic refuses such MTU
changes only because this patch adds this to fbnic_change_mtu():

    err = xsk_validate_dev(dev, new_mtu, NULL, XDP_MODE_DRV, NULL);

A driver that follows only this list would accept MTU changes that a
bound socket cannot handle. One example is raising the MTU above the
header buffer size for a non-SG socket. The statement above that such
MTU changes are refused would then not hold.

[ ... ]

[ ... ]

[Severity: Medium]
On an AF_XDP provider queue, xsk_pp_alloc_netmems() returns 0 whenever
the reuse stack and the FILL ring are empty. That is normal flow control
for the socket. Is it intended to count it as alloc_failed?

Without XDP_USE_NEED_WAKEUP, xsk_pp_refill_done() returns false in this
case:

    more = pp->reuse_cnt || xskq_cons_nb_entries(pp->fq, 1);
    WRITE_ONCE(pp->starved, !more);
    spin_unlock_bh(&pp->lock);

    return !more && wait;

fbnic_fill_bdqs() then returns false, fbnic_clean_rcq() returns budget,
and NAPI polls again at once. Each poll fails the allocation again and
increments the counter again.

This counter reaches userspace through the standard queue stats in
fbnic_netdev.c:

    rx->alloc_fail = fbn->rx_stats.rx.alloc_failed +
        fbn->bdq_stats.bdq.alloc_failed;

Won't an idle xsk queue then report a fast-growing alloc_fail count that
has nothing to do with memory pressure? With need_wakeup, each wakeup
that finds the FILL ring empty would also be counted.

[ ... ]

[Severity: Low]
The commit message says:

    Header and payload rings share one provider page pool, so refills even
    them out, and a short pool does not starve one of them.

In the default configuration, without AF_XDP, fbnic_alloc_qt_page_pools()
already shares one regular page pool between sub0 and sub1, through this
unchanged branch:

    } else {
        page_pool_get(pp);
    }
    qt->sub1.page_pool = pp;

So this test is true for every normal RX queue as well.
fbnic_fill_shared_bdqs() replaces the old policy, which filled the HPQ
fully and then filled the PPQ, on the default datapath. The new policy
interleaves per buffer through fbnic_hpq_takes_next(), which does u64
multiplications for each buffer. Filling of both rings also stops at the
first allocation failure.

Should the commit message mention this change to the existing datapath?
It also does not mention two other changes: QCFG_RX_HEADROOM is now
accepted, and RDE_CTL0 MIN_HROOM is programmed per queue.

[ ... ]

[ ... ]

[Severity: High]
This stores the xsk_buff_pool pointer without taking a reference, and
fbnic_queue_start() copies it into the live triad. What happens to this
pointer if the queue restart during unbind fails?

On socket close, this chain can fail:

__xp_clear_dev()
  xsk_pp_unbind()
    netif_mp_close_rxq()
      __netif_mp_close_rxq()
        netdev_rx_queue_reconfig()

Some ways it can fail:

- fbnic_queue_mem_alloc() returns -ENOMEM from page_pool_create(),
  kvzalloc() or the descriptor ring allocation.
- fbnic_queue_stop() fails through fbnic_wait_queue_idle().
- ndo_queue_start() fails and the old queue is restarted.

__netif_mp_close_rxq() only warns:

    err = netdev_rx_queue_reconfig(dev, rxq_idx, &qcfg[0], &qcfg[1]);
    if (err == -ENETDOWN)
        return 0;
    WARN_ON(err);
    return err;

xsk_pp_unbind() then ignores the result:

    netif_mp_close_rxq(pool->netdev, pool->queue_id, &mp);
    xsk_pp_detach(pp);

xsk_pp_detach() unmaps the DMA through xp_dma_unmap(), and xp_destroy()
later frees the pool. Meanwhile the old fbnic triad keeps running with
qt->xsk_pool still set.

On every poll, fbnic_poll()->fbnic_xsk_tx()->fbnic_xsk_tx_pool() calls
xsk_set_tx_need_wakeup(), xsk_tx_peek_release_desc_batch() and
xsk_buff_raw_get_dma() on that pool. Wouldn't these then read and write
freed memory? In-flight entries in ring->xdp_buf[].xsk would also be
passed to xsk_tx_completed() from fbnic_clean_twq1().

The xsk_rxq_pool() comment says "The pool stays valid while the queue
that the caller allocates runs", but that does not seem to hold on this
failure path. Part of the cause is the core error handling. Still, the
fbnic TX code added here is what keeps using the raw pointer.

[ ... ]

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