linux-kernel - Re: [PATCH] futex: bugfix for robust futex deadlock when waking only one thread in handle_futex

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]

Date:	Tue, 16 Apr 2013 10:05:28 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	zhang.yi20@....com.cn
CC:	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH] futex: bugfix for robust futex deadlock when waking only
 one thread in handle_futex_death

On 04/15/2013 08:02 PM, zhang.yi20@....com.cn wrote:
> From: Zhang Yi <zhang.yi20@....com.cn>
>
> Hello,

Hi Zhang,

I think you've attempted to address this with your second futex patch,
but please use plain text email on LKML, especially for patches. Ensure
to linewrap your plain text email and avoid mid-line breaks as it makes
the message very difficult to read.

(my mailer can't seem to be forced into replying in plain text mode for this
particular email, not sure what is special about this one).

> The function handle_futex_death just wakes one thread, which may be not
> enough when the owner process is dead. Think about this scene：
> 1. A robust futex is shared for two processes, each process has multi
> threads try to get the lock.
> 2. One of the threads gets the lock, and the others are waiting and sorted
> in order of priority.
> 3. The process to which the lock owner thread belongs is dying，and
> handle_futex_death is called to wake the first waiter
> 4. When the first waiter belongs to the same process，it has no chance to
> return to the userspace to get the lock and will not call
> handle_futex_death any
> more，and then the rest threads of the other process will never be waked,
> and
> will block forever.

A simple functional testcase of this would be really nice to add to
futextest:

https://git.kernel.org/cgit/linux/kernel/git/dvhart/futextest.git/

It would also be good for validating the fix and avoiding regressions.

>
> This patch wakes all the waiters when lock owner is in group-exit, letting
>
> all the waiters return to userspace and try to get the lock again.
>
>
> Signed-off-by: Zhang Yi <zhang.yi20@....com.cn>
> Tested-by: Ma Chenggong <ma.chenggong@....com.cn>
> Reviewed-by: Liu Dong <liu.dong3@....com.cn>
> Reviewed-by: Cui Yunfeng <cui.yunfeng@....com.cn>
> Reviewed-by: Lu Zhongjun <lu.zhongjun@....com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@....com.cn>
>
>
> --- orig/linux-3.9-rc7/kernel/futex.c   2013-04-15 00:45:16.000000000
> +0000
> +++ new/linux-3.9-rc7/kernel/futex.c    2013-04-16 10:17:46.264597000
> +0000
> @@ -2545,8 +2545,11 @@ retry:
>                  * Wake robust non-PI futexes here. The wakeup of
>                  * PI futexes happens in exit_pi_state():
>                  */
> -               if (!pi && (uval & FUTEX_WAITERS))
> -                       futex_wake(uaddr, 1, 1, FUTEX_BITSET_MATCH_ANY);
> +               if (!pi && (uval & FUTEX_WAITERS)) {
> +                       int nr = signal_group_exit(current->signal)
> +                                           ? INT_MAX : 1;
> +                       futex_wake(uaddr, 1, nr, FUTEX_BITSET_MATCH_ANY);
> +               }
>         }
>         return 0;
>  }

Performance isn't an issue here as this is an error path. The question
is if the
changed behavior will constitute a problem for existing applications. Rather
than a serialized cascading wake, we have them all wake at once. If an
application depended on the first waker after owner death to do some cleanup
before the rest came along, I could imagine some potential for failure
there.

One possible alternative would be to wake waiters for a different
process group
when OWNER_DEAD is set, and leave it as a single wake.

Peter, Ingo - do you think there is any concern over the wake_all versus
wake_one? I suspect it should be fine, but we obviously need to be
careful when changing the behavior of syscalls.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel
--
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/