Re: [PATCH net] octeontx2-pf: fix XDP frames freed as skbs on interface down

[netdev-bot+sashiko@kernel.org]

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

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

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] The new CQ_XDP branch in otx2_cleanup_tx_cqes()
  (otx2_txrx.c:1352-1355) sends XDP CQEs to…
- [Medium] octeontx2-pf: missing xsk_tx_completed() call in
  otx2_cleanup_tx_cqes()

Pre-existing issues:
- [High] Nothing fences otx2_xdp_xmit() (ndo_xdp_xmit) off from XDP SQ
  teardown on the 'XDP program is replaced' path the commit message…
- [High] otx2_stop() calls only napi_synchronize() for each CINT
  (otx2_pf.c:2267) before otx2_free_hw_resources() (line 2277).
- [High] In AF_XDP zero-copy mode, otx2_xdp_rcv_pkt_handler()
  (otx2_txrx.c:1520-1527) runs the BPF program on xsk_buff and does…
- [Medium] otx2_free_pending_sqe() (otx2_txrx.c:1408) only walks `sq_idx
  < pfvf->hw.tx_queues`.

--

octeontx2-pf: fix XDP frames freed as skbs on interface down

With this patch, otx2_cleanup_tx_cqes() passes CQEs from XDP completion
queues to otx2_xdp_snd_pkt_handler() instead of treating sg->skb as an
sk_buff. Before this, pending xdp_frames on XDP queues were freed with
dev_kfree_skb_any() on ifdown or XDP program replacement, which corrupted
page pool memory.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> index 90af9b9044aa0..e3b07e5335fb8 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> @@ -1332,6 +1332,7 @@ void otx2_cleanup_tx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue *cq)
>  	struct nix_cqe_tx_s *cqe;
>  	struct netdev_queue *txq;
>  	int processed_cqe = 0;
> +	int xsk_frames = 0;
>  	struct sg_list *sg;
>  	int qidx;
>  
> @@ -1348,6 +1349,10 @@ void otx2_cleanup_tx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue *cq)
>  
>  		if (!cqe)
>  			continue;
> +		if (cq->cq_type == CQ_XDP) {
> +			otx2_xdp_snd_pkt_handler(pfvf, sq, cqe, &xsk_frames);
> +			continue;
> +		}

[Severity: Medium]
The new CQ_XDP branch fills in xsk_frames, but is it ever used after
the loop?

For entries flagged OTX2_AF_XDP_FRAME, otx2_xdp_snd_pkt_handler() only
bumps the counter and returns. It leaves it to the caller to publish
the completions. On the NAPI path, otx2_tx_napi_handler() hands the
count to otx2_zc_submit_pkts(), which calls xsk_tx_completed().

otx2_cleanup_tx_cqes() never reads xsk_frames and never calls
xsk_tx_completed(). It still rings NIX_LF_CQ_OP_DOOR for those CQEs,
so the hardware considers them consumed.

When otx2_zc_napi_handler() queued these frames,
xsk_tx_peek_release_desc_batch() had already consumed the TX ring
descriptors and reserved completion ring entries for them. Consider
an AF_XDP zero-copy socket that is transmitting while the interface
goes down or the XDP program is replaced:

otx2_xdp_setup()->otx2_stop()->otx2_free_hw_resources()->
    otx2_cleanup_tx_cqes()

Do the completions reaped here then never reach the XSK completion
ring? If so, userspace would never get those UMEM frames back. The
reserved completion ring slots would also stay reserved across the
down/up cycle.

sq->xsk_pool is still attached at this point. It is only cleared in
otx2_xsk_pool_disable(). Would calling
xsk_tx_completed(sq->xsk_pool, xsk_frames) after the loop, when
sq->xsk_pool is set and xsk_frames is non-zero, be the right fix?
otx2_zc_submit_pkts() itself doesn't fit here, since it also submits
new TX descriptors.

[Severity: Medium]
Should xsk_frames be reported back to the XSK pool after this loop?

For AF_XDP zero-copy entries, otx2_xdp_snd_pkt_handler() only counts
them:

	if (sg->flags & OTX2_AF_XDP_FRAME) {
		(*xsk_frames)++;
		return;
	}

In the NAPI path, otx2_tx_napi_handler() passes the count to
otx2_zc_submit_pkts(), which does:

	if (*xsk_frames)
		xsk_tx_completed(xsk_pool, *xsk_frames);

otx2_cleanup_tx_cqes() fills xsk_frames but never reads it. It then rings
NIX_LF_CQ_OP_DOOR for those CQEs anyway.

Take an AF_XDP ZC socket that is transmitting while the interface goes
down or the XDP program is swapped:

otx2_xdp_setup()->otx2_stop()->otx2_free_hw_resources()->
    otx2_cleanup_tx_cqes()

Do the completed descriptors then never reach the xsk completion ring? If
so, userspace never gets those UMEM frames back. The completion ring slots
reserved by xsk_tx_peek_release_desc_batch() would also stay reserved
across down/up cycles.

sq->xsk_pool is still attached at this point. It is only cleared in
otx2_xsk_pool_disable(). Would adding something like this after the loop
work?

	if (sq->xsk_pool && xsk_frames)
		xsk_tx_completed(sq->xsk_pool, xsk_frames);

