Re: [PATCH net 1/6] gve: increment work_done for XDP and error packets

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

New issues:
- [Medium] The patch inadvertently corrupts `rx->rpackets` statistics by counting descriptor errors, allocation failures, and XDP drops as successfully received packets, and creates a mismatch with `rx->rbytes`.

Pre-existing issues:
- [Critical] Missing state tracking for dropped multi-descriptor packets in the DQO RX path causes subsequent fragments to be treated as new packets, leading to payload corruption, bypass of XDP drop rules, and unsafe XDP execution on partial data.
--

--- Patch [1]: [PATCH net 1/6] gve: increment work_done for XDP and error packets ---
commit 705a50c36a672a5d7810174d670e9330d7aada5f
Author: Joshua Washington <[email protected]>

gve: increment work_done for XDP and error packets

Ensure every packet is accounted for in work_done by incrementing
work_done before checking for the existence of a SKB.

Cc: [email protected]

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 0ece2f6fdffb4..db38bc6452969 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -1145,13 +1145,14 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  		/* Free running counter of completed descriptors */
>  		rx->cnt++;
>  
> -		if (!rx->ctx.skb_head)
> -			continue;
> -
>  		if (!compl_desc->end_of_packet)
>  			continue;

[Severity: Critical]
This is a pre-existing issue, but does the polling loop lack state tracking
for dropped multi-descriptor packets?

If a multi-fragment packet is dropped (for example, by an XDP rule
in gve_rx_dqo()), rx->ctx.skb_head remains NULL. When the loop handles the
next fragment of that same packet, it skips to it because of the
end_of_packet check.

Since skb_head is NULL, gve_rx_dqo() will treat the middle fragment as a
new packet:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_rx_dqo() {
        ...
        /* Append to current skb if one exists. */
        if (rx->ctx.skb_head) {
        ...
}

Can this lead to XDP programs running on partial data and malicious inner
payloads bypassing XDP drop rules?

>  		work_done++;
> +
> +		if (!rx->ctx.skb_head)
> +			continue;
> +

[Severity: Medium]
Does this change inadvertently increment the interface packet statistics
for dropped packets?

By moving the work_done increment before the skb_head check, work_done
now correctly accounts for dropped packets, XDP drops, and allocation
failures in the NAPI budget.

However, work_done is later used to update rx->rpackets at the end of
gve_rx_poll_dqo():

        u64_stats_update_begin(&rx->statss);
        rx->rpackets += work_done;
        rx->rbytes += bytes;
        u64_stats_update_end(&rx->statss);

Will this cause dropped packets to be incorrectly counted as successfully
received packets, causing the rpackets stat to diverge from rbytes?

>  		pkt_bytes = rx->ctx.skb_head->len;
>  		/* The ethernet header (first ETH_HLEN bytes) is snipped off

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1