[PATCH 0/2] Prevent evaluation of WRITE_ONCE()

[PATCH 0/2] Prevent evaluation of WRITE_ONCE()

[Andrea Parri]

Hi all,

This all started when we realized that

  atomic64_t v;
  ...
  typeof(v.counter) my_val = atomic64_set(&v, VAL);

is a valid assignment on some architectures, but not on others [1]:
in particular, the assignment is valid on all architectures #define
-ing their atomic64_set() macro to WRITE_ONCE() (which is currently
evaluated).

Mark suggested to clean up all non-portable users of atomic*_set(),
and to prevent WRITE_ONCE() from being evaluated [2]; this resulted
in this first attempt/patchset based on Mark's atomics/type-cleanup
branch [3].

Cheers Andrea

[1] https://lkml.kernel.org/r/20190523083013.GA4616@andrea
[2] https://lkml.kernel.org/r/20190523141851.GA7523@lakrids.cambridge.arm.com
[3] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git atomics/type-cleanup

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jorgen Hansen <jhansen@vmware.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>

Andrea Parri (2):
  vmw_vmci: Clean up uses of atomic*_set()
  compiler: Prevent evaluation of WRITE_ONCE()

 include/linux/compiler.h      | 5 ++---
 include/linux/vmw_vmci_defs.h | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/2] vmw_vmci: Clean up uses of atomic*_set()

[Andrea Parri]

The primitive vmci_q_set_pointer() relies on atomic*_set() being of
type 'void', but this is a non-portable implementation detail.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jorgen Hansen <jhansen@vmware.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
---
 include/linux/vmw_vmci_defs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
index 0c06178e4985b..eb593868e2e9e 100644
--- a/include/linux/vmw_vmci_defs.h
+++ b/include/linux/vmw_vmci_defs.h
@@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
 				      u64 new_val)
 {
 #if defined(CONFIG_X86_32)
-	return atomic_set((atomic_t *)var, (u32)new_val);
+	atomic_set((atomic_t *)var, (u32)new_val);
 #else
-	return atomic64_set(var, new_val);
+	atomic64_set(var, new_val);
 #endif
 }
 
-- 
2.7.4

Re: [PATCH 1/2] vmw_vmci: Clean up uses of atomic*_set()

[Peter Zijlstra]

On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> type 'void', but this is a non-portable implementation detail.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> ---
>  include/linux/vmw_vmci_defs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> index 0c06178e4985b..eb593868e2e9e 100644
> --- a/include/linux/vmw_vmci_defs.h
> +++ b/include/linux/vmw_vmci_defs.h
> @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
>  				      u64 new_val)
>  {
>  #if defined(CONFIG_X86_32)
> -	return atomic_set((atomic_t *)var, (u32)new_val);
> +	atomic_set((atomic_t *)var, (u32)new_val);
>  #else
> -	return atomic64_set(var, new_val);
> +	atomic64_set(var, new_val);
>  #endif
>  }

All that should just die a horrible death. That code is crap.

See:

  lkml.kernel.org/r/20190524103731.GN2606@hirez.programming.kicks-ass.net

Re: [PATCH 1/2] vmw_vmci: Clean up uses of atomic*_set()

[Greg Kroah-Hartman]

On Fri, May 24, 2019 at 12:39:34PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> > The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> > type 'void', but this is a non-portable implementation detail.
> > 
> > Reported-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jorgen Hansen <jhansen@vmware.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > ---
> >  include/linux/vmw_vmci_defs.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> > index 0c06178e4985b..eb593868e2e9e 100644
> > --- a/include/linux/vmw_vmci_defs.h
> > +++ b/include/linux/vmw_vmci_defs.h
> > @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
> >  				      u64 new_val)
> >  {
> >  #if defined(CONFIG_X86_32)
> > -	return atomic_set((atomic_t *)var, (u32)new_val);
> > +	atomic_set((atomic_t *)var, (u32)new_val);
> >  #else
> > -	return atomic64_set(var, new_val);
> > +	atomic64_set(var, new_val);
> >  #endif
> >  }
> 
> All that should just die a horrible death. That code is crap.
> 
> See:
> 
>   lkml.kernel.org/r/20190524103731.GN2606@hirez.programming.kicks-ass.net

I agree, Peter's patch should be the thing that is applied, not this
one.

thanks,

greg k-h

Re: [PATCH 1/2] vmw_vmci: Clean up uses of atomic*_set()

[Andrea Parri]

> > All that should just die a horrible death. That code is crap.
> > 
> > See:
> > 
> >   lkml.kernel.org/r/20190524103731.GN2606@hirez.programming.kicks-ass.net
> 
> I agree, Peter's patch should be the thing that is applied, not this
> one.

Thanks for the confirmation, Greg.  I'll drop mine.

At this time, it's not clear to me where Peter's patch will be applied,
and where/whether I should rebase 2/2 (if there is interested in that):

