| 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
| ||
|
Date: Mon, 17 May 2021 11:31:42 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Manfred Spraul <manfred@...orfullife.com>
Cc: kasan-dev <kasan-dev@...glegroups.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Davidlohr Bueso <dbueso@...e.de>, 1vier1@....de
Subject: Re: ipc/sem, ipc/msg, ipc/mqueue.c kcsan questions
On Fri, May 14, 2021 at 11:44:55AM -0700, Paul E. McKenney wrote:
> On Fri, May 14, 2021 at 07:41:02AM +0200, Manfred Spraul wrote:
> > On 5/13/21 9:02 PM, Paul E. McKenney wrote:
> > > On Thu, May 13, 2021 at 08:10:51AM +0200, Manfred Spraul wrote:
> > > > On 5/12/21 10:17 PM, Paul E. McKenney wrote:
[ . . . ]
> > > > > int foo;
> > > > > DEFINE_RWLOCK(foo_rwlock);
> > > > >
> > > > > void update_foo(int newval)
> > > > > {
> > > > > write_lock(&foo_rwlock);
> > > > > foo = newval;
> > > > > do_something(newval);
> > > > > write_unlock(&foo_rwlock);
> > > > > }
> > > > >
> > > > > int read_foo(void)
> > > > > {
> > > > > int ret;
> > > > >
> > > > > read_lock(&foo_rwlock);
> > > > > do_something_else();
> > > > > ret = foo;
> > > > > read_unlock(&foo_rwlock);
> > > > > return ret;
> > > > > }
> > > > >
> > > > > int read_foo_diagnostic(void)
> > > > > {
> > > > > return data_race(foo);
> > > > > }
> > > > The text didn't help, the example has helped:
> > > >
> > > > It was not clear to me if I have to use data_race() both on the read and the
> > > > write side, or only on one side.
> > > >
> > > > Based on this example: plain C may be paired with data_race(), there is no
> > > > need to mark both sides.
> > > Actually, you just demonstrated that this example is quite misleading.
> > > That data_race() works only because the read is for diagnostic
> > > purposes. I am queuing a commit with your Reported-by that makes
> > > read_foo_diagnostic() just do a pr_info(), like this:
> > >
> > > void read_foo_diagnostic(void)
> > > {
> > > pr_info("Current value of foo: %d\n", data_race(foo));
> > > }
> > >
> > > So thank you for that!
> >
> > I would not like this change at all.
> > Assume you chase a rare bug, and notice an odd pr_info() output.
> > It will take you really long until you figure out that a data_race() mislead
> > you.
> > Thus for a pr_info(), I would consider READ_ONCE() as the correct thing.
>
> It depends, but I agree with a general preference for READ_ONCE() over
> data_race().
>
> However, for some types of concurrency designs, using a READ_ONCE()
> can make it more difficult to enlist KCSAN's help. For example, if this
> variable is read or written only while holding a particular lock, so that
> read_foo_diagnostic() is the only lockless read, then using READ_ONCE()
> adds a concurrent read. In RCU, the updates would now need WRITE_ONCE(),
> which would cause KCSAN to fail to detect a buggy lockless WRITE_ONCE().
> If data_race() is used, then adding a buggy lockless WRITE_ONCE() will
> cause KCSAN to complain.
>
> Of course, you would be quite correct to say that this must be balanced
> against the possibility of a messed-up pr_info() due to compiler mischief.
> Tradeoffs, tradeoffs! ;-)
>
> I should document this tradeoff, shouldn't I?
Except that Marco Elver reminds me that there are two other possibilities:
1. data_race(READ_ONCE(foo)), which both suppresses compiler
optimizations and causes KCSAN to ignore the access.
2. "void __no_kcsan read_foo_diagnostic(void)" to cause KCSAN to
ignore the entire function, and READ_ONCE() on the access.
So things might be the way you want anyway. Does the patch below work
for you?
Thanx, Paul
------------------------------------------------------------------------
diff --git a/tools/memory-model/Documentation/access-marking.txt b/tools/memory-model/Documentation/access-marking.txt
index fe4ad6d12d24..e3012f666e62 100644
--- a/tools/memory-model/Documentation/access-marking.txt
+++ b/tools/memory-model/Documentation/access-marking.txt
@@ -279,19 +279,34 @@ tells KCSAN that data races are expected, and should be silently
ignored. This data_race() also tells the human reading the code that
read_foo_diagnostic() might sometimes return a bogus value.
-However, please note that your kernel must be built with
-CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n in order for KCSAN to
-detect a buggy lockless write. If you need KCSAN to detect such a
-write even if that write did not change the value of foo, you also
-need CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY=n. If you need KCSAN to
-detect such a write happening in an interrupt handler running on the
-same CPU doing the legitimate lock-protected write, you also need
-CONFIG_KCSAN_INTERRUPT_WATCHER=y. With some or all of these Kconfig
-options set properly, KCSAN can be quite helpful, although it is not
-necessarily a full replacement for hardware watchpoints. On the other
-hand, neither are hardware watchpoints a full replacement for KCSAN
-because it is not always easy to tell hardware watchpoint to conditionally
-trap on accesses.
+If it is necessary to suppress compiler optimization and also detect
+buggy lockless writes, read_foo_diagnostic() can be updated as follows:
+
+ void read_foo_diagnostic(void)
+ {
+ pr_info("Current value of foo: %d\n", data_race(READ_ONCE(foo)));
+ }
+
+Alternatively, given that KCSAN is to ignore all accesses in this function,
+this function can be marked __no_kcsan and the data_race() can be dropped:
+
+ void __no_kcsan read_foo_diagnostic(void)
+ {
+ pr_info("Current value of foo: %d\n", READ_ONCE(foo));
+ }
+
+However, in order for KCSAN to detect buggy lockless writes, your kernel
+must be built with CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n. If you
+need KCSAN to detect such a write even if that write did not change
+the value of foo, you also need CONFIG_KCSAN_REPORT_VALUE_CHANGE_ONLY=n.
+If you need KCSAN to detect such a write happening in an interrupt handler
+running on the same CPU doing the legitimate lock-protected write, you
+also need CONFIG_KCSAN_INTERRUPT_WATCHER=y. With some or all of these
+Kconfig options set properly, KCSAN can be quite helpful, although
+it is not necessarily a full replacement for hardware watchpoints.
+On the other hand, neither are hardware watchpoints a full replacement
+for KCSAN because it is not always easy to tell hardware watchpoint to
+conditionally trap on accesses.
Lock-Protected Writes With Lockless Reads
Powered by blists - more mailing lists