linux-kernel - Re: [PATCH] mutex: Fix mutex_can_spin_on

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:	Fri, 19 Jul 2013 15:08:36 -0400
From:	Waiman Long <waiman.long@...com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Davidlohr Bueso <davidlohr.bueso@...com>,
	Rik van Riel <riel@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	David Howells <dhowells@...hat.com>,
	Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
> mutex_can_spin_on_owner() is broken in that it would allow the compiler
> to load lock->owner twice, seeing a pointer first time and a MULL
> pointer the second time.
>
> Signed-off-by: Peter Zijlstra<peterz@...radead.org>
> ---
>   kernel/mutex.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/mutex.c b/kernel/mutex.c
> index ff05f4b..7ff48c5 100644
> --- a/kernel/mutex.c
> +++ b/kernel/mutex.c
> @@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
>    */
>   static inline int mutex_can_spin_on_owner(struct mutex *lock)
>   {
> +	struct task_struct *owner;
>   	int retval = 1;
>
>   	rcu_read_lock();
> -	if (lock->owner)
> -		retval = lock->owner->on_cpu;
> +	owner = ACCESS_ONCE(lock->owner);
> +	if (owner)
> +		retval = owner->on_cpu;
>   	rcu_read_unlock();
>   	/*
>   	 * if lock->owner is not set, the mutex owner may have just acquired

I am fine with this change. However, the compiler is smart enough to not 
do two memory accesses to the same memory location. So this will not 
change the generated code. Below is the relevant x86 code for that 
section of code:

    0x00000000000005d2 <+34>:    mov    0x18(%rdi),%rdx
    0x00000000000005d6 <+38>:    mov    $0x1,%eax
    0x00000000000005db <+43>:    test   %rdx,%rdx
    0x00000000000005de <+46>:    je     0x5e3 <__mutex_lock_slowpath+51>
    0x00000000000005e0 <+48>:    mov    0x28(%rdx),%eax
    0x00000000000005e3 <+51>:    test   %eax,%eax
    0x00000000000005e5 <+53>:    je     0x6d3 <__mutex_lock_slowpath+291>

Only one memory access is done.

Ack-by: Waiman Long <Waiman.Long@...com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/