Re: [PATCH net v2 7/9] gve: disable NAPI when registering XSK pools in QPL mode

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] On GQI-QPL, the new
  gve_disable_xsk_napis()/gve_enable_xsk_napis() window can leave a…
- [Low] The Fixes: tag and root-cause description in commit "gve: disable
  NAPI when registering XSK pools in QPL mode" appear to point at the…

--

gve: disable NAPI when registering XSK pools in QPL mode

This patch disables the RX NAPI and the XDP TX NAPI of the target queue
while gve_xsk_pool_enable() registers the MEM_TYPE_XSK_BUFF_POOL memory
model in QPL mode. The NAPIs are enabled again afterwards through a new
gve_enable_xsk_napis() helper. The goal is to close a race between packet
processing and the memory model registration.

> GVE in QPL mode does not current respect this, which could lead to a
> race condition between packet processing and the XSK_BUFF_POOL memory
> model registration.

[ ... ]

> Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy support for GQI-QPL format")
> Cc: stable@vger.kernel.org

[Severity: Low]
Is fd8e40321a12 the right Fixes: target? The race described above needs
the memory model of the live rx->xdp_rxq to change while NAPI is running.

In fd8e40321a12, gve_xsk_pool_enable() registered MEM_TYPE_XSK_BUFF_POOL
on a separate rx->xsk_rxq:

    xdp_rxq_info_reg(&rx->xsk_rxq, ...);
    xdp_rxq_info_reg_mem_model(&rx->xsk_rxq, MEM_TYPE_XSK_BUFF_POOL, NULL);
    xsk_pool_set_rxq_info(pool, &rx->xsk_rxq);

The page-backed buffers that NAPI built kept using rx->xdp_rxq, and that
rxq stayed MEM_TYPE_PAGE_SHARED.

The switch on the live rxq seems to come from 077f7153fd25 ("gve: merge
xdp and xsk registration"). That commit changed gve_reg_xsk_pool() to do:

    err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
                                     MEM_TYPE_XSK_BUFF_POOL, pool);

The next patch in this series, "gve: ensure XDP mem model is registered
when disabling XSK pools", fixes the same problem on the disable path, and
it tags 077f7153fd25.

With the current tag, stable would try to backport this to 6.8 through
6.16, including 6.12 LTS. Those kernels don't have the mem-model switch
described here. The patch also won't apply there: gve_reg_xsk_pool() came
from d57ae093c887, and napi_disable_locked() isn't in the older trees.

fd8e40321a12 may have had a separate ordering race, because rx->xsk_pool
was published without an smp_wmb() after xsk_pool_set_rxq_info(). This
NAPI quiesce would close that race too, but the commit message doesn't
describe it.

Should the tag be 077f7153fd25 instead?

Also, the message only says the race "could lead to" a problem and doesn't
name a symptom. Could it mention that the mismatch can end with
container_of(xdp, struct xdp_buff_xsk, xdp) being applied to a page-backed
xdp_buff in the XSK receive path?

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 49ae2b8c6a270..f2bd4011de23f 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1626,6 +1626,40 @@ static int gve_xdp_xmit(struct net_device *dev, int n,

[ ... ]

> +static void gve_enable_xsk_napis(struct gve_priv *priv, u16 qid)
> +{
> +	struct napi_struct *napi_rx, *napi_tx;
> +	u16 tx_qid;
> +
> +	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
> +	napi_enable_locked(napi_rx);
> +
> +	tx_qid = gve_xdp_tx_queue_id(priv, qid);
> +	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
> +	napi_enable_locked(napi_tx);
> +
> +	if (gve_is_gqi(priv)) {
> +		if (gve_rx_work_pending(&priv->rx[qid]))
> +			napi_schedule(napi_rx);
> +
> +		if (gve_tx_clean_pending(priv, &priv->tx[tx_qid]))
> +			napi_schedule(napi_tx);
> +	}
> +}

[Severity: Medium]
On GQI-QPL, can this leave the notify block's IRQ doorbell masked while
NAPI is idle?

In GQI mode, gve_intr() masks the doorbell before it schedules NAPI:

gve_intr() {
    ...
    iowrite32be(GVE_IRQ_MASK, gve_irq_doorbell(priv, block));
    napi_schedule_irqoff(&block->napi);
    ...
}

Only gve_napi_poll() re-arms the doorbell, by writing
GVE_IRQ_ACK | GVE_IRQ_EVENT after napi_complete_done(). A poll that uses
its full budget returns before that write:

gve_napi_poll() {
    ...
    if (reschedule)
        return budget;
    ...
}

Suppose napi_disable_locked() in gve_disable_xsk_napis() overlaps an RX
poll that uses exactly its budget and drains the ring. __napi_poll() then
completes the NAPI without calling the driver again:

__napi_poll() {
    ...
    if (unlikely(napi_disable_pending(n))) {
        napi_complete(n);
        return work;
    }
    ...
}

An interrupt that arrives while the NAPI is disabled has the same effect.
gve_intr() masks the doorbell, but napi_schedule_prep() fails, so no poll
is queued.

gve_enable_xsk_napis() never unmasks the doorbell. It only reschedules if
gve_rx_work_pending() or gve_tx_clean_pending() sees work. If no new
descriptor arrived during the window, would the RX queue and its XDP TX
queue stop getting interrupts?

gve_turnup() unmasks the doorbell and schedules unconditionally:

gve_turnup() {
    ...
    if (gve_is_gqi(priv)) {
        iowrite32be(0, gve_irq_doorbell(priv, block));
    } ...
    mb();
    napi_schedule(&block->napi);
    ...
}

GQI-QPL never sets the XSK RX need_wakeup flag. xsk_set_rx_need_wakeup()
is only used in gve_buffer_mgmt_dqo.c, so an RX-only AF_XDP application
won't kick the NAPI through gve_xsk_wakeup(). The netdev TX watchdog
doesn't cover the XDP TX queue. Regular XDP_PASS traffic on that queue
would stall as well.

This pattern is copied from the existing gve_xsk_pool_disable() code. The
later patch in this series, "gve: ensure XDP mem model is registered when
disabling XSK pools", reuses this helper on the disable path, so the end
of the series still reschedules only conditionally.

Should gve_enable_xsk_napis() unmask the doorbell and schedule NAPI
unconditionally, as gve_turnup() does?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922194533.631387-1-joshwash%40google.com