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

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.

[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.

[ ... ]

[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().

[ ... ]

[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.