Messages in this thread | | | Date | Thu, 5 Feb 2015 19:10:14 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/1] futex: check PF_KTHREAD rather than !p->mm to filter out kthreads |
| |
Let me first say that I simply do not know if PI+robust futex is actually supposed (or guaranteed) to work.
Documentation/pi-futex.txt says
'robustness' and 'PI' are two orthogonal properties of futexes, and all four combinations are possible: futex, robust-futex, PI-futex, robust+PI-futex.
And exit_robust_list() checks bit 0 to detect the "PI" case, so I think this should work.
However, this comment
/* * This task is holding PI mutexes at exit time => bad. * Kernel cleans up PI-state, but userspace is likely hosed. * (Robust-futex cleanup is separate and might save the day for userspace.) */
above exit_pi_state_list() looks confusing. In fact it looks wrong if PI+robust should work. Because handle_futex_death() seems to rely on exit_pi_state_list.
Now, if it should work,
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.
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.
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?
Oleg.
| |