On Thu, 2021-02-04 at 17:28 +0000, Lee Jones wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>
> [ 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@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> ---
> 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;
>
> /*
--
Ben Hutchings
All the simple programs have been written, and all the good names taken
Attachment:
signature.asc
Description: This is a digitally signed message part
