linux-kernel - Re: [PATCH] Add io_sync stubs to generic IO memcpy/memset

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: <d8ab7cae-4f85-4da5-b45e-e4af4f5b5fb2@app.fastmail.com>
Date: Tue, 28 Jan 2025 10:14:23 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Julian Vetter" <julian@...er-limits.org>,
 "Andrew Morton" <akpm@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add io_sync stubs to generic IO memcpy/memset

On Tue, Jan 28, 2025, at 09:32, Julian Vetter wrote:
> On 1/27/25 16:48, Arnd Bergmann wrote:
>> I suspect that the powerpc variant got this wrong for historic
>> reasons, but that's hard to tell now. The ppc32 variant didn't
>> have barriers at all originally, it was just memcpy/memset
>> before it got combined with ppc64 into arch/powerpc.
>> 
>
> hmmm... ok. I'm not sure what do make of this. But maybe I can just send 
> a patch to the PowerPC mailinglist, without those "sync" calls and see 
> what they have to say.

I just looked again and I see that the powerpc I/O memcpy/memset
functions do use the same barriers as readl/writel -- sync
before each one, twi/isync after read and sync after write,
the only difference is the eieio in the middle, so you could
start with a patch that removes the eieio.  The question here
is whether we want to allow or prevent combining and reordering
accesses within the string operations, and my feeling is that
allowing them makes more sense here, but there may be a powerpc
specific reason we don't want that.

I'm still unsure about having barriers before/after the string
operations, I can very much see that debate go either way, but
I also feel like this should be done consistently across all
architectures.

A possible answer may be that we declare these helpers to
include the same barriers as readl_relaxed()/writel_relaxed(),
i.e. ordering between I/O operations is enforced, but not
the barriers for serializing against DMA and interrupts
that is provided by the normal readl()/writel().

Most architectures assume that the relaxed variants don't
need any barriers, so that is what asm-generic/io.h
implements, but I think alpha and mips need barriers here
and powerpc simply defines the relaxed accessors to
be identical to the non-relaxed ones for simplicity.

>>> The existing ones, especially __io_br unfortunately don't resolve to the
>>> right define on these architectures. The __io_ar and __io_br resolve to
>>> the right mb() on SuperH and PowerPC as well, but this would again have
>>> implications on other architectures.
>> 
>> The barriers in the sh functions seem arbitrary, and I would
>> expect them to be wrong.
>> 
> Ok, same for SuperH, I will send a patch to the mailinglist without the 
> 'mb()' before and after and see what they say. If they don't like it, I 
> will come back to this.

I doubt there is anyone left who understands the history behind
the exact implementation on sh, the only real options I see for it
are to not touch it at all, or to remove the functions in favor of
the generic ones.

sh also has custom readsl/writesl functions, which are related,
but I see that it is completely missing the corresponding
readsw/writesw and readsb/writesb interfaces and it prevents
the use of the generic implementations.

     Arnd