Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Marco Elver <elver@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Eric Dumazet <edumazet@google.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	syzbot <syzbot+3ef049d50587836c0606@syzkaller.appspotmail.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrea Parri <parri.andrea@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	LKMM Maintainers -- Akira Yokosawa <akiyks@gmail.com>
Subject: Re: KCSAN: data-race in __alloc_file / __alloc_file
Date: Wed, 13 Nov 2019 22:33:36 +0100
Message-ID: <20191113213336.GA20665@google.com> (raw)
In-Reply-To: <CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com>

On Wed, 13 Nov 2019, Linus Torvalds wrote:

> On Wed, Nov 13, 2019 at 7:00 AM Marco Elver <elver@google.com> wrote:
> >
> > Just to summarize the options we've had so far:
> > 1. Add a comment, and let the tool parse it somehow.
> > 2. Add attribute to variables.
> > 3. Add some new macro to use with expressions, which doesn't do
> > anything if the tool is disabled. E.g. "racy__(counter++)",
> > "lossy__(counter++);" or any suitable name.
> 
> I guess I could live with "data_race(x)" or something simple like
> that, assuming we really can just surround a whole expression with it,
> and we don't have to make a hundred different versions for the
> different cases ("racy plain assignment" vs "racy statistics update"
> vs "racy u64 addition" etc etc).
> 
> I just want the source code to be very legible, which is one of the
> problems with the ugly READ_ONCE() conversions.
> 
> Part of that "legible source code" implies no crazy double
> underscores. But a plain "data_race(x)" might not look too bad, and
> would be easy to grep for, and doesn't seem to exist in the current
> kernel as anything else.
> 
> One question is if it would be a statement expression or an actual
> expression. I think the expression would read much better, IOW you
> could do
> 
>     val = data_race(p->field);
> 
> instead of having to write it as
> 
>     data_race(val = p->field);
> 
> to really point out the race. But at the same time, maybe you need to
> surround several statements, ie
> 
>     // intentionally racy xchg because we don't care and it generates
> better code
>     data_race(a = p->field; p->field = b);
> 
> which all would work fine with a non-instrumented macro something like this:
> 
>     #define data_race(x) ({ x; })
> 
> which would hopefully give the proper syntax rules.
> 
> But that might be very very inconvenient for KCSAN, depending on how
> you annotate the thing.
> 
> So I _suspect_ that what you actually want is to do it as a statement,
> not as an expression. What's the actual underlying syntax for "ignore
> this code for thread safety checking"?

An expression works fine. The below patch would work with KCSAN, and all
your above examples work.

Re name: would it make sense to more directly convey the intent?  I.e.
"this expression can race, and it's fine that the result is approximate
if it does"?

My vote would go to something like 'smp_lossy' or 'lossy_race' -- but
don't have a strong preference, and would also be fine with 'data_race'.
Whatever is most legible.  Comments?

Thanks,
-- Marco


diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 0b6506b9dd11..4d0597f89168 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -308,6 +308,29 @@ unsigned long read_word_at_a_time(const void *addr)
 	__u.__val;					\
 })
 
