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

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: <20200904012443.GB7503@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
Date:   Fri, 4 Sep 2020 09:24:43 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Marco Elver <elver@...gle.com>
Cc:     paulmck@...nel.org, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, kernel-team@...com, mingo@...nel.org,
        andreyknvl@...gle.com, glider@...gle.com, dvyukov@...gle.com,
        cai@....pw, Will Deacon <will@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, Daniel Axtens <dja@...ens.net>,
        Michael Ellerman <mpe@...erman.id.au>,
        linux-arch@...r.kernel.org
Subject: Re: [PATCH kcsan 18/19] bitops, kcsan: Partially revert
 instrumentation for non-atomic bitops

On Wed, Sep 02, 2020 at 08:13:15AM +0200, Marco Elver wrote:
> 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@...nel.org wrote:
> > > From: Marco Elver <elver@...gle.com>
> > > 
> > > 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@casper.infradead.org
> 
> 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.
> 

Thanks for the background! Now I see the point of having this patch ;-)

FWIW, feel free to add for the whole series:

Reviewed-by: Boqun Feng <boqun.feng@...il.com>

Regards,
Boqun

> 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