LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Qian Cai <cai@lca.pw>
Cc: Will Deacon <will@kernel.org>, Elver Marco <elver@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>
Subject: Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock
Date: Sat, 9 May 2020 14:36:54 -0700
Message-ID: <20200509213654.GO2869@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <45D9EEEB-D887-485D-9045-417A7F2C6A1A@lca.pw>

On Sat, May 09, 2020 at 12:53:38PM -0400, Qian Cai wrote:
> 
> 
> > On May 9, 2020, at 12:12 PM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > 
> > Ah, and I forgot to ask.  Why "if (data_race(prev->next == node)" instead
> > of "if (data_race(prev->next) == node"?
> 
> I think the one you suggested is slightly better to point out the exact race. Do you want me to resend or you could squash it instead?

The patch was still at the top of my stack, so I just amended it.  Here
is the updated version.

							Thanx, Paul

------------------------------------------------------------------------

commit 13e69ca01ce1621ce74248bda86cfad47fa5a0fa
Author: Qian Cai <cai@lca.pw>
Date:   Tue Feb 11 08:54:15 2020 -0500

    locking/osq_lock: Annotate a data race in osq_lock
    
    The prev->next pointer can be accessed concurrently as noticed by KCSAN:
    
     write (marked) to 0xffff9d3370dbbe40 of 8 bytes by task 3294 on cpu 107:
      osq_lock+0x25f/0x350
      osq_wait_next at kernel/locking/osq_lock.c:79
      (inlined by) osq_lock at kernel/locking/osq_lock.c:185
      rwsem_optimistic_spin
      <snip>
    
     read to 0xffff9d3370dbbe40 of 8 bytes by task 3398 on cpu 100:
      osq_lock+0x196/0x350
      osq_lock at kernel/locking/osq_lock.c:157
      rwsem_optimistic_spin
      <snip>
    
    Since the write only stores NULL to prev->next and the read tests if
    prev->next equals to this_cpu_ptr(&osq_node). Even if the value is
    shattered, the code is still working correctly. Thus, mark it as an
    intentional data race using the data_race() macro.
    
    Signed-off-by: Qian Cai <cai@lca.pw>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index 1f77349..1de006e 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -154,7 +154,11 @@ bool osq_lock(struct optimistic_spin_queue *lock)
 	 */
 
 	for (;;) {
-		if (prev->next == node &&
+		/*
+		 * cpu_relax() below implies a compiler barrier which would
+		 * prevent this comparison being optimized away.
+		 */
+		if (data_race(prev->next) == node &&
 		    cmpxchg(&prev->next, node, NULL) == node)
 			break;
 

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 13:54 Qian Cai
2020-05-08 20:59 ` Qian Cai
2020-05-09  4:33   ` Paul E. McKenney
2020-05-09 13:01     ` Qian Cai
2020-05-09 16:12       ` Paul E. McKenney
2020-05-09 16:53         ` Qian Cai
2020-05-09 21:36           ` Paul E. McKenney [this message]
2020-05-11 15:58             ` Will Deacon
2020-05-11 16:43               ` Paul E. McKenney
2020-05-11 16:52                 ` Will Deacon
2020-05-11 17:29                   ` Paul E. McKenney
2020-05-11 17:34                     ` Will Deacon
2020-05-11 18:07                       ` Paul E. McKenney
2020-05-11 16:44               ` Qian Cai
2020-05-11 16:54                 ` Will Deacon
2020-05-11 17:10                   ` Qian Cai

Reply instructions:

You may reply publicly 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=20200509213654.GO2869@paulmck-ThinkPad-P72 \
    --to=paulmck@kernel.org \
    --cc=cai@lca.pw \
    --cc=elver@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=will@kernel.org \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

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

Example config snippet for mirrors

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


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