Date: Mon, 13 May 2024 10:13:49 +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 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 ...".

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

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

Thanks,
-- Marco

Powered by blists - more mailing lists