Re: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i40e: prepare for XDP metadata ops support

This adds struct i40e_xdp_buff, which wraps the xdp_buff stored on the Rx
ring together with a pointer to the Rx descriptor, and sets that pointer
before running the XDP program in both the normal and the AF_XDP zero-copy
receive paths.  The ZC path also gains XSK_CHECK_PRIV_TYPE() to verify the
new struct fits in the xdp_buff_xsk cb area.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 0cd0e5597c909..4b11ae9ed8d53 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3622,7 +3622,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
>  	}
>  
>  skip:
> -	xdp_init_buff(&ring->xdp, xdp_frame_sz, &ring->xdp_rxq);
> +	xdp_init_buff(&ring->xdp_ctx.xdp, xdp_frame_sz, &ring->xdp_rxq);

[Severity: High]
This isn't a bug introduced by this patch, but can the state persisted in
rx_ring->xdp_ctx outlive the pages it points at?

i40e_clean_rx_ring() unmaps and drains every Rx page and resets the
next_to_* indices, but leaves the saved buffer alone:

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_rx_ring() {
	...
		__page_frag_cache_drain(rx_bi->page, rx_bi->pagecnt_bias);

		rx_bi->page = NULL;
		rx_bi->page_offset = 0;
	}
skip_free:
	...
	rx_ring->next_to_alloc = 0;
	rx_ring->next_to_clean = 0;
	rx_ring->next_to_process = 0;
	rx_ring->next_to_use = 0;
}

On bring-up the only re-initialization is the xdp_init_buff() call above,
and that touches rxq, frame_sz and flags only:

include/net/xdp.h:xdp_init_buff() {
	xdp->rxq = rxq;
	...
	xdp->frame_sz_flags_init = frame_sz;
	...
}

