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 <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Reported-by: Ben Dooks <[email protected]>
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
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
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 <[email protected]>
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?
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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
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?
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
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
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.
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
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
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