Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 30 Mar 2020 13:18:52 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Qian Cai <cai@lca.pw>
Cc:     mingo@redhat.com, will@kernel.org, dbueso@suse.de,
        juri.lelli@redhat.com, longman@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] locking/percpu-rwsem: fix a task_struct refcount
Message-ID: <20200330111852.GH20696@hirez.programming.kicks-ass.net>
References: <20200327093754.GS20713@hirez.programming.kicks-ass.net>
 <BB30C711-B54C-4D61-8BEE-A55F410C4178@lca.pw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BB30C711-B54C-4D61-8BEE-A55F410C4178@lca.pw>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 27, 2020 at 06:19:37AM -0400, Qian Cai wrote:
> 
> 
> > On Mar 27, 2020, at 5:37 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > If the trylock fails, someone else got the lock and we remain on the
> > waitqueue. It seems like a very bad idea to put the task while it
> > remains on the waitqueue, no?
> 
> Interesting, I thought this was more straightforward to see,

It is indeed as straight forward as you explain; but when doing 10
things at once, and having just dug through some low-level arch assembly
code for the previous email, even obvious things might sometimes need
a little explaining :/

So please, always try and err on the side of a little verbose when
writing Changelogs, esp. when concerning locking / concurrency, you
really can't be clear enough.

> but I may
> be wrong as always. At the beginning of percpu_rwsem_wake_function()
> it calls get_task_struct(), but if the trylock failed, it will remain
> in the waitqueue. However, it will run percpu_rwsem_wake_function()
> again with get_task_struct() to increase the refcount. Can you
> enlighten me where it will call put_task_struct() in waitqueue or
> elsewhere to balance the refcount in this case?

See, had that explaination been part of the Changelog, my brain would've
probably been able to kick itself in gear and actually spot the problem.

Yes, you're right.

That said, I wonder if we can just move the get_task_struct() call like
below; after all the race we're guarding against is percpu_rwsem_wait()
observing !private, terminating the wait and doing a quick exit() while
percpu_rwsem_wake_function() then does wake_up_process(p) as a
use-after-free.

Hmm?

diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index a008a1ba21a7..8bbafe3e5203 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -118,14 +118,15 @@ static int percpu_rwsem_wake_function(struct wait_queue_entry *wq_entry,
 				      unsigned int mode, int wake_flags,
 				      void *key)
 {
-	struct task_struct *p = get_task_struct(wq_entry->private);
 	bool reader = wq_entry->flags & WQ_FLAG_CUSTOM;
 	struct percpu_rw_semaphore *sem = key;
+	struct task_struct *p;
 
 	/* concurrent against percpu_down_write(), can get stolen */
 	if (!__percpu_rwsem_trylock(sem, reader))
 		return 1;
 
+	p = get_task_struct(wq_entry->private);
 	list_del_init(&wq_entry->entry);
 	smp_store_release(&wq_entry->private, NULL);