linux-kernel - Re: [PATCH v2] rcu: Don't return a value from rcu_assign

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:   Mon, 27 May 2019 09:10:50 -0700
From:   "Paul E. McKenney" <paulmck@...ux.ibm.com>
To:     Andrea Parri <andrea.parri@...rulasolutions.com>
Cc:     linux-kernel@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>, rcu@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Mark Rutland <mark.rutland@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Sasha Levin <sashal@...nel.org>, apw@...onical.com,
        joe@...ches.com
Subject: Re: [PATCH v2] rcu: Don't return a value from rcu_assign_pointer()

On Mon, May 27, 2019 at 10:49:57AM +0200, Andrea Parri wrote:
> Quoting Paul [1]:
> 
>   "Given that a quick (and perhaps error-prone) search of the uses
>    of rcu_assign_pointer() in v5.1 didn't find a single use of the
>    return value, let's please instead change the documentation and
>    implementation to eliminate the return value."
> 
> [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com

Queued, thank you!

Adding the checkpatch maintainers on CC as well.  The "do { } while
(0)" prevents the return value from being used, by design.  Given the
checkpatch complaint, is there some better way to achieve this?

							Thanx, Paul

> Signed-off-by: Andrea Parri <andrea.parri@...rulasolutions.com>
> Cc: "Paul E. McKenney" <paulmck@...ux.ibm.com>
> Cc: Josh Triplett <josh@...htriplett.org>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Cc: Lai Jiangshan <jiangshanlai@...il.com>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: rcu@...r.kernel.org
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Will Deacon <will.deacon@....com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Sasha Levin <sashal@...nel.org>
> ---
> Changes in v2:
>   - Fix documentation and style (Paul E. McKenney)
>   - Improve subject line (Mark Rutland) 
> ---
>  Documentation/RCU/whatisRCU.txt           | 8 ++++----
>  include/linux/rcupdate.h                  | 5 ++---
>  tools/include/linux/rcu.h                 | 4 ++--
>  tools/testing/radix-tree/linux/rcupdate.h | 2 +-
>  4 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> index 981651a8b65d2..7e1a8721637ab 100644
> --- a/Documentation/RCU/whatisRCU.txt
> +++ b/Documentation/RCU/whatisRCU.txt
> @@ -212,7 +212,7 @@ synchronize_rcu()
>  
>  rcu_assign_pointer()
>  
> -	typeof(p) rcu_assign_pointer(p, typeof(p) v);
> +	void rcu_assign_pointer(p, typeof(p) v);
>  
>  	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
>  	would be cool to be able to declare a function in this manner.
> @@ -220,9 +220,9 @@ rcu_assign_pointer()
>  
>  	The updater uses this function to assign a new value to an
>  	RCU-protected pointer, in order to safely communicate the change
> -	in value from the updater to the reader.  This function returns
> -	the new value, and also executes any memory-barrier instructions
> -	required for a given CPU architecture.
> +	in value from the updater to the reader.  This macro does not
> +	evaluate to an rvalue, but it does execute any memory-barrier
> +	instructions required for a given CPU architecture.
>  
>  	Perhaps just as important, it serves to document (1) which
>  	pointers are protected by RCU and (2) the point at which a
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index a8ed624da5556..0c9b92799abc7 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>   * other macros that it invokes.
>   */
>  #define rcu_assign_pointer(p, v)					      \
> -({									      \
> +do {									      \
>  	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
>  	rcu_check_sparse(p, __rcu);					      \
>  									      \
> @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>  		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
>  	else								      \
>  		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> -	_r_a_p__v;							      \
> -})
> +} while (0)
>  
>  /**
>   * rcu_swap_protected() - swap an RCU and a regular pointer
> diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> index 7d02527e5bcea..9554d3fa54f33 100644
> --- a/tools/include/linux/rcu.h
> +++ b/tools/include/linux/rcu.h
> @@ -19,7 +19,7 @@ static inline bool rcu_is_watching(void)
>  	return false;
>  }
>  
> -#define rcu_assign_pointer(p, v) ((p) = (v))
> -#define RCU_INIT_POINTER(p, v) p=(v)
> +#define rcu_assign_pointer(p, v)	do { (p) = (v); } while (0)
> +#define RCU_INIT_POINTER(p, v)	do { (p) = (v); } while (0)
>  
>  #endif
> diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> index fd280b070fdb1..fed468fb0c78d 100644
> --- a/tools/testing/radix-tree/linux/rcupdate.h
> +++ b/tools/testing/radix-tree/linux/rcupdate.h
> @@ -7,6 +7,6 @@
>  #define rcu_dereference_raw(p) rcu_dereference(p)
>  #define rcu_dereference_protected(p, cond) rcu_dereference(p)
>  #define rcu_dereference_check(p, cond) rcu_dereference(p)
> -#define RCU_INIT_POINTER(p, v)	(p) = (v)
> +#define RCU_INIT_POINTER(p, v)	do { (p) = (v); } while (0)
>  
>  #endif
> -- 
> 2.7.4
>