Re: [PATCH net-next v6 6/7] net: macb: add XDP support for gem

[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.
---
net: macb: add XDP support for gem

This commit introduces basic XDP support for macb/gem with the XDP_TX,
XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4c5dd70f003b..537d02264851 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -1197,6 +1198,18 @@ static int macb_halt_tx(struct macb *bp)
>  					bp, TSR);
>  }
>  
> +static void macb_tx_release_buff(void *buff, enum macb_tx_buff_type type, int budget)
> +{
> +	if (type == MACB_TYPE_SKB) {
> +		napi_consume_skb(buff, budget);
> +	} else {
> +		if (!budget)
> +			xdp_return_frame(buff);
> +		else
> +			xdp_return_frame_rx_napi(buff);
> +	}
> +}

Does this code cause memory corruption in the page pool lockless cache?
This function is called from macb_tx_complete(), which executes in the
context of the TX NAPI poll routine (queue->napi_tx).

However, the page pool was created and bound to the RX NAPI context
(queue->napi_rx). Since xdp_return_frame_rx_napi() bypasses safety checks
and places the page directly into the pool's lockless cache, and these are
independent NAPI instances that can be scheduled concurrently on different
CPUs, shouldn't this use xdp_return_frame() to safely handle cross-CPU
synchronization?

[ ... ]
> +static int macb_xdp_submit_frame(struct macb *bp, struct xdp_frame *xdpf,
> +				 struct net_device *dev, dma_addr_t addr)
> +{
[ ... ]
> +	tx_buff->ptr = xdpf;
> +	tx_buff->type = MACB_TYPE_XDP_TX;
> +	tx_buff->mapping = 0;
> +	tx_buff->size = xdpf->len;
> +	tx_buff->mapped_as_page = false;
> +
> +	ctrl = (u32)tx_buff->size;
> +	ctrl |= MACB_BIT(TX_LAST);

Could this write overwrite control bits in the descriptor if the XDP frame
length exceeds the hardware's maximum limit?

The hardware uses a 14-bit field for the frame length, with higher bits
reserved for control flags like TX_LAST and TX_WRAP. On architectures with
a PAGE_SIZE larger than 16KB (e.g., ARM64 ZynqMP with 64KB pages), an XDP
program can use bpf_xdp_adjust_tail() to legitimately increase the frame
length beyond this boundary.

Does this need a bounds check against the hardware's maximum supported
transmit size to prevent corrupted DMA operations?

[ ... ]
> @@ -1683,6 +1878,18 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  				queue->stats.rx_dropped++;
>  			}
>  
> +			headroom = bp->rx_headroom;
> +
> +			if (last_frame) {
> +				ret = gem_xdp_run(queue, buff_head, &data_len,
> +						  &headroom, addr - gem_rx_pad(bp));
> +				if (ret == XDP_REDIRECT)
> +					xdp_flush = true;
> +
> +				if (ret != XDP_PASS)
> +					goto next_frame;
> +			}
> +
>  			queue->skb = napi_build_skb(buff_head, gem_total_rx_buffer_size(bp));

Will this sequence allow multi-descriptor RX packets to silently bypass the
XDP program?

If a received packet spans multiple hardware descriptors (first_frame is true
but last_frame is false), the code skips gem_xdp_run() completely. It then
proceeds to allocate an sk_buff using napi_build_skb(), append subsequent
fragments on following loop iterations, and pass the assembled packet to the
network stack.

Shouldn't the driver explicitly drop packets that span multiple descriptors
when an XDP program is attached?