Re: [PATCH 06/10] futex: Provide and use pi_state_update_owner()

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 5391 bytes --]

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;
>  
>         /*

-- 
Ben Hutchings
All the simple programs have been written, and all the good names taken

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]