stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ben Hutchings <ben@decadent.org.uk>
To: Lee Jones <lee.jones@linaro.org>, stable@vger.kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH 06/10] futex: Provide and use pi_state_update_owner()
Date: Sun, 07 Feb 2021 23:16:34 +0100	[thread overview]
Message-ID: <ab46f8fa3e6394f720e3caf501ab7822ffc9cccb.camel@decadent.org.uk> (raw)
In-Reply-To: <20210204172903.2860981-7-lee.jones@linaro.org>

[-- 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 --]

  reply	other threads:[~2021-02-07 22:56 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-04 17:28 [PATCH 4.4 00/10] [Set 2] Futex back-port Lee Jones
2021-02-04 17:28 ` [PATCH 01/10] futex,rt_mutex: Provide futex specific rt_mutex API Lee Jones
2021-02-04 17:28 ` [PATCH 02/10] futex: Remove rt_mutex_deadlock_account_*() Lee Jones
2021-02-04 17:28 ` [PATCH 03/10] futex: Rework inconsistent rt_mutex/futex_q state Lee Jones
2021-02-04 17:28 ` [PATCH 04/10] futex: Avoid violating the 10th rule of futex Lee Jones
2021-02-04 17:28 ` [PATCH 05/10] futex: Replace pointless printk in fixup_owner() Lee Jones
2021-02-04 17:28 ` [PATCH 06/10] futex: Provide and use pi_state_update_owner() Lee Jones
2021-02-07 22:16   ` Ben Hutchings [this message]
2021-02-04 17:29 ` [PATCH 07/10] rtmutex: Remove unused argument from rt_mutex_proxy_unlock() Lee Jones
2021-02-04 17:29 ` [PATCH 08/10] futex: Use pi_state_update_owner() in put_pi_state() Lee Jones
2021-02-04 17:29 ` [PATCH 09/10] futex: Simplify fixup_pi_state_owner() Lee Jones
2021-02-04 17:29 ` [PATCH 10/10] futex: Handle faults correctly for PI futexes Lee Jones
2021-02-05  8:55 ` [PATCH 4.4 00/10] [Set 2] Futex back-port Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2021-02-03 13:45 [PATCH 4.9 " Lee Jones
2021-02-03 13:45 ` [PATCH 06/10] futex: Provide and use pi_state_update_owner() Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ab46f8fa3e6394f720e3caf501ab7822ffc9cccb.camel@decadent.org.uk \
    --to=ben@decadent.org.uk \
    --cc=lee.jones@linaro.org \
    --cc=peterz@infradead.org \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).