linux-kernel - Re: [PATCH] irqchip: riscv: Order normal writes and IPI writes

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] [day] [month] [year] [list]

Message-ID: <CAAhSdy0cDUHASb0iOfGFdh_AW3NH-M+2JHTOG+O_FYkhAS4dDw@mail.gmail.com>
Date: Fri, 17 Jan 2025 21:23:06 +0530
From: Anup Patel <anup@...infault.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Charlie Jenkins <charlie@...osinc.com>, Xu Lu <luxu.kernel@...edance.com>, 
	paul.walmsley@...ive.com, palmer@...belt.com, lihangjing@...edance.com, 
	xieyongji@...edance.com, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] irqchip: riscv: Order normal writes and IPI writes

Hi Thomas,

On Fri, Jan 17, 2025 at 4:05 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Charlie!
>
> On Thu, Jan 16 2025 at 13:09, Charlie Jenkins wrote:
> > On Thu, Jan 16, 2025 at 08:07:10PM +0800, Xu Lu wrote:
> >> Replace writel_relaxed() with writel() when issuing IPI to ensure all
> >> previous write operations made by current CPU are visible to other CPUs.
> >
> > Did you experience an ordering issue from this?
>
> That's not the right question.
>
>       CPU 0                     CPU 1
>       store A   // data
>       store B   // IPI
>                                 IPI handler
>                                 load A
>
> The real question is whether the RISC-V memory model guarantees under
> all circumstances that A is globally visible before the IPI handler
> load. If so, then the writel_relaxed() is fine. If not, the fence is
> required.
>
> That's not a question of observation. It's a question of facts defined
> by the architecture. People have wasted months to analyze such fails
> which tend to happen once every blue moon with no other trace than
> "something went wrong" ....

The RISC-V FENCE instruction distinguishes between normal
memory operations and I/O operations in its predecessor and
successor sets where r = normal read, w = normal write,
i = I/O read, and o = I/O write.

The ipi_mux_send_mask() does smp_mb__after_atomic() (equals
to "fence rw,rw") before calling imsic_ipi_send(). This prevents
ordering of normal memory writes in imsic_ipi_send() before
smp_mb__after_atomic() in ipi_mux_send_mask() but it does
not prevent I/O memory writes in imsic_ipi_send() to be ordered
before smp_mb__after_atomic().

This means currently nothing prevents the I/O memory write in
imsic_ipi_send() to be ordered before normal memory writes in
ipi_mux_send_mask() hence there is a very unlikely possibility
of an IPI handler on the target CPU seeing incorrect data.

The conversion of writel_relaxed() to writel() in imsic_ipi_send()
adds a "fence w,o" before the actual I/O memory write which
ensures that I/O memory write is not ordered before normal
memory writes.

Based on the above, the conversion of writel_relaxed() to
writel() in imsic_ipi_send() looks good to me.

Regards,
Anup