Message-ID: <CANpmjNMqRUNUs1mZEhrOSyK0Hk+PdGOi+VAs22qYD+1zTkwfhg@mail.gmail.com>
Date: Wed, 15 May 2024 09:58:35 +0200
From: Marco Elver <elver@...gle.com>
To: paulmck@...nel.org
Cc: Bart Van Assche <bvanassche@....org>, Breno Leitao <leitao@...ian.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 01:47, Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Mon, May 13, 2024 at 10:13:49AM +0200, Marco Elver wrote:
> > On Sat, 11 May 2024 at 02:41, Paul E. McKenney <paulmck@...nel.org> wrote:
> > [...]
> > > ------------------------------------------------------------------------
> > >
> > > commit 930cb5f711443d8044e88080ee21b0a5edda33bd
> > > Author: Paul E. McKenney <paulmck@...nel.org>
> > > Date:   Fri May 10 15:36:57 2024 -0700
> > >
> > >     kcsan: Add example to data_race() kerneldoc header
> > >
> > >     Although the data_race() kerneldoc header accurately states what it does,
> > >     some of the implications and usage patterns are non-obvious.  Therefore,
> > >     add a brief locking example and also state how to have KCSAN ignore
> > >     accesses while also preventing the compiler from folding, spindling,
> > >     or otherwise mutilating the access.
> > >
> > >     [ paulmck: Apply Bart Van Assche feedback. ]
> > >
> > >     Reported-by: Bart Van Assche <bvanassche@....org>
> > >     Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> > >     Cc: Marco Elver <elver@...gle.com>
> > >     Cc: Breno Leitao <leitao@...ian.org>
> > >     Cc: Jens Axboe <axboe@...nel.dk>
> > >
> > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > > index c00cc6c0878a1..9249768ec7a26 100644
> > > --- a/include/linux/compiler.h
> > > +++ b/include/linux/compiler.h
> > > @@ -194,9 +194,17 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> > >   * This data_race() macro is useful for situations in which data races
> > >   * should be forgiven.  One example is diagnostic code that accesses
> > >   * shared variables but is not a part of the core synchronization design.
> > > + * For example, if accesses to a given variable are protected by a lock,
> > > + * except for diagnostic code, then the accesses under the lock should
> > > + * be plain C-language accesses and those in the diagnostic code should
> > > + * use data_race().  This way, KCSAN will complain if buggy lockless
> > > + * accesses to that variable are introduced, even if the buggy accesses
> > > + * are protected by READ_ONCE() or WRITE_ONCE().
> > >   *
> > > - * This macro *does not* affect normal code generation, but is a hint
> > > - * to tooling that data races here are to be ignored.
> > > + * This macro *does not* affect normal code generation, but is a hint to
> > > + * tooling that data races here are to be ignored.  If code generation must
> > > + * be protected *and* KCSAN should ignore the access, use both data_race()
> >
> > "code generation must be protected" seems ambiguous, because
> > protecting code generation could mean a lot of different things to
> > different people.
> >
> > The more precise thing would be to write that "If the access must be
> > atomic *and* KCSAN should ignore the access, use both ...".
>
> Good point, and I took your wording, thank you.
>
> > I've also had trouble in the past referring to "miscompilation" or
> > similar, because that also entirely depends on the promised vs.
> > expected semantics: if the code in question assumes for the access to
> > be atomic, the compiler compiling the code in a way that the access is
> > no longer atomic would be a "miscompilation". Although is it still a
> > "miscompilation" if the compiler generated code according to specified
> > language semantics (say according to our own LKMM) - and that's where
> > opinions can diverge because it depends on which side we stand
> > (compiler vs. our code).
>
> Agreed, use of words like "miscompilation" can annoy people.  What
> word would you suggest using instead?

Not sure. As suggested above, I try to just stick to "atomic" vs
"non-atomic" because that's ultimately the functional end result of
such a miscompilation. Although I've also had people be confused as in
"what atomic?! as in atomic RMW?!", but I don't know how to remove
that kind of confusion.

If, however, our intended model is the LKMM and e.g. a compiler breaks
a dependency-chain, then we could talk about miscompilation, because
the compiler violates our desired language semantics. Of course the
compiler writers then will say that we try to do things that are
outside any implemented language semantics the compiler is aware of,
so it's not a miscompilation again. So it all depends on which side
we're arguing for. Fun times.

> > > + * and READ_ONCE(), for example, data_race(READ_ONCE(x)).
> >
> > Having more documentation sounds good to me, thanks for adding!
> >
> > This extra bit of documentation also exists in a longer form in
> > access-marking.txt, correct? I wonder how it would be possible to
> > refer to it, in case the reader wants to learn even more.
>
> Good point, especially given that I had forgotten about it.
>
> I don't have any immediate ideas for calling attention to this file,
> but would the following update be helpful?

Mentioning __data_racy along with data_race() could be helpful, thank
you. See comments below.

Thanks,
-- Marco

>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/tools/memory-model/Documentation/access-marking.txt b/tools/memory-model/Documentation/access-marking.txt
> index 65778222183e3..690dd59b7ac59 100644
> --- a/tools/memory-model/Documentation/access-marking.txt
> +++ b/tools/memory-model/Documentation/access-marking.txt
> @@ -24,6 +24,12 @@ The Linux kernel provides the following access-marking options:
>  4.     WRITE_ONCE(), for example, "WRITE_ONCE(a, b);"
>         The various forms of atomic_set() also fit in here.
>
> +5.     ASSERT_EXCLUSIVE_ACCESS() and ASSERT_EXCLUSIVE_WRITER().

Perhaps worth mentioning, but they aren't strictly access-marking
options. In the interest of simplicity could leave it out.

> +6.     The volatile keyword, for example, "int volatile a;"

See below - I'm not sure we should mention volatile. It may set the
wrong example.

> +7.     __data_racy, for example "int __data_racy a;"
> +
>
>  These may be used in combination, as shown in this admittedly improbable
>  example:
> @@ -205,6 +211,27 @@ because doing otherwise prevents KCSAN from detecting violations of your
>  code's synchronization rules.
>
>
> +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

> +Adding the __data_racy type qualifier to the declaration of a variable
> +causes KCSAN to treat all accesses to that variable as if they were
> +enclosed by data_race().  However, __data_racy does not affect the
> +compiler, though one could imagine hardened kernel builds treating the
> +__data_racy type qualifier as if it was the volatile keyword.
> +
> +Note well that __data_racy is subject to the same pointer-declaration
> +rules as is the volatile keyword.  For example:

To avoid referring to volatile could make it more general and say "..
rules as other type qualifiers like const and volatile".


> +       int __data_racy *p; // Pointer to data-racy data.
> +       int *__data_racy p; // Data-racy pointer to non-data-racy data.
> +
> +
>  ACCESS-DOCUMENTATION OPTIONS
>  ============================
>
> @@ -342,7 +369,7 @@ as follows:
>
>  Because foo is read locklessly, all accesses are marked.  The purpose
>  of the ASSERT_EXCLUSIVE_WRITER() is to allow KCSAN to check for a buggy
> -concurrent lockless write.
> +concurrent write, whether marked or not.
>
>
>  Lock-Protected Writes With Heuristic Lockless Reads

Powered by blists - more mailing lists