So data, data_hard_start and the frag state survive an ifdown/ifup, MTU
change, ring resize or PF reset.  If NAPI returned in the middle of a
multi-buffer packet (the state the comment above the member says is kept on
purpose), does the first iteration after the ring comes back up then take
the fragment-append branch on stale data?

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_rx_irq() {
	...
		if (!xdp->data) {
			...
			xdp_prepare_buff(xdp, hard_start, offset, size, true);
		} else if (i40e_add_xdp_frag(xdp, &nfrags, rx_buffer, size) &&
			   !neop) {
	...
}

i40e_add_xdp_frag() derives sinfo from data_hard_start and writes nr_frags,
xdp_frags_size and a frag entry, which would land in a page already handed
back by __page_frag_cache_drain(), and the following i40e_build_skb() /
i40e_process_rx_buffs() would then operate on and drop a reference to that
page.

ice does clear the stash in this situation: ice_clean_rx_ring() calls
libeth_xdp_return_stash(&rx_ring->xdp), described as freeing an incomplete
buffer stash when the queue is about to be destroyed.  Would clearing
xdp_ctx (data = NULL and the new desc = NULL) in i40e_clean_rx_ring() be
appropriate here?

The new desc pointer inherits the same lifetime problem: it is a raw
pointer into the descriptor ring that i40e_free_rx_resources() releases via
dma_free_coherent(), and it is only assigned on the EOP path, never
cleared.

>  
>  	rx_ctx.dbuff = DIV_ROUND_UP(ring->rx_buf_len,
>  				    BIT_ULL(I40E_RXQ_CTX_DBUFF_SHIFT));
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index ef5e657816f00..7f68adb5109b0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c

[ ... ]

> @@ -2530,6 +2531,8 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget,
>  		if (neop)
>  			continue;
>  
> +		xdp_ctx->desc = rx_desc;
> +
>  		xdp_res = i40e_run_xdp(rx_ring, xdp, xdp_prog);
>  
>  		if (xdp_res) {
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index 1e5fd63d47f47..bb741ff3e5f2c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -283,6 +283,11 @@ struct i40e_rx_buffer {
>  	__u32 page_count;
>  };
>  
> +struct i40e_xdp_buff {
> +	struct xdp_buff xdp;
> +	const union i40e_rx_desc *desc;
> +};
> +
>  struct i40e_queue_stats {
>  	u64 packets;
>  	u64 bytes;
> @@ -345,7 +350,7 @@ struct i40e_ring {
>  	 * and to resume packet building for this ring in the next call to
>  	 * i40e_clean_rx_ring_irq().
>  	 */
> -	struct xdp_buff xdp;
> +	struct i40e_xdp_buff xdp_ctx;

[Severity: Low]
This is a pre-existing issue, not something this patch introduced, but the
comment just above this member names i40e_clean_rx_ring_irq() twice and
that function does not exist; git grep for it matches only these two
comment lines.  The function that saves and resumes this state is
i40e_clean_rx_irq().  Since the hunk already touches the member, would it
make sense to fix the name, and also the "Storing xdp_buff on ring" wording
now that the member is a struct i40e_xdp_buff?

While there, could the lifetime of the new desc field be described?  It is
set only on the EOP path in i40e_clean_rx_irq() and is not initialized by
the xdp_init_buff() call in i40e_configure_rx_ring().

>  
>  	/* Next descriptor to be processed; next_to_clean is updated only on
>  	 * processing EOP descriptor
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 9f47388eaba53..51a05ce4c7ce8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c

[ ... ]

> @@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  		if (i40e_is_non_eop(rx_ring, rx_desc))
>  			continue;
>  
> +		xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
> +
>  		xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
>  		i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
>  					  &rx_bytes, xdp_res, &failure);

[Severity: High]
This isn't a bug introduced by this patch either, but can "first" here
already be a freed xsk buffer, so that this new store writes into the cb of
a buffer that is back on the pool free list?

Two places in i40e_clean_rx_irq_zc() free a buffer without advancing
next_to_clean and without clearing the rx_bi_zc slot.  The programming
status branch:

		if (i40e_rx_is_programming_status(qword)) {
			...
			bi = *i40e_rx_bi(rx_ring, next_to_process);
			xsk_buff_free(bi);
			if (++next_to_process == count)
				next_to_process = 0;
			continue;
		}

and the multi-buffer error path:

		if (!first)
			first = bi;
		else if (!xsk_buff_add_frag(first, bi)) {
			xsk_buff_free(first);
			break;
		}

If the loop then exits at:

		size = FIELD_GET(I40E_RXD_QW1_LENGTH_PBUF_MASK, qword);
		if (!size)
			break;

the function writes back rx_ring->next_to_clean unchanged while
next_to_process has moved on, and i40e_alloc_rx_buffers_zc() cannot refill
that slot because I40E_DESC_UNUSED stops at next_to_clean - 1.  So does
rx_bi_zc[next_to_clean] keep pointing at a freed buffer that the next poll
picks up?

	if (next_to_process != next_to_clean)
		first = *i40e_rx_bi(rx_ring, next_to_clean);

That buffer is then passed to xsk_buff_set_size(), xsk_buff_add_frag() and
i40e_run_xdp_zc(), and with this patch also written through by
xsk_buff_to_i40e_ctx(first)->desc = rx_desc.

On teardown, does the same stale pointer get freed a second time?
i40e_xsk_clean_rx_ring() walks [next_to_clean, next_to_use):

	while (ntc != ntu) {
		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);

		xsk_buff_free(rx_bi);

and it runs before i40e_clear_rx_bi_zc() zeroes the array.

ice_clean_rx_irq_zc() frees, clears first, and still advances ntc:

		} else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
			xsk_buff_free(first);
			first = NULL;
		}

		if (++ntc == cnt)
			ntc = 0;

Would the same handling be right for i40e?

The multi-buffer error leg looks hard to reach on this hardware, since
xsk_buff_add_frag() only fails at MAX_SKB_FRAGS while rx_ctx.rxmax is
programmed from I40E_MAX_CHAINED_RX_BUFFERS, but the programming status leg
needs no oversized frame.