Re: [PATCH net v2 9/9] gve: prevent XDP frame leak and corruption during DQO TX cleanup

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch still unmaps XSK pending packets as if the driver
  owned their DMA mapping, so it never closes the bug class its message…
- [Medium] Now that gve_handle_miss_completion() no longer crashes on XSK
  packets, those packets can reach the late-reinjection branch of…
- [Medium] The new XSK teardown handling in
  gve_tx_clean_pending_packets() never runs when XSK packets can…
- [Low] The commit message describes only XDP frames being mistaken for
  SKBs, but the patch also changes AF_XDP zero-copy (XSK) behaviour…
- [Low] The comments were not updated to match the rewritten cleanup
  code.

--

gve: prevent XDP frame leak and corruption during DQO TX cleanup

This commit changes gve_tx_clean_pending_packets(),
gve_handle_miss_completion() and remove_miss_completions() to switch on
pending_packet->type, so XDP frames are returned with xdp_return_frame()
instead of being handled as SKBs. It also marks XSK pending packets
complete during teardown. It moves the XSK reorder queue helpers earlier
in the file so the cleanup path can call gve_tx_process_xsk_completions().

> When tearing down a DQO TX ring or processing miss completions,
> pending packets were assumed to be SKBs. If an XDP frame was pending
> during cleanup or timeout, cur_state->skb was accessed on an xdpf
> union pointer or the XDP frame was leaked without calling
> xdp_return_frame().
>
> Refactor gve_tx_clean_pending_packets(), gve_handle_miss_completion(),
> and remove_miss_completions() to switch on pending_packet->type.
>
> Fixes: d8a8ca14c937 ("gve: add XDP_TX and XDP_REDIRECT support for DQ RDA")

[Severity: Low]
The commit message only talks about XDP frames, but the patch also
changes AF_XDP zero-copy (XSK) behaviour. Could the message describe that
as well?

Before this patch, gve_handle_miss_completion() did:

	*bytes += pending_packet->skb->len;

For XSK pending packets the skb/xdpf union is always NULL, because
gve_xsk_tx_dqo() never sets it. So a miss completion on an XSK packet
would be a NULL pointer dereference in NAPI context. The new XSK case
fixes that crash.

That code came from 2236836eab26 ("gve: implement DQO TX datapath for
AF_XDP zero-copy"), which added GVE_TX_PENDING_PACKET_DQO_XSK. Should
that commit get a Fixes: tag too?

The message also doesn't mention two other changes. At teardown,
gve_tx_clean_pending_packets() now marks XSK slots XSK_COMPLETE and tries
to report them through xsk_tx_completed(). And gve_xsk_reorder_queue_head(),
gve_xsk_reorder_queue_pop_dqo() and gve_tx_process_xsk_completions() have
been moved.

Both commits landed in the same development cycle, so this is mostly
about traceability.

> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index 78f946ae72649..e8481b993beb0 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c

[ ... ]

> @@ -201,11 +244,30 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
>  		else
>  			gve_unmap_packet(tx->dev, cur_state);
>  
> -		if (cur_state->skb) {
> -			dev_consume_skb_any(cur_state->skb);
> -			cur_state->skb = NULL;
> +		switch (cur_state->type) {
> +		case GVE_TX_PENDING_PACKET_DQO_SKB:

[ ... ]

> +		case GVE_TX_PENDING_PACKET_DQO_XSK:
> +			cur_state->state = GVE_PACKET_STATE_XSK_COMPLETE;
> +			break;

[Severity: High]
The gve_unmap_packet() call above the switch still runs for every slot,
XSK slots included. Can this unmap a DMA address the driver doesn't own?

gve_unmap_packet() assumes the driver mapped the slot:

	/* SKB linear portion is guaranteed to be mapped */
	dma_unmap_single(dev, dma_unmap_addr(pkt, dma[0]),
			 dma_unmap_len(pkt, len[0]), DMA_TO_DEVICE);

For XSK, gve_xsk_tx_dqo() posts a pool-owned address and increments
num_bufs, but it never records dma[0]/len[0]:

	pkt->type = GVE_TX_PENDING_PACKET_DQO_XSK;
	pkt->num_bufs = 0;
	...
	addr = xsk_buff_raw_get_dma(pool, desc.addr);
	...
	++pkt->num_bufs;

The XSK case in gve_handle_packet_completion() only sets
GVE_PACKET_STATE_XSK_COMPLETE, and gve_free_pending_packet() doesn't
reset num_bufs. A completed XSK slot therefore keeps num_bufs == 1, along
with whatever dma[0] an earlier user of the slot left behind.

On the XDP TX queue, the same slot can first carry an XDP frame from
gve_xdp_xmit_one_dqo(). That fills dma[0] with a real mapping, which is
unmapped at completion. If the slot is then reused for XSK, a later
ifdown, reset, or XSK pool enable/disable (via gve_restart_rings())
reaches:

gve_queues_stop()
  gve_tx_stop_rings()
    gve_tx_stop_ring_dqo()
      gve_tx_clean_pending_packets()
        gve_unmap_packet()
          dma_unmap_single(dma[0])   <- already unmapped

Slots that were never used for anything else would instead unmap address
0 with length 0.

Under swiotlb or an IOMMU, could this release a bounce slot or IOVA that
now belongs to another mapping?

remove_miss_completions() has the same type-blind unmap right above its
new XSK case. Before this patch, an XSK miss completion crashed in
gve_handle_miss_completion(). Now the packet reaches
remove_miss_completions() after GVE_REINJECT_COMPL_TIMEOUT and hits the
stale unmap at runtime.

Would it be better to move the unmap into the SKB and XDP_FRAME cases,
the way gve_handle_packet_completion() already does?

> +		default:
> +			WARN_ON_ONCE(1);
> +			break;
>  		}
>  	}
> +
> +	if (tx->xsk_pool)
> +		gve_tx_process_xsk_completions(tx);
>  }

