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

[David Laight <david.laight.linux@gmail.com>]

On Thu, 22 Oct 2026 12:41:49 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> 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@nxp.com 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@kernel.org/

I'd fix that in the setsockopt code.
But that might be a bug anyway.
UDP segmentation has to use IP fragmentation and that is usually done because
the interface/path mtu is too short.
But there is a minimum mtu for IPv4 of (something like) 128 bytes, and
it may be invalid to fragment into smaller pieces.

> 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.

> > 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.

> Anyway, with VLAN TX offload, the VLAN tag is not part of the frm_len.
> It's part of the extension BD that we forgot to account for, when
> determining whether the TX BD ring has enough space for the frame under
> transmission.

That is what I was thinking of.
By brain thought it was a normal BD - so would be in the 15 limit.

David

> > I also noticed that the code seems to dma map the fragments for each
> > gso segment separately, just mapping each skb fragment would be better.
> > (But that is for another patch series.)   
> 
> Noted, I can look into this separately.
> 
> Thanks for the review!