[PATCH] fix __percpu annotation in asm-generic

[PATCH] fix __percpu annotation in asm-generic

[Luc Van Oostenryck]

The generic implementation of raw_cpu_generic_add_return() is:

        #define raw_cpu_generic_add_return(pcp, val)            \
        ({                                                      \
                typeof(&(pcp)) __p = raw_cpu_ptr(&(pcp));       \
                                                                \
                *__p += val;                                    \
                *__p;                                           \
        })

where the 'pcp' argument is a __percpu lvalue.
There, the variable '__p' is declared as a __percpu pointer
the type of the address of 'pcp') but:
1) the value assigned to it, the return value of raw_cpu_ptr(), is
   a plain (__kernel) pointer, not a __percpu one.
2) the variable is dereferenced just after while a __percpu pointer
   is implicitly __noderef.

So, fix the declaration of the 'pcp' variable to its correct type:
the plain (non-percpu) pointer corresponding to its address.
Same for raw_cpu_generic_xchg(), raw_cpu_generic_cmpxchg() &
raw_cpu_generic_cmpxchg_double().

This remove 209 warnings on ARM, 460 on x86 & 2600+ on ppc64.

Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Reported-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 include/asm-generic/percpu.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/asm-generic/percpu.h b/include/asm-generic/percpu.h
index c2de013b2cf4..4ae5f89a0e61 100644
--- a/include/asm-generic/percpu.h
+++ b/include/asm-generic/percpu.h
@@ -74,7 +74,7 @@ do {									\
 
 #define raw_cpu_generic_add_return(pcp, val)				\
 ({									\
-	typeof(&(pcp)) __p = raw_cpu_ptr(&(pcp));			\
+	typeof(pcp) __kernel __force *__p = raw_cpu_ptr(&(pcp));	\
 									\
 	*__p += val;							\
 	*__p;								\
@@ -82,7 +82,7 @@ do {									\
 
 #define raw_cpu_generic_xchg(pcp, nval)					\
 ({									\
-	typeof(&(pcp)) __p = raw_cpu_ptr(&(pcp));			\
+	typeof(pcp) __kernel __force *__p = raw_cpu_ptr(&(pcp));	\
 	typeof(pcp) __ret;						\
 	__ret = *__p;							\
 	*__p = nval;							\
@@ -91,7 +91,7 @@ do {									\
 
 #define raw_cpu_generic_cmpxchg(pcp, oval, nval)			\
 ({									\
-	typeof(&(pcp)) __p = raw_cpu_ptr(&(pcp));			\
+	typeof(pcp) __kernel __force *__p = raw_cpu_ptr(&(pcp));	\
 	typeof(pcp) __ret;						\
 	__ret = *__p;							\
 	if (__ret == (oval))						\
@@ -101,8 +101,8 @@ do {									\
 
 #define raw_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2) \
 ({									\
-	typeof(&(pcp1)) __p1 = raw_cpu_ptr(&(pcp1));			\
-	typeof(&(pcp2)) __p2 = raw_cpu_ptr(&(pcp2));			\
+	typeof(pcp1) __kernel __force *__p1 = raw_cpu_ptr(&(pcp1));	\
+	typeof(pcp2) __kernel __force *__p2 = raw_cpu_ptr(&(pcp2));	\
 	int __ret = 0;							\
 	if (*__p1 == (oval1) && *__p2  == (oval2)) {			\
 		*__p1 = nval1;						\
-- 
2.24.0

Re: [PATCH] fix __percpu annotation in asm-generic

[Christopher Lameter]

On Tue, 26 Nov 2019, Luc Van Oostenryck wrote:

> So, fix the declaration of the 'pcp' variable to its correct type:
> the plain (non-percpu) pointer corresponding to its address.
> Same for raw_cpu_generic_xchg(), raw_cpu_generic_cmpxchg() &
> raw_cpu_generic_cmpxchg_double().

Acked-by: Christoph Lameter <cl@linux.com>

Maybe a better fix is to come up with a typeof_strip_percu() or so
macro for all the places where this needs to be done?

Re: [PATCH] fix __percpu annotation in asm-generic

[Dennis Zhou]

On Wed, Nov 27, 2019 at 03:55:19PM +0000, Christopher Lameter wrote:
> On Tue, 26 Nov 2019, Luc Van Oostenryck wrote:
> 
> > So, fix the declaration of the 'pcp' variable to its correct type:
> > the plain (non-percpu) pointer corresponding to its address.
> > Same for raw_cpu_generic_xchg(), raw_cpu_generic_cmpxchg() &
> > raw_cpu_generic_cmpxchg_double().
> 
> Acked-by: Christoph Lameter <cl@linux.com>
> 
> Maybe a better fix is to come up with a typeof_strip_percu() or so
> macro for all the places where this needs to be done?

I like the idea of typeof_strip_percpu(). Luc do you mind spinning v2
with a macro for this instead?

Thanks,
Dennis

Re: [PATCH] fix __percpu annotation in asm-generic

[Luc Van Oostenryck]

On Wed, Nov 27, 2019 at 12:53:50PM -0500, Dennis Zhou wrote:
> On Wed, Nov 27, 2019 at 03:55:19PM +0000, Christopher Lameter wrote:
> > On Tue, 26 Nov 2019, Luc Van Oostenryck wrote:
> > 
> > > So, fix the declaration of the 'pcp' variable to its correct type:
> > > the plain (non-percpu) pointer corresponding to its address.
> > > Same for raw_cpu_generic_xchg(), raw_cpu_generic_cmpxchg() &
> > > raw_cpu_generic_cmpxchg_double().
> > 
> > Acked-by: Christoph Lameter <cl@linux.com>
> > 
> > Maybe a better fix is to come up with a typeof_strip_percu() or so
> > macro for all the places where this needs to be done?
> 
> I like the idea of typeof_strip_percpu(). Luc do you mind spinning v2
> with a macro for this instead?

I wouldn't mind at all (I already thought about doing something
like this several times) but:
1) it would strip any address space, not just __percpu, so:
   it would need to be combined with __verify_pcpu_ptr() or,
   * a better name should be used,
   * it should be defined in a generic header, any idea where?
   * I fear it would be abused to escape sloppy typing
     (like __force and casts are already often used).
2) while I find the current solution:
	typeof(T) __kernel __force *ptr = ...;

   quite readable and relatively easy to understand, the solution
   I have for doing it with a macro (that behaves like typeof) is,
   IMO, much much less readable and understandable:
	#define typeof_strip_percpu(T) \
		typeof(({ typeof(T) __kernel __force __fakename; __fakename; }))

	typeof_strip_perpcu(T) * ptr = ...;

So, if you insist I can do it but I would really prefer not.


Best regards,
-- Luc

Re: [PATCH] fix __percpu annotation in asm-generic

[Christopher Lameter]

On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:

> 1) it would strip any address space, not just __percpu, so:
>    it would need to be combined with __verify_pcpu_ptr() or,
>    * a better name should be used,

typeof_cast_kernel() to express the fact that it creates a kernel pointer
and ignored the attributes??

>    * it should be defined in a generic header, any idea where?

include/linux/compiler-types.h

> 2) while I find the current solution:
> 	typeof(T) __kernel __force *ptr = ...;

It would be

   typeof_cast_kernel(&T) *xx = xxx

or so?

Re: [PATCH] fix __percpu annotation in asm-generic

[Luc Van Oostenryck]

On Fri, Nov 29, 2019 at 06:11:59PM +0000, Christopher Lameter wrote:
> On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:
> 
> > 1) it would strip any address space, not just __percpu, so:
> >    it would need to be combined with __verify_pcpu_ptr() or,
> >    * a better name should be used,
> 
> typeof_cast_kernel() to express the fact that it creates a kernel pointer
> and ignored the attributes??

typeof_strip_address_space() would, I think, express this better. 
It's not obvious at all to me that 'kernel' in 'typeof_cast_kernel()'
relates to the (default) kernel address space.
Maybe it's just me. I don't know.

> >    * it should be defined in a generic header, any idea where?
> 
> include/linux/compiler-types.h

Yes, OK.

> > 2) while I find the current solution:
> > 	typeof(T) __kernel __force *ptr = ...;
> 
> It would be
> 
>    typeof_cast_kernel(&T) *xx = xxx
> 
> or so?

No, it would not. __percpu, and more generally, the address space
is a property of the object, not of its address.
For example, let's say T is a __percpu object:
	int __percpu obj;
then '&T' is just a 'normal'/__kernel pointer to it:
	int __percpu *;
There is nothing to strip (it would be if the __percpu
would be 'on the other side of the *': int * __percpu).
It's exactly the same as with 'const': a 'const char *'
is not const, only a pointer to const.

The situation with raw_cpu_generic_add_return() is:
- pcp is a lvalue of of a __percpu object of type T, so:
	typeof(pcp)  := T __percpu
- pcp's address is given to raw_cpu_ptr(), so
	typeof(&pcp) := T __percpu *
- raw_cpu_ptr() return the corresponding __kernel pointer
  (adjusted for the current percu offset), so:
	typeof(raw_cpu_ptr(&pcp)) := T *
- so, the macro needs to declare a variable __p of type T*
  hence:
	typeof(pcp) __kernel __force *__p;
  or, with this new macro:
	typeof_cast_kernel(pcp) *__p;

Maybe a better solution would be to directly play at pointer
level and thus have something like this:
	typeof_<some good name>(&pcp) __p = raw_cpu_ptr(&pcp);
or even:
	__kernel_pointer(&pcp) __p = raw_cpu_ptr(&pcp);
I dunno.

Note: at implementation level, it complicates things slightly
      to want this 'strip_percpu' macro to behaves like typeof()
      because it means that it can take in argument either an
      expression or a type. And if it's a type, you can't do a
      simple cast on it, you need to declare an intermediate
      variable, hence the horrible:
	  typeof(({ typeof(T) __kernel __force __fakename; __fakename; }))

Note: it would be much much nicer to do all these type generic
      macros with '__auto_type' (only supported in GCC 4.9 IIUC
      and supported in sparse but it shouldn't be very hard to do)..


-- Luc

Re: [PATCH] fix __percpu annotation in asm-generic

[Dennis Zhou]

On Sat, Nov 30, 2019 at 01:00:37AM +0100, Luc Van Oostenryck wrote:
> On Fri, Nov 29, 2019 at 06:11:59PM +0000, Christopher Lameter wrote:
> > On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:
> > 
> > > 1) it would strip any address space, not just __percpu, so:
> > >    it would need to be combined with __verify_pcpu_ptr() or,
> > >    * a better name should be used,
> > 
> > typeof_cast_kernel() to express the fact that it creates a kernel pointer
> > and ignored the attributes??
> 
> typeof_strip_address_space() would, I think, express this better. 
> It's not obvious at all to me that 'kernel' in 'typeof_cast_kernel()'
> relates to the (default) kernel address space.
> Maybe it's just me. I don't know.
> 

I think typeof_cast_kernel() or typeof_force_kernel() are reasonable
names. I kind of like the idea of cast/force over strip because we're
really still moving address spaces even if it is moving it back.

> > >    * it should be defined in a generic header, any idea where?
> > 
> > include/linux/compiler-types.h
> 
> Yes, OK.
> 
> > > 2) while I find the current solution:
> > > 	typeof(T) __kernel __force *ptr = ...;
> > 
> > It would be
> > 
> >    typeof_cast_kernel(&T) *xx = xxx
> > 
> > or so?
> 
> No, it would not. __percpu, and more generally, the address space
> is a property of the object, not of its address.

Maybe for other address spaces that's true, but for percpu, __percpu is
a property of the address. An object can be referenced both from a
percpu address (via accessors) and the kernel address which is the
actual object.

> For example, let's say T is a __percpu object:
> 	int __percpu obj;

This can't exist. __percpu denotes address space not object.

> then '&T' is just a 'normal'/__kernel pointer to it:
> 	int __percpu *;
> There is nothing to strip (it would be if the __percpu
> would be 'on the other side of the *': int * __percpu).
> It's exactly the same as with 'const': a 'const char *'
> is not const, only a pointer to const.
> 
> The situation with raw_cpu_generic_add_return() is:
> - pcp is a lvalue of of a __percpu object of type T, so:
> 	typeof(pcp)  := T __percpu
> - pcp's address is given to raw_cpu_ptr(), so
> 	typeof(&pcp) := T __percpu *
> - raw_cpu_ptr() return the corresponding __kernel pointer
>   (adjusted for the current percu offset), so:
> 	typeof(raw_cpu_ptr(&pcp)) := T *
> - so, the macro needs to declare a variable __p of type T*
>   hence:
> 	typeof(pcp) __kernel __force *__p;
>   or, with this new macro:
> 	typeof_cast_kernel(pcp) *__p;
> 
> Maybe a better solution would be to directly play at pointer
> level and thus have something like this:
> 	typeof_<some good name>(&pcp) __p = raw_cpu_ptr(&pcp);
> or even:
> 	__kernel_pointer(&pcp) __p = raw_cpu_ptr(&pcp);
> I dunno.
> 
> Note: at implementation level, it complicates things slightly
>       to want this 'strip_percpu' macro to behaves like typeof()
>       because it means that it can take in argument either an
>       expression or a type. And if it's a type, you can't do a
>       simple cast on it, you need to declare an intermediate
>       variable, hence the horrible:
> 	  typeof(({ typeof(T) __kernel __force __fakename; __fakename; }))
> 
> Note: it would be much much nicer to do all these type generic
>       macros with '__auto_type' (only supported in GCC 4.9 IIUC
>       and supported in sparse but it shouldn't be very hard to do)..
> 
> 
> -- Luc

Thanks for debugging this. I'm still inclined to have a macro for either
cast/force. I do agree it could be misused, but it's no different doing
it in a macro than by just adding __force __kernel.

Thanks,
Dennis

Re: [PATCH] fix __percpu annotation in asm-generic

[Christopher Lameter]

On Mon, 2 Dec 2019, Dennis Zhou wrote:

> I think typeof_cast_kernel() or typeof_force_kernel() are reasonable
> names. I kind of like the idea of cast/force over strip because we're
> really still moving address spaces even if it is moving it back.

I vote for typeof_cast_kernel()...

percpu addresses are more like an alias.... or more precisely an offset to
a base pointer (that already belongs to a certain "address space") and we
use the notion of a distinctly different "address space" in the linker to
categorize these references.

Re: [PATCH] fix __percpu annotation in asm-generic

[Luc Van Oostenryck]

On Mon, Dec 02, 2019 at 02:07:18PM -0500, Dennis Zhou wrote:
> On Sat, Nov 30, 2019 at 01:00:37AM +0100, Luc Van Oostenryck wrote:
> > On Fri, Nov 29, 2019 at 06:11:59PM +0000, Christopher Lameter wrote:
> > > On Wed, 27 Nov 2019, Luc Van Oostenryck wrote:
> > > 
> > > > 1) it would strip any address space, not just __percpu, so:
> > > >    it would need to be combined with __verify_pcpu_ptr() or,
> > > >    * a better name should be used,
> > > 
> > > typeof_cast_kernel() to express the fact that it creates a kernel pointer
> > > and ignored the attributes??
> > 
> > typeof_strip_address_space() would, I think, express this better. 
> > It's not obvious at all to me that 'kernel' in 'typeof_cast_kernel()'
> > relates to the (default) kernel address space.
> > Maybe it's just me. I don't know.
> > 
> 
> I think typeof_cast_kernel() or typeof_force_kernel() are reasonable
> names. I kind of like the idea of cast/force over strip because we're
> really still moving address spaces even if it is moving it back.

Well, 'typeof_cast_kernel()' somehow conveys the idea but sounds
a bit weird as the macro doesn't contain a cast (expression).

> Thanks for debugging this. I'm still inclined to have a macro for either
> cast/force. I do agree it could be misused, but it's no different doing
> it in a macro than by just adding __force __kernel.

I'm glad to help making the kernel type-clean (with the goal of
catching more bugs earlier) but I admit that I absolutely detest
these layers of ugly macros. 

I'm working on a nicer implementation but it's not yet ready.

Best regards,
-- Luc

Re: [PATCH] fix __percpu annotation in asm-generic

[Jason A. Donenfeld]

On Sat, Nov 30, 2019 at 01:00:37AM +0100, Luc Van Oostenryck wrote:
> Note: it would be much much nicer to do all these type generic
>       macros with '__auto_type' (only supported in GCC 4.9 IIUC
>       and supported in sparse but it shouldn't be very hard to do)..

I'm curious to know if you know why we're not using __auto_type. Because
we're stuck on gcc 4.6, or is there a more subtle reason?

Jason

Re: [PATCH] fix __percpu annotation in asm-generic

[Luc Van Oostenryck]

On Mon, Mar 23, 2020 at 10:13:47PM -0600, Jason A. Donenfeld wrote:
> On Sat, Nov 30, 2019 at 01:00:37AM +0100, Luc Van Oostenryck wrote:
> > Note: it would be much much nicer to do all these type generic
> >       macros with '__auto_type' (only supported in GCC 4.9 IIUC
> >       and supported in sparse but it shouldn't be very hard to do)..
> 
> I'm curious to know if you know why we're not using __auto_type. Because
> we're stuck on gcc 4.6, or is there a more subtle reason?

I suppose. I don't remember having ever seen a discussion on the
subject (other than a simple remark like "it would be simpler
with __auto_type").

Mainline Sparse doesn't support it but I've a pending series for it
that seems to work well. 

-- Luc