otx2_zc_submit_pkts() itself does not fit here, because it also submits
new TX packets.

[Severity: Medium]
This is a pre-existing issue, but it is on the same XDP teardown path.
otx2_free_pending_sqe() only walks the regular TX SQs:

	for (sq_idx = 0; sq_idx < pfvf->hw.tx_queues; sq_idx++) {

So nothing checks the XDP SQs for SQEs the NIC never completed.
otx2_sqb_flush() does not wait for the queue to drain. It sleeps once per
non-empty SQ:

	if (sqe_head != sqe_tail)
		usleep_range(50, 60);

XDP SQEs with no CQE can therefore still be there after
otx2_txschq_stop(), for example under PFC pause or link loss.

Is the xdp_frame in sg->skb ever returned for those entries? For
OTX2_XDP_REDIRECT entries, is the DMA_TO_DEVICE mapping created in
otx2_xdp_xmit_tx() ever unmapped before otx2_free_sq_res() frees sq->sg?

Widening the loop would not work as-is, because it uses
dev_kfree_skb_any(). The XDP SQs need an XDP-aware release.

[Severity: High]
This isn't a bug introduced by this patch. The commit message mentions
"when the XDP program is replaced". On that path, is ndo_xdp_xmit kept
away from the XDP SQ teardown?

otx2_xdp_setup() calls otx2_stop() directly and swaps the program only
afterwards:

	if (if_up)
		otx2_stop(pf->netdev);

	old_prog = xchg(&pf->xdp_prog, prog);

So during teardown netif_running() is still true and pf->xdp_prog is still
set. Those are the only two things otx2_xdp_xmit() checks:

	if (!netif_running(netdev))
		return -ENETDOWN;

	qidx += pf->hw.tx_queues;
	sq = pf->xdp_prog ? &pf->qset.sq[qidx] : NULL;

There is no OTX2_FLAG_INTF_DOWN check and no lock.
otx2_xdp_sq_append_pkt() also writes sq->sqe_base and sq->sg[] without
locking.

Suppose another device redirects into this one through a devmap
(xdp_do_flush()->bq_xmit_all()->ndo_xdp_xmit()) while otx2_stop() runs:

otx2_free_hw_resources()
    otx2_cleanup_tx_cqes()
    otx2_free_sq_res()      /* kfree(sq->sg), qmem_free(sq->sqe) */
otx2_free_queue_mem()       /* kfree(qset->sq); qset->sq = NULL */

Can the redirect path then write into freed sq->sg or SQE memory, or
dereference the NULL qset->sq?

otx2_stop() only synchronizes this device's IRQs and NAPIs, and there is
no synchronize_net(). Frames appended during teardown would also leak
their xdp_frame and DMA mapping.

[Severity: High]
This is a pre-existing issue, but can NAPI run at the same time as this
cleanup loop? otx2_stop() does:

	cq_poll = &qset->napi[qidx];
	napi_synchronize(&cq_poll->napi);
	...
	otx2_free_hw_resources(pf);
	otx2_free_cints(pf, pf->hw.cint_cnt);
	otx2_disable_napi(pf);

napi_synchronize() only waits for a poll that is already running. The
NAPI is still enabled, so socket or AF_XDP busy polling (napi_busy_loop())
can claim it again. It then processes the same XDP CQ:

otx2_napi_handler()
    otx2_tx_napi_handler()
        otx2_xdp_snd_pkt_handler()

This loop and the NAPI path both update cq->cq_head and cq->pend_cqe
without a lock. Could both pick up the same CQE and call
xdp_return_frame() twice on the same sg->skb? Or could one call
xdp_return_frame(NULL) after the other has cleared it?

There is a second window later in otx2_free_hw_resources():
otx2_free_cq_res() frees the CQE memory while busy poll can still run the
handler.

The same race existed before this patch for both the skb and XDP CQs.

[Severity: High]
This isn't a bug introduced by this patch. However, the new branch adds
another consumer of sg->skb values that may be bogus in AF_XDP zero-copy
mode.

In otx2_xdp_rcv_pkt_handler(), the ZC branch jumps past the
initialization of the local xdp_buff:

	if (pool->xsk_pool) {
		xsk_buff = pool->xdp[--cq->rbpool->xdp_top];
		...
		act = bpf_prog_run_xdp(prog, xsk_buff);
		goto handle_xdp_verdict;
	}

The XDP_TX case then uses &xdp rather than xsk_buff:

	case XDP_TX:
		...
		xdpf = xdp_convert_buff_to_frame(&xdp);
		return otx2_xdp_sq_append_pkt(pfvf, xdpf,

xdp_convert_buff_to_frame() reads xdp->rxq->mem.type and writes the frame
header at xdp->data_hard_start.

Doesn't this dereference uninitialized stack data on every XDP_TX verdict
in ZC mode?

The resulting pointer is stored in sq->sg[].skb with OTX2_XDP_TX. Both the
NAPI completion path and the new CQ_XDP branch later pass it to
xdp_return_frame().

The XDP_PASS case has a similar problem in ZC mode:

	case XDP_PASS:
		*metasize = xdp.data - xdp.data_meta;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030-nb-otx2-xdp-cqe-cleanup-net-v1-1-710635c0f504%40tipi-net.de