Re: [PATCH net 2/6] gve: fix XSK buffer leak when rings are stopped

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

Pre-existing issues:
- [High] XSK buffers are permanently leaked during runtime when the received packet descriptor indicates an rx_error.
- [High] Calling `dev_consume_skb_any()` on an XDP frame and incorrectly unmapping DMA for AF_XDP zero-copy buffers during pending packet cleanup.
- [Medium] AF_XDP (XSK) TX buffers are permanently leaked from the user-space application when the TX queue is stopped.
--

--- Patch [2]: [PATCH net 2/6] gve: fix XSK buffer leak when rings are stopped ---
commit 68d9940f45b02ca459717266e19904c11302ff2d
Author: Joshua Washington <[email protected]>

gve: fix XSK buffer leak when rings are stopped
    
GVE does not free XSK buffers when resetting ring state as a part of
stopping queues. This causes all XSK buffers which are posted to the
NIC to be leaked.
    
Free XSK buffers attached to an allocated buf_state when stopping rings.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index db38bc6452969..2c1ce23d15508 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -113,6 +113,12 @@ static void gve_rx_reset_ring_dqo(struct gve_priv *priv, int idx)
>  				gve_free_to_page_pool(rx, bs, false);
>  			else
>  				gve_free_qpl_page_dqo(bs);
> +
> +			if (gve_buf_state_is_allocated(rx, bs) &&
> +			    bs->xsk_buff) {
> +				xsk_buff_free(bs->xsk_buff);
> +				bs->xsk_buff = NULL;
> +			}

[Severity: High]
This is a pre-existing issue, but does this cleanup loop miss leaked XSK
buffers if they were previously freed due to an rx_error?

When a packet is received with compl_desc->rx_error set (such as a CRC error),
gve_rx_dqo() calls gve_free_buffer() before checking for the XSK buffer:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_rx_dqo() {
    ...
    if (unlikely(compl_desc->rx_error)) {
        gve_free_buffer(rx, buf_state);
        return -EINVAL;
    }
    ...
}

gve_free_buffer() returns the buffer state to the free list without calling
xsk_buff_free(), leaving bs->xsk_buff still set. 

Since this cleanup loop checks gve_buf_state_is_allocated(), which evaluates
to false for buffers in the free list, will it completely skip these leaked
buffers?

>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while looking at queue stop
cleanups, is there a type confusion issue when cleaning up pending TX packets?

During interface down or MTU changes, gve_tx_clean_pending_packets() and
remove_miss_completions() iterate over pending TX packets and unconditionally
treat them as SKBs:

drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_tx_clean_pending_packets() {
    ...
    gve_unmap_packet(tx->dev, cur_state);

    if (cur_state->skb) {
        dev_consume_skb_any(cur_state->skb);
        cur_state->skb = NULL;
    }
    ...
}

struct gve_tx_pending_packet_dqo uses a union for skb and xdpf. If the packet
is an XDP frame, cur_state->skb will evaluate to true, passing the XDP frame
to dev_consume_skb_any(). Can this cause a crash?

Additionally, for AF_XDP (XSK) packets, gve_xsk_tx_dqo() does not initialize
dma[0] because XSK uses pre-mapped DMA. Will the unconditional call to
gve_unmap_packet() pass uninitialized or stale dma[0] values to
dma_unmap_single() and corrupt DMA mappings?

[Severity: Medium]
This is a pre-existing issue, but do AF_XDP TX buffers get permanently leaked
from the userspace application when the TX ring is stopped?

The gve_tx_clean_pending_packets() loop traverses and discards all pending
packets:

drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_tx_clean_pending_packets() {
    ...
    for (i = 0; i < tx->dqo.num_pending_packets; i++) {
        struct gve_tx_pending_packet_dqo *cur_state =
            &tx->dqo.pending_packets[i];
    ...
}

Unlike the normal completion path in gve_tx_process_xsk_completions(), this
cleanup path doesn't appear to call xsk_tx_completed() for XSK packets before
dropping them. 

Since the pending packets array is zeroed and freed shortly after, are these
buffers lost permanently from the perspective of the AF_XDP socket?

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