linux-kernel - Re: [PATCH 0/1] futex: check PF_KTHREAD rather than !p->mm to filter out kthreads

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150206104658.GI23123@twins.programming.kicks-ass.net>
Date:	Fri, 6 Feb 2015 11:46:58 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Darren Hart <darren@...art.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jerome Marchand <jmarchan@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Mateusz Guzik <mguzik@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] futex: check PF_KTHREAD rather than !p->mm to filter
 out kthreads

On Thu, Feb 05, 2015 at 07:10:14PM +0100, Oleg Nesterov wrote:
> Let me first say that I simply do not know if PI+robust futex is actually
> supposed (or guaranteed) to work.

> Now, if it should work,

I 'think' it _should_ work. Afaict the glibc code sees this as a valid
combination.

> On 02/05, Peter Zijlstra wrote:
> >
> > So as long as we unhash _last_ I can't see this happening, we'll always
> > find the task, the robust list walk doesn't care about PI state.
> 
> and it simply can't take care of PI state. ->pi_state can be NULL by
> the time exit_robust_list() is called.
> 
> > But please, if you suspect, share a little more detail on how you see
> > this happening, this is not code I've looked at in detail before.
> 
> Heh, I am reading it for the first time ;) So I can be easily wrong.
> 
> But afaics the race/problem is very simple. Suppose a task T locks a PI+robust
> mutex and exits. I this case (I presume) sys_futex(uaddr, FUTEX_LOCK_PI)
> from another task X must always succeed sooner or later. But
> 
> 	- X takes queue_lock() and reads *uaddr == T->pid. Need to setup
> 	  pi_state and wait. FUTEX_WAITERS is set.
> 
> 	- T exits and calls handle_futex_death(). This clears FUTEX_TID_MASK
> 	  and sets FUTEX_OWNER_DIED, without any lock.
> 
> 	  T->pi_state_list is empty, exit_pi_state_list() does nothing.

Right, because T acquired the lock from userspace and there have not yet
been any waiters, so there's no pi state.

> 	  T goes away or simply sets PF_EXITPIDONE (lets ignore PF_EXITING).
> 
> 	- X calls attach_to_pi_owner() and futex_find_get_task() returns NULL,
> 	  or we detect PF_EXITPIDONE, this doesn't really matter.
> 
> 	  What does matter (unless I missed something) is that -ESRCH is wrong
> 	  in this case. This mutex was unlocked. It is robust, so we should not
> 	  miss this unlock.

Right,..

> So I think that in this case we either need to recheck that *uaddr is still the
> same (and turn -ESRCH into -EAGAIN otherwise), or change handle_futex_death() to
> serialize with X so that it can proceed and attach pi_state.
> 
> No?

I _think_ you're right, doing -ESRCH is wrong without first looking to
see if uval changed and gained an FUTEX_OWNER_DIED.

I don't think making handle_futex_death() wait on hb lock works because
of the -EAGAIN loop releasing that lock.

Now, I think Darren actually had a futex test suite; Darren can you add
a robust-pi test like the above to stress this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/