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 --]
next prev parent 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).