linux-kernel - Re: [PATCH -next] locking/percpu-rwsem: fix a task

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <37BFF03B-470A-46B4-91AB-8A8A64FEF7B8@lca.pw>
Date:   Mon, 30 Mar 2020 09:18:25 -0400
From:   Qian Cai <cai@....pw>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        dbueso@...e.de, juri.lelli@...hat.com,
        Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] locking/percpu-rwsem: fix a task_struct refcount



> On Mar 30, 2020, at 7:18 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> On Fri, Mar 27, 2020 at 06:19:37AM -0400, Qian Cai wrote:
>> 
>> 
>>> On Mar 27, 2020, at 5:37 AM, Peter Zijlstra <peterz@...radead.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.

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

> 
> 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);
>