linux-kernel - Re: [PATCH AUTOSEL 4.19 18/19] RISC-V: Upgrade smp_mb__after

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: <mhng-5bf9e67f-f6f9-4c9b-9d56-afd0a6e21cd6@palmerdabbelt-glaptop1>
Date:   Sun, 26 Jul 2020 11:36:58 -0700 (PDT)
From:   Palmer Dabbelt <palmerdabbelt@...gle.com>
To:     sashal@...nel.org
CC:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        sashal@...nel.org, linux-riscv@...ts.infradead.org
Subject:     Re: [PATCH AUTOSEL 4.19 18/19] RISC-V: Upgrade smp_mb__after_spinlock() to iorw,iorw

On Mon, 20 Jul 2020 14:38:49 PDT (-0700), sashal@...nel.org wrote:
> From: Palmer Dabbelt <palmerdabbelt@...gle.com>
>
> [ Upstream commit 38b7c2a3ffb1fce8358ddc6006cfe5c038ff9963 ]
>
> While digging through the recent mmiowb preemption issue it came up that
> we aren't actually preventing IO from crossing a scheduling boundary.
> While it's a bit ugly to overload smp_mb__after_spinlock() with this
> behavior, it's what PowerPC is doing so there's some precedent.
>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@...gle.com>
> Signed-off-by: Sasha Levin <sashal@...nel.org>
> ---
>  arch/riscv/include/asm/barrier.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
> index d4628e4b3a5ea..f4c92c91aa047 100644
> --- a/arch/riscv/include/asm/barrier.h
> +++ b/arch/riscv/include/asm/barrier.h
> @@ -69,8 +69,16 @@ do {									\
>   * The AQ/RL pair provides a RCpc critical section, but there's not really any
>   * way we can take advantage of that here because the ordering is only enforced
>   * on that one lock.  Thus, we're just doing a full fence.
> + *
> + * Since we allow writeX to be called from preemptive regions we need at least
> + * an "o" in the predecessor set to ensure device writes are visible before the
> + * task is marked as available for scheduling on a new hart.  While I don't see
> + * any concrete reason we need a full IO fence, it seems safer to just upgrade
> + * this in order to avoid any IO crossing a scheduling boundary.  In both
> + * instances the scheduler pairs this with an mb(), so nothing is necessary on
> + * the new hart.
>   */
> -#define smp_mb__after_spinlock()	RISCV_FENCE(rw,rw)
> +#define smp_mb__after_spinlock()	RISCV_FENCE(iorw,iorw)
>
>  #include <asm-generic/barrier.h>

While I don't think it hurts to have this, IIRC we didn't have the generic
mmiowb spinlock stuff back then so it doesn't really fix the problem.  That
said, I'm pretty sure 4.19 doesn't make it to userspace so backports are really
an academic discussion at this point.  Whatever's less work for everyone is
fine on my end for 4.19.