linux-kernel - Re: [PATCH] rtmutex: Add acquire semantics for rtmutex lock acquisition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Dec 2022 12:43:51 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Mel Gorman <mgorman@...hsingularity.net>
Cc:     Peter Zijlstra <peterz@...radead.org>, Jan Kara <jack@...e.cz>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Pierre Gondois <pierre.gondois@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Davidlohr Bueso <dave@...olabs.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux-RT <linux-rt-users@...r.kernel.org>
Subject: Re: [PATCH] rtmutex: Add acquire semantics for rtmutex lock
 acquisition

On 2022-12-02 15:01:58 [+0000], Mel Gorman wrote:
> > Fixes: 700318d1d7b38 ("locking/rtmutex: Use acquire/release semantics")
> > 
> 
> Adding Davidlohr to cc.
> 
> It might have made the problem worse but even then rt_mutex_set_owner was
> just a plain assignment and while I didn't check carefully, at a glance
> try_to_take_rt_mutex didn't look like it guaranteed ACQUIRE semantics.

It looks like it had a strong cmpxchg which was relaxed. But I might be
wrong ;) Either way we need stable tag so that this gets back ported.
The commit in question is since v4.4 and stable trees are maintained
down to 4.9 (but only until JAN so we should hurry ;)).

> > Before that, it did cmpxchg() which should be fine.
> > 
> > Regarding mark_rt_mutex_waiters(). Isn't acquire semantic required in
> > order for the lock-owner not perform the fastpath but go to the slowpath
> > instead?
> > 
> 
> Good spot, it does. While the most straight-forward solution is to use
> cmpxchg_acquire, I think it is overkill because it could incur back-to-back
> ACQUIRE operations in the event of contention. There could be a smp_wmb
> after the cmpxchg_relaxed but that impacts all arches and a non-paired
> smp_wmb is generally frowned upon.

but in general, it should succeed on the first iteration. It can only
fail (and retry) if the owner was able to unlock it first. A second
locker will spin on the wait_lock so.

> I'm thinking this on top of the patch should be sufficient even though
> it's a heavier operation than is necesary for ACQUIRE as well as being
> "not typical" according to Documentation/atomic_t.txt. Will, as this
> affects ARM primarily do you have any preference?
> 
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 35212f260148..af0dbe4d5e97 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -238,6 +238,13 @@ static __always_inline void mark_rt_mutex_waiters(struct rt_mutex_base *lock)
>  		owner = *p;
>  	} while (cmpxchg_relaxed(p, owner,
>  				 owner | RT_MUTEX_HAS_WAITERS) != owner);
> +
> +	/*
> +	 * The cmpxchg loop above is relaxed to avoid back-to-back ACQUIRE
> +	 * operations in the event of contention. Ensure the successful
> +	 * cmpxchg is visible.
> +	 */
> +	smp_mb__after_atomic();
>  }

Sebastian