linux-kernel - Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+V-a8vudn0=kSnaAT4qDCcRtVShmS+n2A4GOQH2iogYizUBzw@mail.gmail.com>
Date: Wed, 15 Oct 2025 18:01:13 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Niklas Söderlund <niklas.soderlund@...natech.se>
Cc: Paul Barker <paul@...rker.dev>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>, netdev@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>, stable@...r.kernel.org
Subject: Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to
 prevent early DMA start

Hi Niklas,

Thank you for the review.

On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
<niklas.soderlund@...natech.se> wrote:
>
> Hi Prabhakar,
>
> Thanks for your work.
>
> On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Ensure TX descriptor type fields are written in a safe order so the DMA
> > engine does not begin processing a chain before all descriptors are
> > fully initialised.
> >
> > For multi-descriptor transmissions the driver writes DT_FEND into the
> > last descriptor and DT_FSTART into the first. The DMA engine starts
> > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > and process an incomplete chain, leading to corrupted transmissions or
> > DMA errors.
> >
> > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > descriptor case), and then adding a wmb() after the type updates to
> > ensure CPU-side ordering before ringing the hardware doorbell.
> >
> > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > observed as TX stalls and timeouts:
> >
> >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
> >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> >
> > This change enforces the required ordering and prevents the DMA engine
> > from observing DT_FSTART before the rest of the descriptor chain is
> > valid.
> >
> > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > Cc: stable@...r.kernel.org
> > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index a200e205825a..2a995fa9bfff 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >
> >               skb_tx_timestamp(skb);
> >       }
> > -     /* Descriptor type must be set after all the above writes */
> > -     dma_wmb();
> > +
> > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> >       if (num_tx_desc > 1) {
> >               desc->die_dt = DT_FEND;
> >               desc--;
> > -             desc->die_dt = DT_FSTART;
> > -     } else {
> > -             desc->die_dt = DT_FSINGLE;
> >       }
> > +
> > +     /* Descriptor type must be set after all the above writes */
> > +     dma_wmb();
> > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
>
> IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> code the full steps in each branch of the if above. It would make it
> easier to read and understand.
>
I did this just to avoid compiler optimizations. With the previous
similar code on 5.10 CIP RT it was observed that the compiler
optimized code in such a way that the DT_FSTART was written first
before DT_FEND while the DMA was active because of which we ran into
DMA issues causing QEF errors.

> > +
> > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > +     wmb();
>
> All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> before ringing the doorbell? Maybe I'm missing something obvious?
>
This wmb() was mainly added to ensure all the descriptor data is in
RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
mentions that we need to read back the last written descriptor before
triggering the DMA. Please let me know if you think this can be
handled differently.

Cheers,
Prabhakar

> >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> >
> >       priv->cur_tx[q] += num_tx_desc;
> > --
> > 2.43.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund