futex: Fix pi_state->owner serialization
There was a reported suspicion about a race between exit_pi_state_list() and put_pi_state(). The same report mentioned the comment with put_pi_state() said it should be called with hb->lock held, and it no longer is in all places. As it turns out, the pi_state->owner serialization is indeed broken. As per the new rules:734009e96d
("futex: Change locking rules") pi_state->owner should be serialized by pi_state->pi_mutex.wait_lock. For the sites setting pi_state->owner we already hold wait_lock (where required) but exit_pi_state_list() and put_pi_state() were not and raced on clearing it. Fixes:734009e96d
("futex: Change locking rules") Reported-by: Gratian Crisan <gratian.crisan@ni.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: dvhart@infradead.org Cc: stable@vger.kernel.org Link: https://lkml.kernel.org/r/20170922154806.jd3ffltfk24m4o4y@hirez.programming.kicks-ass.net
This commit is contained in:
parent
e19b205be4
commit
c74aef2d06
|
@ -821,8 +821,6 @@ static void get_pi_state(struct futex_pi_state *pi_state)
|
||||||
/*
|
/*
|
||||||
* Drops a reference to the pi_state object and frees or caches it
|
* Drops a reference to the pi_state object and frees or caches it
|
||||||
* when the last reference is gone.
|
* when the last reference is gone.
|
||||||
*
|
|
||||||
* Must be called with the hb lock held.
|
|
||||||
*/
|
*/
|
||||||
static void put_pi_state(struct futex_pi_state *pi_state)
|
static void put_pi_state(struct futex_pi_state *pi_state)
|
||||||
{
|
{
|
||||||
|
@ -837,16 +835,22 @@ static void put_pi_state(struct futex_pi_state *pi_state)
|
||||||
* and has cleaned up the pi_state already
|
* and has cleaned up the pi_state already
|
||||||
*/
|
*/
|
||||||
if (pi_state->owner) {
|
if (pi_state->owner) {
|
||||||
raw_spin_lock_irq(&pi_state->owner->pi_lock);
|
struct task_struct *owner;
|
||||||
list_del_init(&pi_state->list);
|
|
||||||
raw_spin_unlock_irq(&pi_state->owner->pi_lock);
|
|
||||||
|
|
||||||
rt_mutex_proxy_unlock(&pi_state->pi_mutex, pi_state->owner);
|
raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
|
||||||
|
owner = pi_state->owner;
|
||||||
|
if (owner) {
|
||||||
|
raw_spin_lock(&owner->pi_lock);
|
||||||
|
list_del_init(&pi_state->list);
|
||||||
|
raw_spin_unlock(&owner->pi_lock);
|
||||||
|
}
|
||||||
|
rt_mutex_proxy_unlock(&pi_state->pi_mutex, owner);
|
||||||
|
raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (current->pi_state_cache)
|
if (current->pi_state_cache) {
|
||||||
kfree(pi_state);
|
kfree(pi_state);
|
||||||
else {
|
} else {
|
||||||
/*
|
/*
|
||||||
* pi_state->list is already empty.
|
* pi_state->list is already empty.
|
||||||
* clear pi_state->owner.
|
* clear pi_state->owner.
|
||||||
|
@ -907,13 +911,14 @@ void exit_pi_state_list(struct task_struct *curr)
|
||||||
raw_spin_unlock_irq(&curr->pi_lock);
|
raw_spin_unlock_irq(&curr->pi_lock);
|
||||||
|
|
||||||
spin_lock(&hb->lock);
|
spin_lock(&hb->lock);
|
||||||
|
raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
|
||||||
raw_spin_lock_irq(&curr->pi_lock);
|
raw_spin_lock(&curr->pi_lock);
|
||||||
/*
|
/*
|
||||||
* We dropped the pi-lock, so re-check whether this
|
* We dropped the pi-lock, so re-check whether this
|
||||||
* task still owns the PI-state:
|
* task still owns the PI-state:
|
||||||
*/
|
*/
|
||||||
if (head->next != next) {
|
if (head->next != next) {
|
||||||
|
raw_spin_unlock(&pi_state->pi_mutex.wait_lock);
|
||||||
spin_unlock(&hb->lock);
|
spin_unlock(&hb->lock);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
@ -922,9 +927,10 @@ void exit_pi_state_list(struct task_struct *curr)
|
||||||
WARN_ON(list_empty(&pi_state->list));
|
WARN_ON(list_empty(&pi_state->list));
|
||||||
list_del_init(&pi_state->list);
|
list_del_init(&pi_state->list);
|
||||||
pi_state->owner = NULL;
|
pi_state->owner = NULL;
|
||||||
raw_spin_unlock_irq(&curr->pi_lock);
|
raw_spin_unlock(&curr->pi_lock);
|
||||||
|
|
||||||
get_pi_state(pi_state);
|
get_pi_state(pi_state);
|
||||||
|
raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
|
||||||
spin_unlock(&hb->lock);
|
spin_unlock(&hb->lock);
|
||||||
|
|
||||||
rt_mutex_futex_unlock(&pi_state->pi_mutex);
|
rt_mutex_futex_unlock(&pi_state->pi_mutex);
|
||||||
|
@ -1208,6 +1214,10 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,
|
||||||
|
|
||||||
WARN_ON(!list_empty(&pi_state->list));
|
WARN_ON(!list_empty(&pi_state->list));
|
||||||
list_add(&pi_state->list, &p->pi_state_list);
|
list_add(&pi_state->list, &p->pi_state_list);
|
||||||
|
/*
|
||||||
|
* Assignment without holding pi_state->pi_mutex.wait_lock is safe
|
||||||
|
* because there is no concurrency as the object is not published yet.
|
||||||
|
*/
|
||||||
pi_state->owner = p;
|
pi_state->owner = p;
|
||||||
raw_spin_unlock_irq(&p->pi_lock);
|
raw_spin_unlock_irq(&p->pi_lock);
|
||||||
|
|
||||||
|
@ -2878,6 +2888,7 @@ retry:
|
||||||
raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
|
raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
|
||||||
spin_unlock(&hb->lock);
|
spin_unlock(&hb->lock);
|
||||||
|
|
||||||
|
/* drops pi_state->pi_mutex.wait_lock */
|
||||||
ret = wake_futex_pi(uaddr, uval, pi_state);
|
ret = wake_futex_pi(uaddr, uval, pi_state);
|
||||||
|
|
||||||
put_pi_state(pi_state);
|
put_pi_state(pi_state);
|
||||||
|
|
Loading…
Reference in New Issue