linux-kernel - Re: [PATCH] block: Annotate a racy read in blk_do_io

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: <CANpmjNP1ajhz5kSuJDv+go=UUvoSaQM2BVphX9LcDA9iGR5A1Q@mail.gmail.com>
Date: Wed, 15 May 2024 15:20:41 +0200
From: Marco Elver <elver@...gle.com>
To: Breno Leitao <leitao@...ian.org>
Cc: paulmck@...nel.org, Bart Van Assche <bvanassche@....org>, Jens Axboe <axboe@...nel.dk>, 
	"open list:BLOCK LAYER" <linux-block@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] block: Annotate a racy read in blk_do_io_stat()

On Wed, 15 May 2024 at 14:48, Breno Leitao <leitao@...ian.org> wrote:
>
> On Wed, May 15, 2024 at 09:58:35AM +0200, Marco Elver wrote:
> > On Wed, 15 May 2024 at 01:47, Paul E. McKenney <paulmck@...nel.org> wrote:
> > > On Mon, May 13, 2024 at 10:13:49AM +0200, Marco Elver wrote:
> > > +Use of volatile and __data_racy
> > > +-------------------------------
> > > +
> > > +Adding the volatile keyword to the declaration of a variable causes both
> > > +the compiler and KCSAN to treat all reads from that variable as if they
> > > +were protected by READ_ONCE() and all writes to that variable as if they
> > > +were protected by WRITE_ONCE().
>
> > "volatile" isn't something we encourage, right? In which case, I think
> > to avoid confusion we should not mention volatile. After all we have
> > this: Documentation/process/volatile-considered-harmful.rst
>
> Since you mentioned this document, the other day I was reading
> volatile-considered-harmful.rst document, and I was surprised that there
> is no reference for READ_ONCE() primitive at all (same for WRITE_ONCE).
>
>         # grep -c READ_ONCE Documentation/process/volatile-considered-harmful.rst
>         0
>
> From my perspective, READ_ONCE() is another way of doing real memory
> read (volatile) when you really need, at the same time keeping the
> compiler free to optimize and reuse the value that was read.

The Linux kernel memory model [1] defines the semantics of READ_ONCE()
/ WRITE_ONCE(). You could say that a READ_ONCE() is something like a
"relaxed (but sometimes consume) atomic load" (in C11 lingo), and a
WRITE_ONCE() is a "relaxed atomic store". But again, not exactly
because the kernel wants to do things that are outside the C standard
and no compiler fully supports. This has fun consequences, such as
[2].

[1] https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0124r6.html
[2] https://lpc.events/event/16/contributions/1174/attachments/1108/2121/Status%20Report%20-%20Broken%20Dependency%20Orderings%20in%20the%20Linux%20Kernel.pdf

To get what the kernel wants, implementing *ONCE in terms of
"volatile" works in all important cases. But we know that it can go
wrong - [2] discusses this.

The big hope is that one day, the kernel can just switch the *ONCE()
implementation to use something better but as a programmer we
shouldn't care that there's volatile underneath.

> Should volatile-considered-harmful.rst be also expanded to cover
> READ_ONCE()?

No, on most architectures READ_ONCE/WRITE_ONCE are implemented with
volatile, but that's merely an implementation detail. Once upon a
time, READ_ONCE on alpha actually implied a real barrier. On arm64
with LTO, READ_ONCE translates into an acquire-load [3] to mitigate
the risk of "volatile" actually resulting in "miscompilation"
(according to our desired semantics of READ_ONCE).

[3] https://elixir.bootlin.com/linux/latest/source/arch/arm64/include/asm/rwonce.h#L26

Thanks,
-- Marco