Please let me know.

Thanks,
  Andrea

Re: [PATCH 1/2] vmw_vmci: Clean up uses of atomic*_set()

[Andrea Parri]

On Fri, May 24, 2019 at 12:39:34PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> > The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> > type 'void', but this is a non-portable implementation detail.
> > 
> > Reported-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jorgen Hansen <jhansen@vmware.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > ---
> >  include/linux/vmw_vmci_defs.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> > index 0c06178e4985b..eb593868e2e9e 100644
> > --- a/include/linux/vmw_vmci_defs.h
> > +++ b/include/linux/vmw_vmci_defs.h
> > @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
> >  				      u64 new_val)
> >  {
> >  #if defined(CONFIG_X86_32)
> > -	return atomic_set((atomic_t *)var, (u32)new_val);
> > +	atomic_set((atomic_t *)var, (u32)new_val);
> >  #else
> > -	return atomic64_set(var, new_val);
> > +	atomic64_set(var, new_val);
> >  #endif
> >  }
> 
> All that should just die a horrible death. That code is crap.
> 
> See:
> 
>   lkml.kernel.org/r/20190524103731.GN2606@hirez.programming.kicks-ass.net

I see, that was indeed 'racy' with my patch!  ;-)  Thank you!

  Andrea

[PATCH 2/2] compiler: Prevent evaluation of WRITE_ONCE()

[Andrea Parri]

Now that there's no single use of the value of WRITE_ONCE(), change
the implementation to eliminate it.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jorgen Hansen <jhansen@vmware.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
---
 include/linux/compiler.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b06..4024c809a6c63 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -277,12 +277,11 @@ unsigned long read_word_at_a_time(const void *addr)
 }
 
 #define WRITE_ONCE(x, val) \
-({							\
+do {							\
 	union { typeof(x) __val; char __c[1]; } __u =	\
 		{ .__val = (__force typeof(x)) (val) }; \
 	__write_once_size(&(x), __u.__c, sizeof(x));	\
-	__u.__val;					\
-})
+} while (0)
 
 #endif /* __KERNEL__ */
 
-- 
2.7.4

Re: [PATCH 2/2] compiler: Prevent evaluation of WRITE_ONCE()

[Mark Rutland]

This would be better titled as:

  compiler: don't return a value from WRITE_ONCE()

... since we do want the WRITE_ONCE() itself to be evaluated.

On Fri, May 24, 2019 at 12:35:36PM +0200, Andrea Parri wrote:
> Now that there's no single use of the value of WRITE_ONCE(), change
> the implementation to eliminate it.

I hope that's the case, but it's possible that some macros might be
relying on this, so it's probably worth waiting to see if the kbuild
test robot screams.

Otherwise, I agree that WRITE_ONCE() returning a value is surprising,
and unnecessary. IIRC you said that trying to suport that in other
implementations was painful, so aligning on a non-returning version
sounds reasonable to me.

> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> ---
>  include/linux/compiler.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 8aaf7cd026b06..4024c809a6c63 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -277,12 +277,11 @@ unsigned long read_word_at_a_time(const void *addr)
>  }
>  
>  #define WRITE_ONCE(x, val) \
> -({							\
> +do {							\
>  	union { typeof(x) __val; char __c[1]; } __u =	\
>  		{ .__val = (__force typeof(x)) (val) }; \
>  	__write_once_size(&(x), __u.__c, sizeof(x));	\
> -	__u.__val;					\
> -})
> +} while (0)

With the title fixed, and assuming that the kbuild test robot doesn't
find uses we've missed:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.


>  
>  #endif /* __KERNEL__ */
>  
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] compiler: Prevent evaluation of WRITE_ONCE()

[Andrea Parri]

On Fri, May 24, 2019 at 11:53:40AM +0100, Mark Rutland wrote:
> 
> This would be better titled as:
> 
>   compiler: don't return a value from WRITE_ONCE()

No strong opinion here: I'll adopt your suggestion in v2 if there are
no objections.  And similarly for the rcu_assign_pointer() patch.


> 
> ... since we do want the WRITE_ONCE() itself to be evaluated.
> 
> On Fri, May 24, 2019 at 12:35:36PM +0200, Andrea Parri wrote:
> > Now that there's no single use of the value of WRITE_ONCE(), change
> > the implementation to eliminate it.
> 
> I hope that's the case, but it's possible that some macros might be
> relying on this, so it's probably worth waiting to see if the kbuild
> test robot screams.

Absolutely!  Does kbuild process your tree?  I might be worth to apply
the patch to just see what kbuild 'think' about it...


> 
> Otherwise, I agree that WRITE_ONCE() returning a value is surprising,
> and unnecessary. IIRC you said that trying to suport that in other
> implementations was painful, so aligning on a non-returning version
> sounds reasonable to me.

And I should probably also modify the few #define-s under tools/ (that
I missed in this iteration...)

Thanks,
  Andrea