| Message ID | 20210204172903.2860981-7-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Futex back-port | expand |
On Thu, 2021-02-04 at 17:28 +0000, Lee Jones wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > [ Upstream commit c5cade200ab9a2a3be9e7f32a752c8d86b502ec7 ] > > Updating pi_state::owner is done at several places with the same > code. Provide a function for it and use that at the obvious places. > > This is also a preparation for a bug fix to avoid yet another copy of > the > same code or alternatively introducing a completely unpenetratable > mess of > gotos. > > Originally-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: stable@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > kernel/futex.c | 64 ++++++++++++++++++++++++++---------------------- > -- > 1 file changed, 33 insertions(+), 31 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index a247942d69799..1390ffa874a6b 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -835,6 +835,29 @@ static struct futex_pi_state * > alloc_pi_state(void) > return pi_state; > } > > +static void pi_state_update_owner(struct futex_pi_state *pi_state, > + struct task_struct *new_owner) > +{ > + struct task_struct *old_owner = pi_state->owner; > + > + lockdep_assert_held(&pi_state->pi_mutex.wait_lock); But not all callers do hold the wait_lock. That may not be needed as we don't have commit 734009e96d19 "futex: Change locking rules". > + if (old_owner) { > + raw_spin_lock(&old_owner->pi_lock); (Some of) the callers used to disable interrupts when taking pi_lock, and I think that behaviour needs to be preserved here. I'm doubtful that this cherry-picking approach is going to work. Ben. > + WARN_ON(list_empty(&pi_state->list)); > + list_del_init(&pi_state->list); > + raw_spin_unlock(&old_owner->pi_lock); > + } > + > + if (new_owner) { > + raw_spin_lock(&new_owner->pi_lock); > + WARN_ON(!list_empty(&pi_state->list)); > + list_add(&pi_state->list, &new_owner->pi_state_list); > + pi_state->owner = new_owner; > + raw_spin_unlock(&new_owner->pi_lock); > + } > +} > + > /* > * Must be called with the hb lock held. > */ > @@ -1427,26 +1450,16 @@ static int wake_futex_pi(u32 __user *uaddr, > u32 uval, struct futex_q *this, > else > ret = -EINVAL; > } > - if (ret) { > - raw_spin_unlock(&pi_state->pi_mutex.wait_lock); > - return ret; > - } > - > - raw_spin_lock_irq(&pi_state->owner->pi_lock); > - WARN_ON(list_empty(&pi_state->list)); > - list_del_init(&pi_state->list); > - raw_spin_unlock_irq(&pi_state->owner->pi_lock); > > - raw_spin_lock_irq(&new_owner->pi_lock); > - WARN_ON(!list_empty(&pi_state->list)); > - list_add(&pi_state->list, &new_owner->pi_state_list); > - pi_state->owner = new_owner; > - raw_spin_unlock_irq(&new_owner->pi_lock); > - > - /* > - * We've updated the uservalue, this unlock cannot fail. > - */ > - deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, > &wake_q); > + if (!ret) { > + /* > + * This is a point of no return; once we modified the > uval > + * there is no going back and subsequent operations > must > + * not fail. > + */ > + pi_state_update_owner(pi_state, new_owner); > + deboost = __rt_mutex_futex_unlock(&pi_state- > >pi_mutex, &wake_q); > + } > > raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock); > spin_unlock(&hb->lock); > @@ -2318,19 +2331,8 @@ retry: > * We fixed up user space. Now we need to fix the pi_state > * itself. > */ > - if (pi_state->owner != NULL) { > - raw_spin_lock_irq(&pi_state->owner->pi_lock); > - WARN_ON(list_empty(&pi_state->list)); > - list_del_init(&pi_state->list); > - raw_spin_unlock_irq(&pi_state->owner->pi_lock); > - } > - > - pi_state->owner = newowner; > + pi_state_update_owner(pi_state, newowner); > > - raw_spin_lock_irq(&newowner->pi_lock); > - WARN_ON(!list_empty(&pi_state->list)); > - list_add(&pi_state->list, &newowner->pi_state_list); > - raw_spin_unlock_irq(&newowner->pi_lock); > return 0; > > /*
diff --git a/kernel/futex.c b/kernel/futex.c index a247942d69799..1390ffa874a6b 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -835,6 +835,29 @@ static struct futex_pi_state * alloc_pi_state(void) return pi_state; } +static void pi_state_update_owner(struct futex_pi_state *pi_state, + struct task_struct *new_owner) +{ + struct task_struct *old_owner = pi_state->owner; + + lockdep_assert_held(&pi_state->pi_mutex.wait_lock); + + if (old_owner) { + raw_spin_lock(&old_owner->pi_lock); + WARN_ON(list_empty(&pi_state->list)); + list_del_init(&pi_state->list); + raw_spin_unlock(&old_owner->pi_lock); + } + + if (new_owner) { + raw_spin_lock(&new_owner->pi_lock); + WARN_ON(!list_empty(&pi_state->list)); + list_add(&pi_state->list, &new_owner->pi_state_list); + pi_state->owner = new_owner; + raw_spin_unlock(&new_owner->pi_lock); + } +} + /* * Must be called with the hb lock held. */ @@ -1427,26 +1450,16 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this, else ret = -EINVAL; } - if (ret) { - raw_spin_unlock(&pi_state->pi_mutex.wait_lock); - return ret; - } - - raw_spin_lock_irq(&pi_state->owner->pi_lock); - WARN_ON(list_empty(&pi_state->list)); - list_del_init(&pi_state->list); - raw_spin_unlock_irq(&pi_state->owner->pi_lock); - raw_spin_lock_irq(&new_owner->pi_lock); - WARN_ON(!list_empty(&pi_state->list)); - list_add(&pi_state->list, &new_owner->pi_state_list); - pi_state->owner = new_owner; - raw_spin_unlock_irq(&new_owner->pi_lock); - - /* - * We've updated the uservalue, this unlock cannot fail. - */ - deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q); + if (!ret) { + /* + * This is a point of no return; once we modified the uval + * there is no going back and subsequent operations must + * not fail. + */ + pi_state_update_owner(pi_state, new_owner); + deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q); + } raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock); spin_unlock(&hb->lock); @@ -2318,19 +2331,8 @@ retry: * We fixed up user space. Now we need to fix the pi_state * itself. */ - if (pi_state->owner != NULL) { - raw_spin_lock_irq(&pi_state->owner->pi_lock); - WARN_ON(list_empty(&pi_state->list)); - list_del_init(&pi_state->list); - raw_spin_unlock_irq(&pi_state->owner->pi_lock); - } - - pi_state->owner = newowner; + pi_state_update_owner(pi_state, newowner); - raw_spin_lock_irq(&newowner->pi_lock); - WARN_ON(!list_empty(&pi_state->list)); - list_add(&pi_state->list, &newowner->pi_state_list); - raw_spin_unlock_irq(&newowner->pi_lock); return 0; /*