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 for Android: free password hash cracker in your pocket

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

Message-ID: <87frle9br2.ffs@tglx>
Date: Mon, 20 Jan 2025 08:37:37 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Anup Patel <anup@...infault.org>
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

On Fri, Jan 17 2025 at 21:23, Anup Patel wrote:
> On Fri, Jan 17, 2025 at 4:05 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>> 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.

Very unlikely is a valid assumption for a single system, but at scale it
becomes very likely :)

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

Can we please have something like the above in the change log so this is
documented for posterity?

Thanks

        tglx