Re: [PATCH kcsan 18/19] bitops, kcsan: Partially revert instrumentation for non-atomic bitops

[Marco Elver]

On Wed, Sep 02, 2020 at 11:30AM +0800, Boqun Feng wrote:
> Hi Paul and Marco,
> 
> The whole update patchset looks good to me, just one question out of
> curiosity fo this one, please see below:
> 
> On Mon, Aug 31, 2020 at 11:18:04AM -0700, paulmck@xxxxxxxxxx wrote:
> > From: Marco Elver <elver@xxxxxxxxxx>
> > 
> > Previous to the change to distinguish read-write accesses, when
> > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y is set, KCSAN would consider
> > the non-atomic bitops as atomic. We want to partially revert to this
> > behaviour, but with one important distinction: report racing
> > modifications, since lost bits due to non-atomicity are certainly
> > possible.
> > 
> > Given the operations here only modify a single bit, assuming
> > non-atomicity of the writer is sufficient may be reasonable for certain
> > usage (and follows the permissible nature of the "assume plain writes
> > atomic" rule). In other words:
> > 
> > 	1. We want non-atomic read-modify-write races to be reported;
> > 	   this is accomplished by kcsan_check_read(), where any
> > 	   concurrent write (atomic or not) will generate a report.
> > 
> > 	2. We do not want to report races with marked readers, but -do-
> > 	   want to report races with unmarked readers; this is
> > 	   accomplished by the instrument_write() ("assume atomic
> > 	   write" with Kconfig option set).
> > 
> 
> Is there any code in kernel using the above assumption (i.e.
> non-atomicity of the writer is sufficient)? IOW, have you observed
> anything bad (e.g. an anoying false positive) after applying the
> read_write changes but without this patch?

We were looking for an answer to:

	https://lkml.kernel.org/r/20200810124516.GM17456@xxxxxxxxxxxxxxxxxxxx

Initially we thought using atomic bitops might be required, but after a
longer offline discussion realized that simply marking the reader in
this case, but retaining the non-atomic bitop is probably all that's
needed.

The version of KCSAN that found the above was still using KCSAN from
Linux 5.8, but we realized with the changed read-write instrumentation
to bitops in this series, we'd regress and still report the race even if
the reader was marked. To avoid this with the default KCSAN config, we
determined that we need the patch here.

The bitops are indeed a bit more special, because for both the atomic
and non-atomic bitops we *can* reason about the generated code (since we
control it, although not sure about the asm-generic ones), and that
makes reasoning about accesses racing with non-atomic bitops more
feasible. At least that's our rationale for deciding that reverting
non-atomic bitops treatment to it's more relaxed version is ok.

Thanks,
-- Marco