+#include <linux/kcsan.h>
+
+/*
+ * smp_lossy: macro that provides a way to document the intent that an
+ * expression may be lossy or approximate on an SMP system due to data races.
+ *
+ * This macro *does not* affect normal code generation, but is a hint to tooling
+ * that it is intentional that accesses in the expression may conflict with
+ * concurrent accesses.
+ */
+#ifdef __SANITIZE_THREAD__
+#define smp_lossy(expr)                                                        \
+	({                                                                     \
+		typeof(({ expr; })) __val;                                     \
+		kcsan_nestable_atomic_begin();                                 \
+		__val = ({ expr; });                                           \
+		kcsan_nestable_atomic_end();                                   \
+		__val;                                                         \
+	})
+#else
+#define smp_lossy(expr) ({ expr; })
+#endif
+
 #endif /* __KERNEL__ */
 
 /*

  reply index

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAHk-=wjB61GNmqpX0BLA5tpL4tsjWV7akaTc2Roth7uGgax+mw@mail.gmail.com>
2019-11-10 16:09 ` Alan Stern
2019-11-10 19:10   ` Marco Elver
2019-11-11 15:51     ` Alan Stern
2019-11-11 16:51       ` Linus Torvalds
2019-11-11 17:52         ` Eric Dumazet
2019-11-11 18:04           ` Linus Torvalds
2019-11-11 18:31             ` Eric Dumazet
2019-11-11 18:44               ` Eric Dumazet
2019-11-11 19:00                 ` Linus Torvalds
2019-11-11 19:13                   ` Eric Dumazet
2019-11-11 20:43                     ` Linus Torvalds
2019-11-11 20:46                       ` Linus Torvalds
2019-11-11 21:53                         ` Eric Dumazet
2019-11-11 23:51                   ` Linus Torvalds
2019-11-12 16:50                     ` Kirill Smelkov
2019-11-12 17:23                       ` Linus Torvalds
2019-11-12 17:36                         ` Linus Torvalds
2019-11-17 18:56                           ` Kirill Smelkov
2019-11-17 19:20                             ` Linus Torvalds
2019-11-11 18:50               ` Linus Torvalds
2019-11-11 18:59                 ` Marco Elver
2019-11-11 18:59                 ` Eric Dumazet
2019-11-10 19:12   ` Linus Torvalds
2019-11-10 19:20     ` Linus Torvalds
2019-11-10 20:44       ` Paul E. McKenney
2019-11-10 21:10         ` Linus Torvalds
2019-11-10 21:31           ` Paul E. McKenney
2019-11-11 14:17         ` Marco Elver
2019-11-11 14:31           ` Paul E. McKenney
2019-11-11 15:10             ` Marco Elver
2019-11-13  0:25               ` Paul E. McKenney
2019-11-12 19:14     ` Alan Stern
2019-11-12 19:47       ` Linus Torvalds
2019-11-12 20:29         ` Alan Stern
2019-11-12 20:58           ` Linus Torvalds
2019-11-12 21:13             ` Linus Torvalds
2019-11-12 22:05               ` Marco Elver
2019-11-12 21:48             ` Alan Stern
2019-11-12 22:07               ` Eric Dumazet
2019-11-12 22:44                 ` Alexei Starovoitov
2019-11-12 23:17                   ` Eric Dumazet
2019-11-12 23:40                     ` Linus Torvalds
2019-11-13 15:00                       ` Marco Elver
2019-11-13 16:57                         ` Linus Torvalds
2019-11-13 21:33                           ` Marco Elver [this message]
2019-11-13 21:50                             ` Alan Stern
2019-11-13 22:48                               ` Marco Elver
2019-11-08 13:16 syzbot
2019-11-08 13:28 ` Eric Dumazet
2019-11-08 17:01   ` Linus Torvalds
2019-11-08 17:22     ` Eric Dumazet
2019-11-08 17:38       ` Linus Torvalds
2019-11-08 17:53         ` Eric Dumazet
2019-11-08 17:55           ` Eric Dumazet
2019-11-08 18:02             ` Eric Dumazet
2019-11-08 18:12               ` Linus Torvalds
2019-11-08 20:30             ` Linus Torvalds
2019-11-08 20:53               ` Eric Dumazet
2019-11-08 21:36                 ` Linus Torvalds
2019-11-08 18:05           ` Linus Torvalds
2019-11-08 18:15             ` Marco Elver
2019-11-08 18:40               ` Linus Torvalds
2019-11-08 19:48                 ` Marco Elver
2019-11-08 20:26                   ` Linus Torvalds
2019-11-08 21:57                     ` Alan Stern
2019-11-08 22:06                       ` Linus Torvalds
2019-11-09 23:08                         ` Alan Stern

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191113213336.GA20665@google.com \
    --to=elver@google.com \
    --cc=akiyks@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=parri.andrea@gmail.com \
    --cc=paulmck@kernel.org \
    --cc=stern@rowland.harvard.edu \
    --cc=syzbot+3ef049d50587836c0606@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git