Re: [PATCH net-next v9 3/7] net: bcmgenet: add basic XDP support (PASS/DROP)

[Jakub Kicinski]

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: bcmgenet: add basic XDP support (PASS/DROP)

This patch adds XDP program attachment via ndo_bpf and executes XDP
programs in the RX path, handling XDP_PASS by building an SKB from the
xdp_buff and XDP_DROP by returning the page to the page pool.
NETDEV_XDP_ACT_BASIC is advertised in xdp_features.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 5bedc18685b0..ee1d4ecc2b87 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2326,9 +2381,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  	       (rxpktprocessed < budget)) {
>  		struct status_64 *status;
>  		struct page *rx_page;
> +		unsigned int xdp_act;
>  		unsigned int rx_off;
> -		__be16 rx_csum;
> +		struct xdp_buff xdp;
> +		__be16 rx_csum = 0;
>  		void *hard_start;
> +		int pkt_len;
>  
>  		cb = &priv->rx_cbs[ring->read_ptr];
>  
> @@ -2413,30 +2471,34 @@ 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.
> +		pkt_len = len - GENET_RSB_PAD;
> +		if (priv->crc_fwd_en)
> +			pkt_len -= ETH_FCS_LEN;

Can this produce a negative pkt_len on short frames?

The earlier sanity check only rejects len < GENET_RSB_PAD:

    if (unlikely(len > RX_BUF_LENGTH || len < GENET_RSB_PAD)) {
        ...
        goto next;
    }

So len in the range [GENET_RSB_PAD, GENET_RSB_PAD + ETH_FCS_LEN - 1]
(i.e. 66..69) will pass that check.  When crc_fwd_en is set, the
computation then yields pkt_len in [-4, -1].

The driver already tracks such frames via rx_runt_cnt / rx_runt_fcs /
rx_runt_bytes in its MIB counters, so the hardware can deliver them.

> +
> +		/* Save rx_csum before XDP runs - an XDP program
> +		 * could overwrite the RSB via bpf_xdp_adjust_head.
>  		 */
> -		skb = napi_build_skb(hard_start, PAGE_SIZE - XDP_PACKET_HEADROOM);
> -		if (unlikely(!skb)) {
> -			BCMGENET_STATS64_INC(stats, dropped);
> -			page_pool_put_full_page(ring->page_pool, rx_page,
> -						true);
> -			goto next;
> -		}
> +		if (dev->features & NETIF_F_RXCSUM)
> +			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
>  
> -		skb_mark_for_recycle(skb);
> +		xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> +		xdp_prepare_buff(&xdp, page_address(rx_page),
> +				 GENET_RX_HEADROOM, pkt_len, true);

A negative pkt_len gets passed here as data_len, so
xdp_prepare_buff() sets xdp->data_end = xdp->data + pkt_len and
xdp->data_end is now below xdp->data.

>  
> -		/* Reserve the RSB + pad, then set the data length */
> -		skb_reserve(skb, GENET_RSB_PAD);
> -		__skb_put(skb, len - GENET_RSB_PAD);
> +		xdp_act = bcmgenet_run_xdp(ring, xdp_prog, &xdp, rx_page);
> +		if (xdp_act != XDP_PASS)
> +			goto next;
>  
> -		if (priv->crc_fwd_en) {
> -			skb_trim(skb, skb->len - ETH_FCS_LEN);
> +		skb = bcmgenet_xdp_build_skb(ring, &xdp);

With no XDP program attached, bcmgenet_run_xdp() unconditionally
returns XDP_PASS and bcmgenet_xdp_build_skb() is called, which does:

    __skb_put(skb, xdp->data_end - xdp->data);

The (ptrdiff_t) xdp->data_end - xdp->data is negative, but __skb_put()
takes an unsigned int, so it becomes a value near 4GB.  __skb_put()
does not bounds-check and unconditionally advances skb->tail and
skb->len by that amount.  The resulting SKB is then handed to
napi_gro_receive().

Would it make sense to tighten the length check to include ETH_FCS_LEN
when crc_fwd_en is set, or validate pkt_len >= 0 before calling
xdp_prepare_buff()?

The pre-patch code happened to be safe here because skb_trim() has
implicit underflow protection:

    if (skb->len > len)
        ...

So removing the skb_trim path drops that protection.
-- 
pw-bot: cr