linux-kernel - Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg

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]

Message-ID: <20160315170835.GA5058@lerouge>
Date:	Tue, 15 Mar 2016 18:08:37 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to
 'xchg_or()'


Thanks a lot for fixing this!
Some comments below:

On Tue, Mar 15, 2016 at 01:21:45PM +0100, Ingo Molnar wrote:
> Linus noticed a couple of problems with the fetch_or() implementation introduced
> by 5fd7a09cfb8c ("atomic: Export fetch_or()"):
> 
>  - Sloppy macro implementation: 'mask' and 'ptr' is evaluated multiple times,
>    which will break if arguments have side effects. Also, it uses 
>    double-underscore temporary variables - which is dangerous as low level asm 
>    ones are using those too and they may alias in unexpected ways.
> 
>  - Sloppy semantics: the naming and structure of the macro is ambigious, with
>    no well-defined semantics. It's neither an atomic nor a cmpxchg() interface,
>    but still it lives in include/linux/atomic.h.
> 
> Solve this by:
> 
>  - Extracting the arguments into helper variables once, and never
>    using the original arguments from that point on. This makes it
>    pretty clear that there can be no unwanted macro evaluation
>    side effects.
> 
>  - Using single-underscore temporary variables.
> 
>  - Renaming fetch_or() to xchg_or(), recognizing that the semantics
>    are xchg()-alike.

I wouldn't mind that much having xchg_or() but I think Peterz has good arguments in favour
of keeping the original name.

> 
> Also propagate the rename to the call sites.
> 
> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Chris Metcalf <cmetcalf@...hip.com>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Luiz Capitulino <lcapitulino@...hat.com>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Rik van Riel <riel@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> Link: http://lkml.kernel.org/r/20160315093245.GA7943@gmail.com
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> ---
>  include/linux/atomic.h   | 26 ++++++++++++++++----------
>  kernel/sched/core.c      |  2 +-
>  kernel/time/tick-sched.c |  4 ++--
>  3 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index 6c502cb13c95..7bc5297bcca8 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -549,22 +549,28 @@ static inline int atomic_dec_if_positive(atomic_t *v)
>  #endif
>  
>  /**
> - * fetch_or - perform *ptr |= mask and return old value of *ptr
> - * @ptr: pointer to value
> + * xchg_or - perform *ptr |= mask atomically and return old value of *ptr
> + * @ptr: pointer to value (cmpxchg() compatible integer pointer type)
>   * @mask: mask to OR on the value
>   *
> - * cmpxchg based fetch_or, macro so it works for different integer types
> + * cmpxchg() based, it's a macro so it works for different integer types.
>   */
> -#ifndef fetch_or
> -#define fetch_or(ptr, mask)						\
> -({	typeof(*(ptr)) __old, __val = *(ptr);				\
> +#ifndef xchg_or
> +# define xchg_or(ptr, mask)						\
> +({									\
> +	typeof(ptr)  _ptr  = (ptr);					\

Can we add a comment above to prevent from future mistakes with cmpxchg
variables aliasing?

I'm suprised that GCC doesn't warn about that actually.