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

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The driver incorrectly applies the original hardware checksum (`CHECKSUM_COMPLETE`) to the SKB after an XDP program has processed the packet.
--

commit 5fcb56745df8a1527b5006d1089794e51dbc65ee
Author: Nicolai Buchwitz <nb@tipi-net.de>

net: bcmgenet: add basic XDP support (PASS/DROP)

This commit adds basic XDP program attachment via ndo_bpf and executes
XDP programs in the RX path. It handles XDP_PASS by building an SKB from
the xdp_buff, and XDP_DROP by returning the page to the page_pool.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2404,30 +2462,34 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,

[ ... ]

> +		/* Save rx_csum before XDP runs - an XDP program
> +		 * could overwrite the RSB via bpf_xdp_adjust_head.
>  		 */
> +		if (dev->features & NETIF_F_RXCSUM)
> +			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
>  
> +		xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> +		xdp_prepare_buff(&xdp, page_address(rx_page),
> +				 GENET_RX_HEADROOM, pkt_len, true);
>  
> +		xdp_act = bcmgenet_run_xdp(ring, xdp_prog, &xdp, rx_page);
> +		if (xdp_act != XDP_PASS)
> +			goto next;
>  
> +		skb = bcmgenet_xdp_build_skb(ring, &xdp);
> +		if (unlikely(!skb)) {
> +			BCMGENET_STATS64_INC(stats, dropped);
> +			page_pool_put_full_page(ring->page_pool,
> +						rx_page, true);
> +			goto next;
>  		}
>  
>  		/* Set up checksum offload */
>  		if (dev->features & NETIF_F_RXCSUM) {
>  			if (rx_csum) {
>  				skb->csum = (__force __wsum)ntohs(rx_csum);
>  				skb->ip_summed = CHECKSUM_COMPLETE;

Does this unconditionally apply the original hardware checksum to the SKB
after the XDP program has executed?

Since XDP programs can modify the packet payload before returning XDP_PASS,
the original hardware sum might no longer be mathematically valid for the
modified packet.

If a modified packet retains CHECKSUM_COMPLETE with the old checksum, could
the network stack rely on it, fail validation, and erroneously drop the
packet?

Would it make sense to invalidate the hardware checksum (for example, by
setting skb->ip_summed to CHECKSUM_NONE) if an XDP program is present and
processes the packet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416054743.1289191-1-nb@tipi-net.de?part=3