On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <tglx@linutronix.de> wrote:

> >> > That's so sad because it would provide us compiler based __percpu
> >> > validation.
> >>
> >> Unfortunately, the c compiler can't strip qualifiers, so typeof() is
> >> of limited use also when const and volatile qualifiers are used.
> >> Perhaps some extension could be introduced to c standard to provide an
> >> unqualified type, e.g. typeof_unqual().
> >
> > Oh, there is one in C23 [1].
>
> Yes. I found it right after ranting.
>
> gcc >= 14 and clang >= 16 have support for it of course only when adding
> -std=c2x to the command line.
>
> Sigh. The name space qualifiers are non standard and then the thing
> which makes them more useful is hidden behind a standard.
>
> Why can't we have useful tools?
>
> Though the whole thing looks worthwhile:
>
> #define verify_per_cpu_ptr(ptr)                                         \
> do {                                                                    \
>         const void __seg_gs *__vpp_verify = (typeof((ptr) + 0))NULL;    \
>         (void)__vpp_verify;                                             \
> } while (0)
>
> #define per_cpu_ptr(ptr, cpu)                                           \
> ({                                                                      \
>         verify_per_cpu_ptr(ptr);                                        \
>         (typeof_unqual(*(ptr)) *)(uintptr_t)ptr + per_cpu_offset(cpu);  \
> })
>
> unsigned int __seg_gs test;
>
> unsigned int foo1(unsigned int cpu)
> {
>         return *per_cpu_ptr(&test, cpu);
> }
>
> unsigned int foo2(unsigned int cpu)
> {
>         unsigned int x, *p = per_cpu_ptr(&x, cpu);
>
>         return *p;
> }
>
> x.c:29:23: error: initializing 'const __attribute__((address_space(256))) void *' with an expression of type 'typeof ((&x) + 0)' (aka 'unsigned int *') changes address space of pointer
>         unsigned int x, *p = per_cpu_ptr(&x, cpu);
>
> That's exactly what we want. It would have caught all the long standing
> and ignored __percpu sparse warnings right away.
>
> This also simplifies all the other per cpu accessors. The most trivial
> is read()
>
> #define verify_per_cpu(variable)                                        \
> {                                                                       \
>         const unsigned int __s = sizeof(variable);                      \
>                                                                         \
>         verify_per_cpu_ptr(&(variable));                                \
>         BUILD_BUG_ON(__s == 1 || __s == 2 || __s == 4 || __s == 8,      \
>                      "Wrong size for per CPU variable");                \
> }
>
> #define __pcpu_read(variable)                                           \
> ({                                                                      \
>         verify_per_cpu(variable);                                       \
>         READ_ONCE(variable);                                            \
> })
>
> which in turn catches all the mistakes, i.e. wrong namespace and wrong
> size.
>
> I'm really tempted to implement this as an alternative to the current
> pile of macro horrors. Of course this requires to figure out first what
> kind of damage -std=c2x will do.
>
> I get to that in my copious spare time some day.

Please find attached the prototype patch that does the above.

The idea of the patch is to add named address qualifier to the __percpu tag:

-# define __percpu    BTF_TYPE_TAG(percpu)
+# define __percpu    __percpu_seg_override BTF_TYPE_TAG(percpu)

So instead of being merely a benign hint to the checker, __percpu
becomes the real x86 named address space qualifier to enable the
compiler checks for access to different address spaces. Following the
above change, we can remove various casts that cast "fake" percpu
addresses at the usage site and use the kernel type system to handle
named AS qualified addresses instead:

-#define __my_cpu_type(var)    typeof(var) __percpu_seg_override
-#define __my_cpu_ptr(ptr)    (__my_cpu_type(*(ptr))*)(__force uintptr_t)(ptr)
-#define __my_cpu_var(var)    (*__my_cpu_ptr(&(var)))
-#define __percpu_arg(x)        __percpu_prefix "%" #x
+#define __my_cpu_type(var)    typeof(var)
+#define __my_cpu_ptr(ptr)    (ptr)
+#define __my_cpu_var(var)    (var)
+#define __percpu_arg(x)        "%" #x

As can be seen from the patch, various temporary non-percpu variables
need to be declared with __typeof_unqual__ to use unqualified base
type without named AS qualifier. In addition to the named AS
qualifier, __typeof_unqual__ also strips const and volatile
qualifiers, so it can enable some further optimizations involving
this_cpu_read_stable, not a topic of this patch.

The patch is against the recent -tip tree and needs to be compiled
with gcc-14. It is tested by compiling and booting the defconfig
kernel, but other than that, as a prototype patch, it does not even
try to be a generic patch that would handle compilers without
__typeof_unqual__ support. The patch unearths and fixes some address
space inconsistencies to avoid __verify_pcpu_ptr and x86 named address
space compile failures with a defconfig compilation, demonstrating the
effectiveness of the proposed approach.

Uros.