Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[Vladimir Oltean]

On Thu, Oct 22, 2026 at 11:22:18AM +0100, David Laight wrote:
> On Thu, 22 Oct 2026 12:41:49 +0300
> Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
> 
> > On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote:
> > > On Wed, 21 Oct 2026 17:53:12 +0300
> > > vladimir.oltean@xxxxxxx wrote:
> > >   
> > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers.
> > > > This works, however sending short frames from user space and looking at
> > > > the padding bytes inserted by the MAC at the link partner, we notice on
> > > > LS1028A that the first octet of padding is sometimes not 0.
> > > > 
> > > > This is fine per 802.3, which does not require that the padding is zero.
> > > > 
> > > > However, there is an unproven suspicion that the padding bytes may
> > > > come from internal memories, and unfortunately I couldn't get sufficient
> > > > priority for this octet's origin to be investigated by hardware
> > > > engineers.
> > > > 
> > > > To be defensive and not leak any sensitive information from the SoC,
> > > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't
> > > > have to do anything.
> > > > 
> > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO
> > > > things are trivial; we need to call eth_skb_pad(). For GSO, things are
> > > > not simple, because user space may set gso_size to a very small value,
> > > > which could make the GSO super-skb smaller than ETH_ZLEN;  
> > > 
> > > That isn't sensible.
> > > The minimum frame is 64 bytes (including FCS), subtract that, the MAC
> > > header (14 bytes), minimum IP header (20), minimum TCP (20) and that
> > > leaves 6 bytes for user data.
> > > I doubt it is sensible to do GSO for anything under the minimum mtu
> > > (128 bytes rings a bell somewhere).  
> > 
> > My fault for not being clear and inviting this kind of objection.
> > 
> > Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and
> > there, Sashiko came on v3 with the observation that for UDP GSO, user
> > space can set gso_size to a small value using setsockopt(UDP_SEGMENT):
> > https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@xxxxxxxxxx/
> 
> I'd fix that in the setsockopt code.
> But that might be a bug anyway.

Why a bug?

> UDP segmentation has to use IP fragmentation and that is usually done because
> the interface/path mtu is too short.

In my testing, UDP GSO does not generate IP fragments, but rather, each
UDP segment is its own datagram.

Are you thinking of UDP Fragmentation Offload instead? As per
https://docs.kernel.org/networking/segmentation-offloads.html#udp-fragmentation-offload
this is deprecated and AFAIU not what this is about.

The docs do also explicitly say that UDP Segmentation Offload does not
generate IP fragments.

> But there is a minimum mtu for IPv4 of (something like) 128 bytes, and
> it may be invalid to fragment into smaller pieces.

I have no idea what you are referring to, here, with the minimum MTU of
128 bytes.

> > Sensible or not, it is possible, and I did generate a test program using
> > an LLM to confirm this.
> > 
> > Regardless of that, the last segment of a GSO super-skb would need
> > padding anyway, because even with a 'normal' TCP MSS, the skb->len
> > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding
> > just the GSO super-skb, as I did in v3, would also not fix this.
> 
> True, but that is a different check to the one you did.

So what is your suggestion for the driver to do?

> > > But does this code allow for the VLAN tag that you forgot about in the
> > > other patch?  
> > 
> > Not sure I understand your concern here? Is it that with VLAN TX
> > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC
> > pushes an extra VLAN_HLEN)? I'm not sure that's a problem..
> 
> That is a problem, VLAN packets should only be padded to 64 bytes
> including the VLAN header and CRC.
> However such overlong packets are likely to be tolerated because
> an ethernet switch can add a VLAN header before forwarding a packet
> and it can't know whether a short packet is padded.

Well, if it's a theoretical problem but it's not a practical problem,
I'd prefer leaving it as is.

Since eth_skb_pad() itself is unaware of VLAN TX offloads, I'd be
opening myself up to an endless rabbit hole ("why didn't you fix this
in eth_skb_pad()? It's a generic problem." followed by
"changing eth_skb_pad() broke 20 drivers!")...

Unless it's very clear why I'd be doing this, I'd very much prefer not to.