[Severity: Medium]
Can this drain ever run while XSK packets are pending?

gve_queues_stop() clears the XSK pool pointers before it stops the TX
rings:

gve_main.c:gve_queues_stop() {
	...
	gve_unreg_xdp_info(priv);

	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	...
}

gve_unreg_xdp_info() calls gve_unreg_xsk_pool(), which does:

	priv->tx[gve_xdp_tx_queue_id(priv, qid)].xsk_pool = NULL;

So when gve_tx_stop_ring_dqo() calls gve_clean_tx_done_dqo() with a NULL
napi and then gve_tx_clean_pending_packets(), tx->xsk_pool is already
NULL and both drains are skipped. gve_tx_reset_ring_dqo() then zeroes
xsk_reorder_queue_head/tail and the reorder queue.

As a result, outstanding XSK descriptors are never passed to
xsk_tx_completed(). That includes descriptors the device completed during
the final gve_clean_tx_done_dqo() pass.

gve_queues_stop() is used by gve_close(), gve_restart_rings() and the RDA
XSK pool enable/disable paths. In those paths, AF_XDP sockets on other
queues stay bound. Won't their completion rings lose entries on every
stop/start that had TX in flight?

The remaining caller is the gve_queues_start() error path, which runs
before any XSK packets could be pending. So the new drain looks like it
can never run in practice.

[ ... ]

> @@ -1347,8 +1387,25 @@ static void gve_handle_miss_completion(struct gve_priv *priv,
>  			secs_to_jiffies(GVE_REINJECT_COMPL_TIMEOUT);
>  	add_to_list(tx, &tx->dqo_compl.miss_completions, pending_packet);
>  
> -	*bytes += pending_packet->skb->len;
> -	(*pkts)++;
> +	switch (pending_packet->type) {

[ ... ]

> +	case GVE_TX_PENDING_PACKET_DQO_XSK:
> +		break;

[Severity: Medium]
XSK packets now get past this point instead of crashing. Can they reach
the late reinjection branch of gve_handle_packet_completion(), which frees
the slot without checking its type?

After GVE_REINJECT_COMPL_TIMEOUT, remove_miss_completions() moves the XSK
packet to GVE_PACKET_STATE_TIMED_OUT_COMPL. Its completion tag stays in
tx->dqo.xsk_reorder_queue. If the reinjection completion then arrives
before GVE_DEALLOCATE_COMPL_TIMEOUT:

gve_handle_packet_completion() {
	...
		if (unlikely(pending_packet->state ==
			     GVE_PACKET_STATE_TIMED_OUT_COMPL)) {
			...
			remove_from_list(tx,
					 &tx->dqo_compl.timed_out_completions,
					 pending_packet);
			gve_free_pending_packet(tx, pending_packet);
			return;
		}
	...
}

The slot becomes GVE_PACKET_STATE_UNALLOCATED while the reorder queue
still points at it. gve_tx_process_xsk_completions() stops at the first
entry that isn't XSK_COMPLETE. Wouldn't every later XSK completion on this
queue then be held back?

This doesn't look like a double free, since gve_free_pending_packet() sets
the slot to UNALLOCATED. The queue only unblocks if that exact slot is
later reused for XSK and completes. Even then, a second reorder entry
points at the same slot, so one entry stays behind for good.

If the AF_XDP completion ring fills while the queue is blocked,
xsk_tx_peek_desc() stops handing out descriptors. TX on that queue could
then stall until the rings are restarted.

remove_timed_out_completions() already handles this case:

		if (pending_packet->type == GVE_TX_PENDING_PACKET_DQO_XSK)
			pending_packet->state = GVE_PACKET_STATE_XSK_COMPLETE;
		else
			gve_free_pending_packet(tx, pending_packet);

Should the late reinjection branch do the same?

> @@ -1377,9 +1434,26 @@ static void remove_miss_completions(struct gve_priv *priv,
>  		else
>  			gve_unmap_packet(tx->dev, pending_packet);
>  
> -		/* This indicates the packet was dropped. */
> -		dev_kfree_skb_any(pending_packet->skb);
> -		pending_packet->skb = NULL;
> +		switch (pending_packet->type) {
> +		case GVE_TX_PENDING_PACKET_DQO_SKB:
> +			if (pending_packet->skb) {
> +				/* This indicates the packet was dropped. */
> +				dev_kfree_skb_any(pending_packet->skb);
> +				pending_packet->skb = NULL;
> +			}
> +			break;
> +		case GVE_TX_PENDING_PACKET_DQO_XDP_FRAME:

[Severity: Low]
This isn't a bug, but the comments here no longer match the code. The
comment above the unmap in remove_miss_completions() still says:

		/* Unmap/free TX buffers and free skb but do not unallocate packet i.e.

The code now also returns XDP frames and skips XSK packets.

The "This indicates the packet was dropped" comment has also moved into
the SKB case only. The XDP_FRAME case is the same drop and also
increments tx->dropped_pkt.

Separately, the header comment above gve_tx_clean_pending_packets()
still names a function that doesn't exist:

/* gve_tx_free_desc - Cleans up all pending tx requests and buffers.
 */

That name predates this patch. Since the function is being rewritten
here anyway, could it be fixed as well?

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