FAILED: patch "[PATCH] futex: Cure exit race" failed to apply to 4.14-stable tree
[email protected]
maintainerThe patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From da791a667536bf8322042e38ca85d55a78d3c273 Mon Sep 17 00:00:00 2001
From: Thomas Gleixner <[email protected]>
Date: Mon, 10 Dec 2018 14:35:14 +0100
Subject: [PATCH] futex: Cure exit race
Stefan reported, that the glibc tst-robustpi4 test case fails occasionally. That case creates the following race between sys_exit() and sys_futex_lock_pi():
CPU0 CPU1
sys_exit() sys_futex()
do_exit() futex_lock_pi()
exit_signals(tsk) No waiters:
tsk->flags |= PF_EXITING; *uaddr == 0x00000PID
mm_release(tsk) Set waiter bit
exit_robust_list(tsk) { *uaddr = 0x80000PID;
Set owner died attach_to_pi_owner() {
*uaddr = 0xC0000000; tsk = get_task(PID);
} if (!tsk->flags & PF_EXITING) {
... attach();
tsk->flags |= PF_EXITPIDONE; } else {
if (!(tsk->flags & PF_EXITPIDONE))
return -EAGAIN;
return -ESRCH; <--- FAIL
}ESRCH is returned all the way to user space, which triggers the glibc test case assert. Returning ESRCH unconditionally is wrong here because the user space value has been changed by the exiting task to 0xC0000000, i.e. the FUTEX_OWNER_DIED bit is set and the futex PID value has been cleared. This is a valid state and the kernel has to handle it, i.e. taking the futex.
Cure it by rereading the user space value when PF_EXITING and PF_EXITPIDONE is set in the task which 'owns' the futex. If the value has changed, let the kernel retry the operation, which includes all regular sanity checks and correctly handles the FUTEX_OWNER_DIED case.
If it hasn't changed, then return ESRCH as there is no way to distinguish this case from malfunctioning user space. This happens when the exiting task did not have a robust list, the robust list was corrupted or the user space value in the futex was simply bogus.
kernel/futex.c +63 -6
@@ -1148,11 +1148,65 @@ static int attach_to_pi_state(u32 __user *uaddr, u32 uval,return ret; } +static int handle_exit_race (u32 __user *uaddr,u32 uval, +struct task_struct *tsk) +{ +u32 uval2; + +/* +* If PF_EXITPIDONE is not yet set, then try again. +*/ +if (tsk && !(tsk->flags &PF_EXITPIDONE )) +return -EAGAIN ; + +/* +* Reread the user space value to handle the following situation: +* +* CPU0 CPU1 +* +* sys_exit() sys_futex() +* do_exit() futex_lock_pi() +* futex_lock_pi_atomic() +* exit_signals(tsk) No waiters: +* tsk->flags |= PF_EXITING; *uaddr == 0x00000PID +* mm_release(tsk) Set waiter bit +* exit_robust_list(tsk) { *uaddr = 0x80000PID; +* Set owner died attach_to_pi_owner() { +* *uaddr = 0xC0000000; tsk = get_task(PID); +* } if (!tsk->flags & PF_EXITING) { +* ... attach(); +* tsk->flags |= PF_EXITPIDONE; } else { +* if (!(tsk->flags & PF_EXITPIDONE)) +* return -EAGAIN; +* return -ESRCH; <--- FAIL +* } +* +* Returning ESRCH unconditionally is wrong here because the +* user space value has been changed by the exiting task. +* +* The same logic applies to the case where the exiting task is +* already gone. +*/ +if (get_futex_value_locked (&uval2, uaddr)) +return -EFAULT ; + +/* If the user space value has changed, try again. */ +if (uval2 != uval) +return -EAGAIN ; + +/* +* The exiting task did not have a robust list, the robust list was +* corrupted or the user space value in *uaddr is simply bogus. +* Give up and tell user space. +*/ +return -ESRCH ; +} +/* * Lookup the task for the TID provided from user space and attach to * it after doing proper sanity checks. */ -static int attach_to_pi_owner (u32 uval,union futex_key *key, +static int attach_to_pi_owner (u32 __user *uaddr,u32 uval,union futex_key *key,struct futex_pi_state **ps) {pid_t pid = uval &FUTEX_TID_MASK ;
@@ -1162,12 +1216,15 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,/* * We are the first waiter - try to look up the real owner and attach * the new pi_state to it, but bail out when TID = 0 [1] +* +* The !pid check is paranoid. None of the call sites should end up +* with pid == 0, but better safe than sorry. Let the caller retry */ if (!pid) -return -ESRCH ; +return -EAGAIN ; p =find_get_task_by_vpid (pid);if (!p) -return -ESRCH ; +return handle_exit_race (uaddr, uval,NULL );if (unlikely (p->flags &PF_KTHREAD )) {put_task_struct (p);
@@ -1187,7 +1244,7 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,* set, we know that the task has finished the * cleanup: */ -int ret = (p->flags &PF_EXITPIDONE ) ? -ESRCH : -EAGAIN ; +int ret =handle_exit_race (uaddr, uval, p);raw_spin_unlock_irq (&p->pi_lock);put_task_struct (p);
@@ -1244,7 +1301,7 @@ static int lookup_pi_state(u32 __user *uaddr, u32 uval,* We are the first waiter - try to look up the owner based on * @uval and attach to it. */ -return attach_to_pi_owner (uval, key, ps); +return attach_to_pi_owner (uaddr, uval, key, ps); }static int lock_pi_update_atomic (u32 __user *uaddr,u32 uval,u32 newval)
@@ -1352,7 +1409,7 @@ static int futex_lock_pi_atomic(u32 __user *uaddr, struct futex_hash_bucket *hb,* attach to the owner. If that fails, no harm done, we only * set the FUTEX_WAITERS bit in the user space variable. */ -return attach_to_pi_owner (uval, key, ps); +return attach_to_pi_owner (uaddr, newval, key, ps); }/**
Sudip Mukherjee
Acked-by Peter ZijlstraHi Greg,
Quoted 5 lines · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
The attached backported patch should apply to 4.14-stable tree.
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop. I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
--
Regards
Sudip
From 03cf90bf8a29dfd2bc3202ff8d322e9498058228 Mon Sep 17 00:00:00 2001
From: Thomas Gleixner <[email protected]>
Date: Mon, 10 Dec 2018 14:35:14 +0100
Subject: [PATCH] futex: Cure exit race
commit da791a667536bf8322042e38ca85d55a78d3c273 upstream
Stefan reported, that the glibc tst-robustpi4 test case fails occasionally. That case creates the following race between sys_exit() and sys_futex_lock_pi():
CPU0 CPU1
sys_exit() sys_futex()
do_exit() futex_lock_pi()
exit_signals(tsk) No waiters:
tsk->flags |= PF_EXITING; *uaddr == 0x00000PID
mm_release(tsk) Set waiter bit
exit_robust_list(tsk) { *uaddr = 0x80000PID;
Set owner died attach_to_pi_owner() {
*uaddr = 0xC0000000; tsk = get_task(PID);
} if (!tsk->flags & PF_EXITING) {
... attach();
tsk->flags |= PF_EXITPIDONE; } else {
if (!(tsk->flags & PF_EXITPIDONE))
return -EAGAIN;
return -ESRCH; <--- FAIL
}ESRCH is returned all the way to user space, which triggers the glibc test case assert. Returning ESRCH unconditionally is wrong here because the user space value has been changed by the exiting task to 0xC0000000, i.e. the FUTEX_OWNER_DIED bit is set and the futex PID value has been cleared. This is a valid state and the kernel has to handle it, i.e. taking the futex.
Cure it by rereading the user space value when PF_EXITING and PF_EXITPIDONE is set in the task which 'owns' the futex. If the value has changed, let the kernel retry the operation, which includes all regular sanity checks and correctly handles the FUTEX_OWNER_DIED case.
If it hasn't changed, then return ESRCH as there is no way to distinguish this case from malfunctioning user space. This happens when the exiting task did not have a robust list, the robust list was corrupted or the user space value in the futex was simply bogus.
--- kernel/futex.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 6 deletions(-)
kernel/futex.c +63 -6
@@ -1166,11 +1166,65 @@ static int attach_to_pi_state(u32 __user *uaddr, u32 uval,return ret; } +static int handle_exit_race (u32 __user *uaddr,u32 uval, +struct task_struct *tsk) +{ +u32 uval2; + +/* +* If PF_EXITPIDONE is not yet set, then try again. +*/ +if (tsk && !(tsk->flags &PF_EXITPIDONE )) +return -EAGAIN ; + +/* +* Reread the user space value to handle the following situation: +* +* CPU0 CPU1 +* +* sys_exit() sys_futex() +* do_exit() futex_lock_pi() +* futex_lock_pi_atomic() +* exit_signals(tsk) No waiters: +* tsk->flags |= PF_EXITING; *uaddr == 0x00000PID +* mm_release(tsk) Set waiter bit +* exit_robust_list(tsk) { *uaddr = 0x80000PID; +* Set owner died attach_to_pi_owner() { +* *uaddr = 0xC0000000; tsk = get_task(PID); +* } if (!tsk->flags & PF_EXITING) { +* ... attach(); +* tsk->flags |= PF_EXITPIDONE; } else { +* if (!(tsk->flags & PF_EXITPIDONE)) +* return -EAGAIN; +* return -ESRCH; <--- FAIL +* } +* +* Returning ESRCH unconditionally is wrong here because the +* user space value has been changed by the exiting task. +* +* The same logic applies to the case where the exiting task is +* already gone. +*/ +if (get_futex_value_locked (&uval2, uaddr)) +return -EFAULT ; + +/* If the user space value has changed, try again. */ +if (uval2 != uval) +return -EAGAIN ; + +/* +* The exiting task did not have a robust list, the robust list was +* corrupted or the user space value in *uaddr is simply bogus. +* Give up and tell user space. +*/ +return -ESRCH ; +} +/* * Lookup the task for the TID provided from user space and attach to * it after doing proper sanity checks. */ -static int attach_to_pi_owner (u32 uval,union futex_key *key, +static int attach_to_pi_owner (u32 __user *uaddr,u32 uval,union futex_key *key,struct futex_pi_state **ps) {pid_t pid = uval &FUTEX_TID_MASK ;
@@ -1180,12 +1234,15 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,/* * We are the first waiter - try to look up the real owner and attach * the new pi_state to it, but bail out when TID = 0 [1] +* +* The !pid check is paranoid. None of the call sites should end up +* with pid == 0, but better safe than sorry. Let the caller retry */ if (!pid) -return -ESRCH ; +return -EAGAIN ; p =futex_find_get_task (pid);if (!p) -return -ESRCH ; +return handle_exit_race (uaddr, uval,NULL );if (unlikely (p->flags &PF_KTHREAD )) {put_task_struct (p);
@@ -1205,7 +1262,7 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,* set, we know that the task has finished the * cleanup: */ -int ret = (p->flags &PF_EXITPIDONE ) ? -ESRCH : -EAGAIN ; +int ret =handle_exit_race (uaddr, uval, p);raw_spin_unlock_irq (&p->pi_lock);put_task_struct (p);
@@ -1262,7 +1319,7 @@ static int lookup_pi_state(u32 __user *uaddr, u32 uval,* We are the first waiter - try to look up the owner based on * @uval and attach to it. */ -return attach_to_pi_owner (uval, key, ps); +return attach_to_pi_owner (uaddr, uval, key, ps); }static int lock_pi_update_atomic (u32 __user *uaddr,u32 uval,u32 newval)
@@ -1370,7 +1427,7 @@ static int futex_lock_pi_atomic(u32 __user *uaddr, struct futex_hash_bucket *hb,* attach to the owner. If that fails, no harm done, we only * set the FUTEX_WAITERS bit in the user space variable. */ -return attach_to_pi_owner (uval, key, ps); +return attach_to_pi_owner (uaddr, newval, key, ps); }/**
Signature · 1 line
2.11.0
Quoted 9 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Quoted 7 lines
The attached backported patch should apply to 4.14-stable tree.
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop. I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
There are more patches in that area and you also need a fixed glibc.
Thanks,
tglx
Hi Thomas,
Quoted 8 lines · Thomas Gleixner
Quoted 5 lines · Sudip Mukherjee
Hi Greg,
Quoted 1 line · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
<snip>
Quoted 7 lines
Quoted 5 lines
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop. I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
There are more patches in that area and you also need a fixed glibc.
I can see 1a1fb985f2e2 ("futex: Handle early deadlock return correctly") is already there in 4.14-stable.
Is anything else missing, other than this one?
glibc might be a problem, but lets see what can be done.
Signature · 2 lines
Regards Sudip
Quoted 18 lines · Sudip Mukherjee
Hi Thomas,
Quoted 8 lines · Thomas Gleixner
Quoted 5 lines · Sudip Mukherjee
Hi Greg,
Quoted 1 line · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
<snip>
Quoted 6 lines
Quoted 5 lines
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop. I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
Quoted 7 lines
Quoted 1 line
There are more patches in that area and you also need a fixed glibc.
I can see
1a1fb985f2e2("futex: Handle early deadlock return correctly") is already there in 4.14-stable.
Is anything else missing, other than this one?glibc might be a problem, but lets see what can be done.
Those two are the kernel side of affairs I think.
The relevant glibc commits are:
8f9450a0b7a9e78267e8ae1ab1000ebca08e473e
823624bdc47f1f80109c9c52dee7939b9386d708
Thanks,
tglx
Hi Sudip,
Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Sudip Mukherjee
Hi Thomas,
Quoted 5 lines · Thomas Gleixner
Quoted 2 lines · Sudip Mukherjee
Hi Greg,
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
Quoted 1 line
<snip>
Quoted 3 lines
Quoted 3 lines
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop.
=> This behaviour depends on the configuration of assert.
See glibc code in nptl/pthread_mutex_lock.c (you will encounter either an abort due to assert or an indefinite loop):
/* ESRCH can happen only for non-robust PI mutexes where the owner of the lock died. */ assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH || !robust); /* Delay the thread indefinitely. */ while (1) __pause_nocancel ();
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
There are more patches in that area and you also need a fixed glibc.
I can see
1a1fb985f2e2("futex: Handle early deadlock return correctly") is already there in 4.14-stable.
Is anything else missing, other than this one?
Quoted 7 lines
Quoted 1 line
glibc might be a problem, but lets see what can be done.
Those two are the kernel side of affairs I think.
The relevant glibc commits are:
8f9450a0b7a9e78267e8ae1ab1000ebca08e473e
=> Needed for pthread_mutex_lock / pthread_mutex_timedlock (within glibc release 2.25)
Quoted 1 line
823624bdc47f1f80109c9c52dee7939b9386d708
=> Needed for pthread_mutex_trylock (will be within next glibc release 2.30, but is backported to glibc release branches 2.25 ... 2.29)
Bye
Stefan
Quoted 6 lines
Thanks,
tglx
Quoted 47 lines · Stefan Liebler
Hi Sudip,
Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Sudip Mukherjee
Hi Thomas,
Quoted 5 lines · Thomas Gleixner
Quoted 2 lines · Sudip Mukherjee
Hi Greg,
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
Quoted 1 line
<snip>
Quoted 3 lines
Quoted 3 lines
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop.
=> This behaviour depends on the configuration of assert. See glibc code in
nptl/pthread_mutex_lock.c(you will encounter either an abort due to assert or an indefinite loop):/* ESRCH can happen only for non-robust PI mutexes where the owner of the lock died. */ assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH || !robust); /* Delay the thread indefinitely. */ while (1) __pause_nocancel ();Quoted 15 lines
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
There are more patches in that area and you also need a fixed glibc.
I can see
1a1fb985f2e2("futex: Handle early deadlock return correctly") is already there in 4.14-stable.
Is anything else missing, other than this one?glibc might be a problem, but lets see what can be done.
Those two are the kernel side of affairs I think.
The relevant glibc commits are:
Quoted 7 lines
Quoted 1 line
8f9450a0b7a9e78267e8ae1ab1000ebca08e473e=> Needed for
pthread_mutex_lock/pthread_mutex_timedlock(within glibc release 2.25)Quoted 1 line
823624bdc47f1f80109c9c52dee7939b9386d708=> Needed for
pthread_mutex_trylock(will be within next glibc release 2.30, but is backported to glibc release branches 2.25 ... 2.29)
Thanks.
I tried with only the kernel changes and it was not resolved. Then I tried with both kernel changes and the glibc changes and I saw the problem improving significantly. But since we are using an ancient version of eglibc, I am not expecting it to get better than this.
Signature · 2 lines
Regards Sudip
Greg KH
maintainerQuoted 9 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Mon, Dec 24, 2018 at 12:52:22PM +0100, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Quoted 7 lines
The attached backported patch should apply to 4.14-stable tree.
I think we have a real usecase which is triggering this error and I was still in the middle of debugging that. But my initial analysis was showing that the userspace thread was stuck in the indefinite loop. I have a reliable reproducer of the problem and will setup a test tomorrow and confirm.
Now applied, thanks.
greg k-h