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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware checksum offload (CHECKSUM_COMPLETE) is unconditionally applied after XDP execution, causing valid modified packets to be dropped.
- [Medium] The driver advertises NETDEV_XDP_ACT_BASIC without supporting XDP_TX, violating the XDP feature API contract.
--

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

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

This commit adds basic XDP support to the bcmgenet driver, allowing XDP
programs to execute in the RX path with support for XDP_PASS and XDP_DROP.

> 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) {
> -			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
>  			if (rx_csum) {
>  				skb->csum = (__force __wsum)ntohs(rx_csum);
>  				skb->ip_summed = CHECKSUM_COMPLETE;
>  			}

If the XDP program modifies the packet payload (e.g., rewriting IPs or
ports), the hardware checksum saved before XDP execution will no longer match
the new payload.

If we unconditionally apply this old checksum and set CHECKSUM_COMPLETE on
the SKB here, will the network stack drop the valid modified packet due to a
hardware checksum failure?

Should the checksum be invalidated when an XDP program is present?

[ ... ]
> @@ -4054,6 +4148,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  			 NETIF_F_RXCSUM;
>  	dev->hw_features |= dev->features;
>  	dev->vlan_features |= dev->features;
> +	dev->xdp_features = NETDEV_XDP_ACT_BASIC;
>  
>  	netdev_sw_irq_coalesce_default_on(dev);

The commit message notes that XDP_TX is not yet supported and returns
XDP_ABORTED.

The UAPI definition for NETDEV_XDP_ACT_BASIC implies support for XDP_PASS,
XDP_DROP, XDP_ABORTED, and XDP_TX. Does advertising this flag without XDP_TX
support break feature detection for userspace applications?

I see from the patch series that XDP_TX support is added in a later commit.
Would it be safer to defer setting NETDEV_XDP_ACT_BASIC until that patch to
avoid breaking userspace assumptions during a bisect?

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