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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH -next] locking/percpu-rwsem: fix a task_struct refcount
From:   Qian Cai <cai@lca.pw>
In-Reply-To: <20200330111852.GH20696@hirez.programming.kicks-ass.net>
Date:   Mon, 30 Mar 2020 09:18:25 -0400
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        dbueso@suse.de, juri.lelli@redhat.com,
        Waiman Long <longman@redhat.com>, linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <37BFF03B-470A-46B4-91AB-8A8A64FEF7B8@lca.pw>
References: <20200327093754.GS20713@hirez.programming.kicks-ass.net>
 <BB30C711-B54C-4D61-8BEE-A55F410C4178@lca.pw>
 <20200330111852.GH20696@hirez.programming.kicks-ass.net>
To:     Peter Zijlstra <peterz@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Mar 30, 2020, at 7:18 AM, Peter Zijlstra <peterz@infradead.org> =
wrote:
>=20
> On Fri, Mar 27, 2020 at 06:19:37AM -0400, Qian Cai wrote:
>>=20
>>=20
>>> On Mar 27, 2020, at 5:37 AM, Peter Zijlstra <peterz@infradead.org> =
wrote:
>>>=20
>>> 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?
>>=20
>> Interesting, I thought this was more straightforward to see,
>=20
> 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 :/
>=20
> 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.
>=20
>> 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?
>=20
> 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.
>=20
> Yes, you're right.
>=20
> 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.

Looks good to me. If no one has any objection, I=E2=80=99ll dust-out the =
commit log
and send out a v2 for it.=20

>=20
> Hmm?
>=20
> 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 =3D get_task_struct(wq_entry->private);
> 	bool reader =3D wq_entry->flags & WQ_FLAG_CUSTOM;
> 	struct percpu_rw_semaphore *sem =3D key;
> +	struct task_struct *p;
>=20
> 	/* concurrent against percpu_down_write(), can get stolen */
> 	if (!__percpu_rwsem_trylock(sem, reader))
> 		return 1;
>=20
> +	p =3D get_task_struct(wq_entry->private);
> 	list_del_init(&wq_entry->entry);
> 	smp_store_release(&wq_entry->private, NULL);
>=20