Re: [net-next,v2,3/6] net: bcmgenet: add basic XDP support (PASS/DROP)

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: bcmgenet: add basic XDP support (PASS/DROP)

This commit adds XDP program attachment via ndo_bpf and executes XDP
programs in the RX path. XDP_PASS builds an SKB from the xdp_buff
(handling xdp_adjust_head/tail), and XDP_DROP returns the page to
page_pool without SKB allocation.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 6e610e73e12f..f6a2567af7cb 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2274,6 +2276,53 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
>  	return 0;
>  }
>
> +static struct sk_buff *bcmgenet_xdp_build_skb(struct bcmgenet_rx_ring *ring,
> +					      struct xdp_buff *xdp,
> +					      struct page *rx_page)
> +{
> +	unsigned int metasize;
> +	struct sk_buff *skb;
> +
> +	skb = napi_build_skb(xdp->data_hard_start, PAGE_SIZE);
> +	if (unlikely(!skb))
> +		return NULL;
> +
> +	skb_mark_for_recycle(skb);
> +
> +	metasize = xdp->data - xdp->data_meta;
> +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> +	__skb_put(skb, xdp->data_end - xdp->data);
> +
> +	if (metasize)
> +		skb_metadata_set(skb, metasize);
                          ^^^^

Can this corrupt memory? Looking at bcmgenet_desc_rx(), the xdp_buff is
initialized with meta_valid=false:

	xdp_prepare_buff(&xdp, page_address(rx_page),
			 GENET_RX_HEADROOM, pkt_len, false);

When meta_valid is false, xdp_prepare_buff() sets xdp->data_meta to
xdp->data + 1 as a sentinel value to indicate no metadata is present.

In bcmgenet_xdp_build_skb(), the metasize calculation becomes:

	metasize = xdp->data - xdp->data_meta
	         = xdp->data - (xdp->data + 1)
	         = (ptrdiff_t)-1

When assigned to 'unsigned int metasize', this wraps to UINT_MAX. The
'if (metasize)' check passes since UINT_MAX != 0, and
skb_metadata_set(skb, metasize) truncates UINT_MAX to u8, setting
skb_shinfo(skb)->meta_len = 255.

This incorrect meta_len of 255 can cause downstream SKB consumers (TC BPF
programs, skb_data_move) to read or write 255 bytes before
skb_mac_header(skb) when accessing the metadata region.

Other XDP drivers that use skb_metadata_set() (bnxt, mvneta, ixgbe,
i40e, virtio_net) all pass meta_valid=true to xdp_prepare_buff(), which
sets data_meta=data, giving metasize=0 and correctly skipping
skb_metadata_set().

Should xdp_prepare_buff() be called with meta_valid=true instead of
false, since bcmgenet_xdp_build_skb() handles XDP metadata?

> +
> +	return skb;
> +}

[ ... ]

> @@ -2403,26 +2455,52 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  			goto next;
>  		} /* error packet */
>
> -		/* Build SKB from the page - data starts at hard_start,
> -		 * frame begins after RSB(64) + pad(2) = 66 bytes.
> -		 */
> -		skb = napi_build_skb(hard_start, PAGE_SIZE - GENET_XDP_HEADROOM);
> -		if (unlikely(!skb)) {
> -			BCMGENET_STATS64_INC(stats, dropped);
> -			page_pool_put_full_page(ring->page_pool, rx_page,
> -						true);
> -			goto next;
> -		}
> -
> -		skb_mark_for_recycle(skb);
> +		/* XDP: frame data starts after RSB + pad */
> +		if (xdp_prog) {
> +			struct xdp_buff xdp;
> +			unsigned int xdp_act;
> +			int pkt_len;
> +
> +			pkt_len = len - GENET_RSB_PAD;
> +			if (priv->crc_fwd_en)
> +				pkt_len -= ETH_FCS_LEN;
> +
> +			xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> +			xdp_prepare_buff(&xdp, page_address(rx_page),
> +					 GENET_RX_HEADROOM, pkt_len, false);
> +
> +			xdp_act = bcmgenet_run_xdp(ring, xdp_prog, &xdp,
> +						   rx_page);
> +			if (xdp_act != XDP_PASS)
> +				goto next;
> +
> +			/* XDP_PASS: build SKB from (possibly modified) xdp */
> +			skb = bcmgenet_xdp_build_skb(ring, &xdp, rx_page);
> +			if (unlikely(!skb)) {
> +				BCMGENET_STATS64_INC(stats, dropped);
> +				page_pool_put_full_page(ring->page_pool,
> +							rx_page, true);
> +				goto next;
> +			}
> +		} else {
> +			/* Build SKB from the page - data starts at
> +			 * hard_start, frame begins after RSB(64) + pad(2).
> +			 */
> +			skb = napi_build_skb(hard_start,
> +					     PAGE_SIZE - GENET_XDP_HEADROOM);
> +			if (unlikely(!skb)) {
> +				BCMGENET_STATS64_INC(stats, dropped);
> +				page_pool_put_full_page(ring->page_pool,
> +							rx_page, true);
> +				goto next;
> +			}
>
> -		/* Reserve the RSB + pad, then set the data length */
> -		skb_reserve(skb, GENET_RSB_PAD);
> -		__skb_put(skb, len - GENET_RSB_PAD);
> +			skb_mark_for_recycle(skb);
> +			skb_reserve(skb, GENET_RSB_PAD);
> +			__skb_put(skb, len - GENET_RSB_PAD);
>
> -		if (priv->crc_fwd_en) {
> -			skb_trim(skb, skb->len - ETH_FCS_LEN);
> -			len -= ETH_FCS_LEN;
> +			if (priv->crc_fwd_en)
> +				skb_trim(skb, skb->len - ETH_FCS_LEN);
>  		}

[ ... ]