Re: [PATCH net-next v8 7/7] net: bcmgenet: reject MTU changes incompatible with XDP

Re: [PATCH net-next v8 7/7] net: bcmgenet: reject MTU changes incompatible with XDP

[Nicolai Buchwitz]

On 29.4.2026 23:17, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential 
> issue(s) to consider:
> - [Medium] The XDP MTU validation logic contains a mathematical error 
> (ignoring L2 headers) and is structurally unreachable (dead code) 
> because the device's maximum MTU is fixed at 1500.
> --
> 
> commit b8018c0a123b4927db5ca8c39c8395c751532069
> Author: Nicolai Buchwitz <nb@tipi-net.de>
> 
> net: bcmgenet: reject MTU changes incompatible with XDP
> 
> This commit introduces a minimal ndo_change_mtu function to the 
> bcmgenet
> driver. It checks if an XDP program is attached and rejects MTU values 
> that
> would exceed the capacity of a single-page XDP buffer.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 02ad2f410d6c..4d1ec68ec0c5 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -4085,6 +4085,20 @@ static int bcmgenet_xdp_xmit(struct net_device 
>> *dev, int num_frames,
> [ ... ]
>> +static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu)
>> +{
>> +	struct bcmgenet_priv *priv = netdev_priv(dev);
>> +
>> +	if (priv->xdp_prog && new_mtu > PAGE_SIZE - GENET_RX_HEADROOM -
>> +	    SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) {
> 
> Is this condition actually reachable, and does it correctly account for 
> L2
> headers?
> 
> The bcmgenet driver does not appear to override dev->max_mtu, which
> defaults to ETH_DATA_LEN (1500). Because dev_validate_mtu() in the core
> networking stack checks that new_mtu <= dev->max_mtu before calling
> ndo_change_mtu, new_mtu will never exceed 1500.
> 
> Given that PAGE_SIZE (typically 4096) minus GENET_RX_HEADROOM and the
> skb_shared_info alignment is roughly 3454 bytes, the check new_mtu > 
> 3454
> will always evaluate to false.
> 
> Also, new_mtu represents the L3 payload size. Shouldn't the check add 
> the
> L2 header overhead when comparing against the L2 buffer capacity?
> 
> Furthermore, is it possible for an MTU change to corrupt the XDP buffer
> layout as the commit message claims?

Correct, the check is currently unreachable since dev->max_mtu defaults
to 1500. It is a safety guard for the planned follow-up series adding
full ndo_change_mtu support. L2 overhead handling will be part of that
follow-up.

The commit message wording is slightly misleading; today no MTU change
can corrupt the layout, but once jumbo MTU support is added, this guard
prevents misconfiguration while an XDP program is attached.

The original discussion with Mohsin and Florian on the rationale for
adding this in the XDP series rather than deferring to the MTU
follow-up is here:
https://lore.kernel.org/netdev/1d551008-3c42-4b7b-9507-03072740a54a@gmail.com/
https://lore.kernel.org/netdev/99062602-05a3-418f-9182-f60a83fdbe3b@broadcom.com/

> 
> The hardware DMA appears to be statically configured to write a maximum 
> of
> RX_BUF_LENGTH (2048) bytes per buffer, and bcmgenet_desc_rx() 
> explicitly
> drops any packet larger than 2048 bytes. It seems structurally 
> impossible
> to overflow the 4096-byte page and corrupt skb_shared_info.
> 
>> +		netdev_warn(dev, "MTU too large for single-page XDP buffer\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	WRITE_ONCE(dev->mtu, new_mtu);
>> +	return 0;
>> +}


Thanks
Nicolai