Re: [PATCH net-next v3 1/2] gve: use xdp_build_skb methods for XDP_PASS case

[Joshua Washington]

On Wed, Jul 22, 2026 at 3:16 PM Joshua Washington <joshwash@xxxxxxxxxx> wrote:
>
> Newer common methods have been introduced to construct SKBs in the
> event of XDP_PASS because many drivers replicated very similar
> functionality. Update GVE to use these common methods for copy mode and
> zero-copy mode.
>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> Reviewed-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> Signed-off-by: Joshua Washington <joshwash@xxxxxxxxxx>
> ---
> v3:
>   - Newly introduced. Ensures that XDP metadata is passed onto SKB in
>     XDP_PASS case.
> ---
>  drivers/net/ethernet/google/gve/gve_rx_dqo.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 02cba280d81a..907a0c15bf30 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -736,8 +736,7 @@ static int gve_rx_xsk_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>         }
>
>         /* Copy the data to skb */
> -       rx->ctx.skb_head = gve_rx_copy_data(priv->dev, napi,
> -                                           xdp->data, buf_len);
> +       rx->ctx.skb_head = xdp_build_skb_from_zc(xdp);

Sashiko says:
---
[Severity: High]
This isn't a bug introduced by this patch, but does this unconditionally
overwrite the SKB head on every fragment for multi-fragment packets?

If an AF_XDP enabled interface receives a jumbo frame (e.g., 9000 bytes),
the hardware splits the packet across multiple RX descriptors. For each
descriptor, gve_rx_xsk_dqo() allocates a new SKB here and assigns it to
rx->ctx.skb_head.

Since this ignores whether rx->ctx.skb_head is already populated from previous
fragments, does it leak the SKBs of prior fragments and truncate the delivered
packet?
---

Multi-buffer XDP is not supported in GVE DQ.

>         if (unlikely(!rx->ctx.skb_head)) {
>                 xsk_buff_free(xdp);
>                 gve_free_buf_state(rx, buf_state);
> @@ -745,8 +744,6 @@ static int gve_rx_xsk_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>         }
>         rx->ctx.skb_tail = rx->ctx.skb_head;
>
> -       /* Free XSK buffer and Buffer state */
> -       xsk_buff_free(xdp);
>         gve_free_buf_state(rx, buf_state);
>
>         /* Update Stats */
> @@ -899,9 +896,17 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>                         return 0;
>                 }
>
> +               rx->ctx.skb_head = xdp_build_skb_from_buff(&gve_xdp.xdp);
> +               if (unlikely(!rx->ctx.skb_head))
> +                       goto error;
> +               rx->ctx.skb_tail = rx->ctx.skb_head;
> +
> +               gve_reuse_buffer(rx, buf_state);
> +
>                 u64_stats_update_begin(&rx->statss);
>                 rx->xdp_actions[XDP_PASS]++;
>                 u64_stats_update_end(&rx->statss);
> +               return 0;

Sashiko says:
---
[Severity: High]
Does returning 0 here bypass the copy-on-demand safety check for XDP_PASS
packets?

The gve_rx_should_trigger_copy_ondemand() check at the end of gve_rx_dqo()
appears to be skipped. By bypassing this check, could the networking stack
hold onto the QPL pages indefinitely?

This might drive used_buf_states_cnt to num_buf_states, exhausting the buffer
pool and preventing new RX buffers from being posted to hardware, ultimately
starving the RX ring.
---

DQ QPL doesn't have XDP support, so QPL pages are not in use.

>         }
>
>         if (eop && buf_len <= priv->rx_copybreak &&
> --
> 2.55.0.229.g6434b31f56-goog
>


Sashiko says:
---
> @@ -894,14 +891,22 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>               buf_state->page_info.pad += gve_xdp.xdp.data - old_data;
>               buf_len = gve_xdp.xdp.data_end - gve_xdp.xdp.data;
>               if (xdp_act != XDP_PASS) {
>                       gve_xdp_done_dqo(priv, rx, &gve_xdp.xdp, xprog, xdp_act,
>                                        buf_state);
>                       return 0;
>               }

[Severity: High]
This is a pre-existing issue, but if the first fragment of a multi-descriptor
packet is dropped (e.g. XDP_DROP), does the driver fail to track that the rest
of the packet must be dropped?

When the next descriptor containing the middle of the payload arrives,
rx->ctx.skb_head will be NULL. Will the driver incorrectly treat this fragment
as a new Ethernet frame and run the XDP program on arbitrary payload data?
---

Again, GVE DQ doesn't support multi-buffer XDP. This will be addressed
when support is added.

-- 

Joshua Washington | Software Engineer | joshwash@xxxxxxxxxx | (414) 366-4423