handle_exit_race && PF_EXITING
Commit Message
On 11/06, Thomas Gleixner wrote:
>
> But even if we adapt that patch to the current code it won't solve the
> -ESRCH issue I described above.
Hmm. I must have missed something. Why?
Please see the unfinished/untested patch below.
I think that (with or without this fix) handle_exit_race() logic needs
cleanups, there is no reason for get_futex_value_locked(), we can drop
->pi_lock right after we see PF_EXITPIDONE. Lets discuss this later.
But why we can not rely on handle_exit_race() without PF_EXITING check?
Yes, exit_robust_list() is called before exit_pi_state_list() which (with
this patch) sets PF_EXITPIDONE. But this is fine? handle_futex_death()
doesn't wakeup pi futexes, exit_pi_state_list() does this.
Could you explain?
Oleg.
Comments
On Wed, 6 Nov 2019, Oleg Nesterov wrote:
> But why we can not rely on handle_exit_race() without PF_EXITING check?
>
> Yes, exit_robust_list() is called before exit_pi_state_list() which (with
> this patch) sets PF_EXITPIDONE. But this is fine? handle_futex_death()
> doesn't wakeup pi futexes, exit_pi_state_list() does this.
I know. You still create inconsistent state because of this:
> raw_spin_lock_irq(&p->pi_lock);
> - if (unlikely(p->flags & PF_EXITING)) {
> + if (unlikely(p->flags & PF_EXITPIDONE)) {
> /*
> * The task is on the way out. When PF_EXITPIDONE is
> * set, we know that the task has finished the
> * cleanup:
> */
> - int ret = handle_exit_race(uaddr, uval, p);
> + int ret = handle_exit_race(uaddr, uval);
>
> raw_spin_unlock_irq(&p->pi_lock);
> put_task_struct(p);
Same explanation as before just not prosa this time:
exit() lock_pi(futex2)
exit_pi_state_list()
lock(tsk->pi_lock)
tsk->flags |= PF_EXITPIDONE; attach_to_pi_owner()
...
// Loop unrolled for clarity
while(!list_empty()) lock(tsk->pi_lock);
cleanup(futex1)
unlock(tsk->pi_lock)
... if (tsk->flags & PF_EXITPIDONE)
ret = handle_exit_race()
if (uval != orig_uval)
return -EAGAIN;
return -ESRCH;
cleanup(futex2) return to userspace err = -ESRCH
update futex2->uval
with new owner TID and set
OWNER_DIED
userspace handles -ESRCH
but futex2->uval has a valid TID
and OWNER_DIED set.
That's inconsistent state, the futex became a SNAFUtex and user space
cannot recover from that. At least not existing user space and we cannot
break that, right?
If the kernel returns -ESRCH then the futex value must be preserved (except
for the waiters bit being set, but that's not part of the problem at hand).
You cannot just look at the kernel state with futexes. We have to guarantee
consistent state between kernel and user space no matter what. And of
course we have to be careful about user space creating inconsistent state
for stupid or malicious reasons. See the whole state table above
attach_to_pi_state().
The only way to fix this live lock issue is to gracefully wait for the
cleanup to complete instead of busy looping.
Yes, it sucks, but futexes suck by definition.
Thanks,
tglx
On Wed, 6 Nov 2019, Oleg Nesterov wrote:
>
> I think that (with or without this fix) handle_exit_race() logic needs
> cleanups, there is no reason for get_futex_value_locked(), we can drop
> ->pi_lock right after we see PF_EXITPIDONE. Lets discuss this later.
Which still is in atomic because the hash bucket lock is held, ergo
get_futex_value_locked() needs to stay for now.
So the only thing we could do is to reduce the pi_lock held section a bit.
Thanks,
tglx
On 11/06, Thomas Gleixner wrote:
>
> On Wed, 6 Nov 2019, Oleg Nesterov wrote:
> >
> > I think that (with or without this fix) handle_exit_race() logic needs
> > cleanups, there is no reason for get_futex_value_locked(), we can drop
> > ->pi_lock right after we see PF_EXITPIDONE. Lets discuss this later.
>
> Which still is in atomic because the hash bucket lock is held, ergo
> get_futex_value_locked() needs to stay for now.
Indeed, you are right.
> Same explanation as before just not prosa this time:
>
> exit() lock_pi(futex2)
> exit_pi_state_list()
> lock(tsk->pi_lock)
> tsk->flags |= PF_EXITPIDONE; attach_to_pi_owner()
> ...
> // Loop unrolled for clarity
> while(!list_empty()) lock(tsk->pi_lock);
> cleanup(futex1)
> unlock(tsk->pi_lock)
^^^^^^^^^^^^^^^^^^^^
Ah! Thanks.
Hmm. In particular, exit_pi_state() drops pi_lock if refcount_inc_not_zero() fails.
Isn't this another potential source of livelock ?
Suppose that a realtime lock owner X sleeps somewhere, another task T
calls put_pi_state(), refcount_dec_and_test() succeeds.
What if, say, X is killed right after that and preempts T on the same
CPU?
Oleg.
@@ -761,17 +761,6 @@ void __noreturn do_exit(long code)
}
exit_signals(tsk); /* sets PF_EXITING */
- /*
- * Ensure that all new tsk->pi_lock acquisitions must observe
- * PF_EXITING. Serializes against futex.c:attach_to_pi_owner().
- */
- smp_mb();
- /*
- * Ensure that we must observe the pi_state in exit_mm() ->
- * mm_release() -> exit_pi_state_list().
- */
- raw_spin_lock_irq(&tsk->pi_lock);
- raw_spin_unlock_irq(&tsk->pi_lock);
if (unlikely(in_atomic())) {
pr_info("note: %s[%d] exited with preempt_count %d\n",
@@ -846,12 +835,6 @@ void __noreturn do_exit(long code)
* Make sure we are holding no locks:
*/
debug_check_no_locks_held();
- /*
- * We can do this unlocked here. The futex code uses this flag
- * just to verify whether the pi state cleanup has been done
- * or not. In the worst case it loops once more.
- */
- tsk->flags |= PF_EXITPIDONE;
if (tsk->io_context)
exit_io_context(tsk);
@@ -1297,8 +1297,7 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
tsk->compat_robust_list = NULL;
}
#endif
- if (unlikely(!list_empty(&tsk->pi_state_list)))
- exit_pi_state_list(tsk);
+ exit_pi_state_list(tsk);
#endif
uprobe_free_utask(tsk);
@@ -905,6 +905,7 @@ void exit_pi_state_list(struct task_struct *curr)
* versus waiters unqueueing themselves:
*/
raw_spin_lock_irq(&curr->pi_lock);
+ curr->flags |= PF_EXITPIDONE;
while (!list_empty(head)) {
next = head->next;
pi_state = list_entry(next, struct futex_pi_state, list);
@@ -1169,18 +1170,11 @@ 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)
+static int handle_exit_race(u32 __user *uaddr, u32 uval)
{
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
@@ -1245,7 +1239,7 @@ static int attach_to_pi_owner(u32 __user *uaddr, u32 uval, union futex_key *key,
return -EAGAIN;
p = find_get_task_by_vpid(pid);
if (!p)
- return handle_exit_race(uaddr, uval, NULL);
+ return handle_exit_race(uaddr, uval);
if (unlikely(p->flags & PF_KTHREAD)) {
put_task_struct(p);
@@ -1259,13 +1253,13 @@ static int attach_to_pi_owner(u32 __user *uaddr, u32 uval, union futex_key *key,
* p->pi_lock:
*/
raw_spin_lock_irq(&p->pi_lock);
- if (unlikely(p->flags & PF_EXITING)) {
+ if (unlikely(p->flags & PF_EXITPIDONE)) {
/*
* The task is on the way out. When PF_EXITPIDONE is
* set, we know that the task has finished the
* cleanup:
*/
- int ret = handle_exit_race(uaddr, uval, p);
+ int ret = handle_exit_race(uaddr, uval);
raw_spin_unlock_irq(&p->pi_lock);
put_task_struct(p);