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

On 29.4.2026 23:17, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

This is the standard pattern across XDP drivers (e.g. ice). With
CHECKSUM_COMPLETE, the stack performs software validation by combining
the hardware checksum with its own pseudo-header sum. If an XDP program
modifies the packet, validation fails and the stack drops the packet,
no silent corruption. XDP programs that modify packets and return PASS
are extremely rare; the common pattern is modify+TX/REDIRECT.

> 
> [ ... ]
>> @@ -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?

Per <linux/netdev.h>, NETDEV_XDP_ACT_BASIC = "XDP features set supported
by all drivers (XDP_ABORTED, XDP_DROP, XDP_PASS, XDP_TX)". The XDP_TX
listed there is the BPF action constant, which is always handled (we
return XDP_ABORTED for unsupported actions, which is the documented
behavior). The driver-level XDP_TX *capability* is a separate flag
(NETDEV_XDP_ACT_XDP_TX), not implied by BASIC.

Thanks
Nicolai