Re: [PATCH v3 net 4/7] net: enetc: pad short frames in software

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Sep 17, 2026 at 11:11:27AM +0100, David Laight wrote:
> On Wed, 16 Sep 2026 01:27:31 +0300
> vladimir.oltean@nxp.com wrote:
> 
> > The ENETC does not support BUF_LEN or FRM_LEN in TX buffer descriptors
> > less than 16. This is written in the reference manual of all SoCs
> > supported by the driver: LS1028A, i.MX943, i.MX95 etc.
> >
> >   Frames must not have a FRM_LEN that is less than 16 bytes. Frames of
> >   0-15 bytes are not supported.
> >   (...)
> >   The first descriptor in a chain must not have a BUFF_LEN that is less
> >   than 16 bytes.
> >
> > I don't think proper attention was paid to this during development, we
> > found the text at the end of a bug investigation. Therefore, the driver
> > does not enforce this.
> >
> > But the frame length is out of the driver's control, and the network
> > stack can actually send packets with skb->len smaller than that. The
> > result is unpleasant, as will be explained below, so for simplicity
> > sake, we just pad anything shorter than ETH_ZLEN.
> >
> > Zefir Kurtisi found a case where transmitting L2 WNM keep-alive frames
> > through ENETC would soft-lockup the host through an IRQ storm. He later
> > distilled this into a small enetc-killer.c user space program which
> > sends a packet with MAC DA, MAC SA and EtherType IPv4 (14 octets in
> > length) through an AF_PACKET raw socket.
> >
> > The IRQ storm is actually a curious effect of a chain of events.
> >
> > The hardware behaviour, when an invalid BD is put in its TX ring, is
> > that it would transmit the packet as normal, update counters, raise
> > completion interrupt as normal, but it would just not advance the
> > consumer index of the ring (TBaCIR) to signify that the BD has been
> > consumed and is available for software to free. The ring will also get
> > its TBaSR[BUSY] bit persistently set to 1 afterwards.
> >
> > It deserves an explanation why the behaviour above would lead to an
> > IRQ storm, since ENETC interrupts are message-based (MSI-X), and an
> > unhandled interrupt would typically just be lost rather than retrigger
> > itself as a wired interrupt would.
> >
> > NAPI processing in ENETC has 3 steps:
> >
> > I. the enetc_msix() hardirq handler disables RBaIER, TBaIER and sets
> >    softirq processing to the 'pending' state.
> >
> > II. the enetc_poll() softirq handler for the IRQ vector walks through
> >     the TX rings affine to that vector, checks which ones have a TBCIR
> >     updated since last time - enetc_bd_ready_count() - processes those
> >     completed frames, and clears pending interrupts in these updated TX
> >     rings by writing to TBaIDR. (I've excluded RX processing due to it
> >     being irrelevant).
> >
> > III. After the softirq handler does its round of checking all RX and TX
> >      rings for updates, it re-enables all interrupts in RBaIER and
> >      TBaIER that were previously disabled by the hardirq handler, and
> >      exits.
> >
> > Because the TX ring with the short frame is skipped at step II (TBCIR
> > wasn't updated as part of HW malfunction), its pending IRQ is not
> > cleared in TBaIDR by enetc_clean_tx_ring().
> >
> > But because enetc_msix() disables TBaIER at step I and re-enables it at
> > step III, another MSI will be fired upon re-enabling it. This is what
> > completes the cycle and the driver goes back to step I.
> >
> > So the driver misinterprets the mixed signals it's getting from the
> > hardware, and ends up causing a software-amplified IRQ storm.
> >
> > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
> > Reported-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
> > Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/
> > Tested-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
> > Reviewed-by: Wei Fang <wei.fang@nxp.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > v1->v3: none
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 13 +++++++++++++
> >  drivers/net/ethernet/freescale/enetc/enetc.h |  2 ++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 0216f7d08e19..bbad942041f5 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -1077,6 +1077,19 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
> >       u8 udp, msgtype, twostep;
> >       u16 offset1, offset2;
> >
> > +     /* Hardware does not support transmit buffer descriptors with a total
> > +      * length of less than 16 bytes, or a first buffer size of less than
> > +      * 16 bytes.
> > +      */
> > +     if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> > +                  skb_linearize(skb))) {
> 
> Isn't it only necessary to pull a few bytes into the linear region?

It looks like pskb_may_pull() is an adequate improvement over
skb_linearize(), so I'll use that, thanks.

> > +             dev_kfree_skb_any(skb);
> > +             return NETDEV_TX_OK;
> > +     }
> > +
> > +     if (eth_skb_pad(skb))
> > +             return NETDEV_TX_OK;
> 
> That could be inside the (skb_headlen(skb) < ENETC_MIN_BUFF_SIZE) test.

There was an undocumented reason why it's done unconditionally for any
packet < ETH_ZLEN and not just < ENETC_MIN_BUFF_SIZE. It is to avoid a
separate potential erratum. I'll make sure to mention that in the commit
message.