linux-kernel - Re: [patch 3/4] pi-futex: Fix exit races and locking problems

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: <20070608071611.GB30226@elte.hu>
Date:	Fri, 8 Jun 2007 09:16:11 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	Ulrich Drepper <drepper@...hat.com>
Subject: Re: [patch 3/4] pi-futex: Fix exit races and locking problems


* Thomas Gleixner <tglx@...utronix.de> wrote:

> From: Alexey Kuznetsov <kuznet@....inr.ac.ru>
> 
> 1. New entries can be added to tsk->pi_state_list after task completed
>    exit_pi_state_list(). The result is memory leakage and deadlocks.
> 
> 2. handle_mm_fault() is called under spinlock. The result is obvious.
> 
> 3. results in self-inflicted deadlock inside glibc.
>    Sometimes futex_lock_pi returns -ESRCH, when it is not expected
>    and glibc enters to for(;;) sleep() to simulate deadlock. This problem
>    is quite obvious and I think the patch is right. Though it looks like
>    each "if" in futex_lock_pi() got some stupid special case "else if". :-)
> 
> 4. sometimes futex_lock_pi() returns -EDEADLK,
>    when nobody has the lock. The reason is also obvious (see comment
>    in the patch), but correct fix is far beyond my comprehension.
>    I guess someone already saw this, the chunk:
> 
>                         if (rt_mutex_trylock(&q.pi_state->pi_mutex))
>                                 ret = 0;
> 
>    is obviously from the same opera. But it does not work, because the
>    rtmutex is really taken at this point: wake_futex_pi() of previous
>    owner reassigned it to us. My fix works. But it looks very stupid.
>    I would think about removal of shift of ownership in wake_futex_pi()
>    and making all the work in context of process taking lock.
> 
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> Fix 1) Avoid the tasklist lock variant of the exit race fix by adding
>     an additional state transition to the exit code.
> 
>     This fixes also the issue, when a task with recursive segfaults
>     is not able to release the futexes.
> 
> Fix 2) Cleanup the lookup_pi_state() failure path and solve the -ESRCH
>     problem finally.
> 
> Fix 3) Solve the fixup_pi_state_owner() problem which needs to do the fixup
>     in the lock protected section by using the in_atomic userspace access
>     functions.
> 	
>     This removes also the ugly lock drop / unqueue inside of fixup_pi_state()
> 
> Fix 4) Fix a stale lock in the error path of futex_wake_pi()
> 
> Added some error checks for verification.
> 
> The -EDEADLK problem is solved by the rtmutex fixups.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>

i've read the patch and it looks all good - but obviously testing will 
tell us what's up, this stuff is ... complex.

Acked-by: Ingo Molnar <mingo@...e.hu>

	Ingo
-
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/