by Uros Bizjak

[permalink] [raw]

Subject: Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

On Wed, Apr 3, 2024 at 7:57 PM Nathan Chancellor <[email protected]> wrote:

> > > > With GCC, you can use __typeof_unqual__ (please note underscores)
> > > > without -std=c2x [1]:
> > > >
> > > > "... Alternate spelling __typeof_unqual__ is available in all C modes
> > > > and provides non-atomic unqualified version of what __typeof__
> > > > operator returns..."
> > > >
> > > > Please also see the example in my last post. It can be compiled without -std=...
> > >
> > > With gcc >= 14. Not so with clang...
> >
> > Please note that clang-17.0.6 currently fails to compile kernel with
> > named address spaces [1]. So perhaps kernel can use __typeof_unqual__
> > (available without -std=c2x) in the hope that clang implements
> > __typeof_unqual__ in one of its next releases, following the examples
> > of GCC [2] and MSVC[3].
>
> This is now supported in clang 19.0.0 (main):
>
> https://github.com/llvm/llvm-project/commit/cc308f60d41744b5920ec2e2e5b25e1273c8704b
>
> I have inquired about applying this to the 18.x series, such that it
> would either make 18.1.3 or 18.1.4, but that is still open for
> discussion.
>
> I think the error that I mentioned at [1] is resolved with using
> __typeof_unqual__, I tested this diff, which is likely incorrect but
> allows me to continue testing without that warning/error due to -Werror:
>
> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> index 20696df5d567..fc77c99d2e80 100644
> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -95,7 +95,7 @@
>
> #endif /* CONFIG_SMP */
>
> -#define __my_cpu_type(var) typeof(var) __percpu_seg_override
> +#define __my_cpu_type(var) __typeof_unqual__(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

IMO, the above change is not correct. Currently, the percpu variables
still live in generic address space, and the above casts are used at
the usage site of the percpu variable to convert pointer from generic
to disjoint __seg_gs address space (please see [2], section 6.17.5).
The -Wduplicate-decl-specifier warning at [1] (if correct) perhaps
points to the percpu accessor chain. GCC does not care about duplicate
__seg_gs conversions (the operation is idempotent), but the issue
should be corrected nevertheless.

BTW: With the above approach we get all the benefits of named address
spaces, but *not* checks for invalid access between disjoint address
spaces. This check is currently done by sparse (this is the reason for
__force in the above cast chain), but the check is not enabled by
default. The proposed improvement would *define* the percpu variable
in __seg_gs named address space, so the compiler will error out with
"assignment/return from pointer to non-enclosed address space" when
invalid access is detected (please see attached testcase, should be
compiled with gcc-14 due to usage of __typeof_unqual__) or warn with
"cast to generic address space pointer from disjoint ‘__seg_gs’
address space pointer" with explicit cast.

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

Uros.

Attachments:

seg.c (549.00 B)

2024-04-29 21:38:15

by Uros Bizjak

[permalink] [raw]

Subject: [RFC PATCH] Use x86 named address spaces to catch "sparse: incorrect type in initializer (different address spaces)" __percpu errors

On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner <[email protected]> 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.

Attachments:

pcpu-unqual.diff.txt (8.51 kB)