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

Commit Message

Uros Bizjak April 29, 2024, 9:30 p.m. UTC

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

Patch

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 3bedee1801e2..d2505a47dc27 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -89,10 +89,10 @@ 
 
 #endif /* CONFIG_SMP */
 
-#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
 #define __force_percpu_arg(x)	__force_percpu_prefix "%" #x
 
 /*
@@ -148,7 +148,7 @@ 
 do {									\
 	__pcpu_type_##size pto_val__ = __pcpu_cast_##size(_val);	\
 	if (0) {		                                        \
-		typeof(_var) pto_tmp__;					\
+		__typeof_unqual__(_var) pto_tmp__;			\
 		pto_tmp__ = (_val);					\
 		(void)pto_tmp__;					\
 	}								\
@@ -173,7 +173,7 @@  do {									\
 			      ((val) == 1 || (val) == -1)) ?		\
 				(int)(val) : 0;				\
 	if (0) {							\
-		typeof(var) pao_tmp__;					\
+		__typeof_unqual__(var) pao_tmp__;			\
 		pao_tmp__ = (val);					\
 		(void)pao_tmp__;					\
 	}								\
@@ -223,7 +223,7 @@  do {									\
  */
 #define raw_percpu_xchg_op(_var, _nval)					\
 ({									\
-	typeof(_var) pxo_old__ = raw_cpu_read(_var);			\
+	__typeof_unqual__(_var) pxo_old__ = raw_cpu_read(_var);		\
 	raw_cpu_write(_var, _nval);					\
 	pxo_old__;							\
 })
@@ -235,7 +235,7 @@  do {									\
  */
 #define this_percpu_xchg_op(_var, _nval)				\
 ({									\
-	typeof(_var) pxo_old__ = this_cpu_read(_var);			\
+	__typeof_unqual__(_var) pxo_old__ = this_cpu_read(_var);	\
 	do { } while (!this_cpu_try_cmpxchg(_var, &pxo_old__, _nval));	\
 	pxo_old__;							\
 })
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index a817ed0724d1..f5d6ad351cc4 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -560,9 +560,10 @@  void early_setup_idt(void)
 void __head startup_64_setup_gdt_idt(void)
 {
 	void *handler = NULL;
+	struct desc_struct *gdt = (struct desc_struct *)(uintptr_t)init_per_cpu_var(gdt_page.gdt);
 
 	struct desc_ptr startup_gdt_descr = {
-		.address = (unsigned long)&RIP_REL_REF(init_per_cpu_var(gdt_page.gdt)),
+		.address = (unsigned long)&RIP_REL_REF(*gdt),
 		.size    = GDT_SIZE - 1,
 	};
 
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..ae52721bc79e 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -1223,6 +1223,6 @@  EXPORT_SYMBOL_GPL(__devm_alloc_percpu);
 void devm_free_percpu(struct device *dev, void __percpu *pdata)
 {
 	WARN_ON(devres_destroy(dev, devm_percpu_release, devm_percpu_match,
-			       (__force void *)pdata));
+			       (__force void *)(uintptr_t)pdata));
 }
 EXPORT_SYMBOL_GPL(devm_free_percpu);
diff --git a/fs/aio.c b/fs/aio.c
index 0f4f531c9780..baba27450696 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -100,7 +100,7 @@  struct kioctx {
 
 	unsigned long		user_id;
 
-	struct __percpu kioctx_cpu *cpu;
+	struct kioctx_cpu __percpu *cpu;
 
 	/*
 	 * For percpu reqs_available, number of slots we move to/from global
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 2abaa3a825a9..7c574d686486 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -57,7 +57,7 @@  static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
 #  define __user	BTF_TYPE_TAG(user)
 # endif
 # define __iomem
-# define __percpu	BTF_TYPE_TAG(percpu)
+# define __percpu	__percpu_seg_override BTF_TYPE_TAG(percpu)
 # define __rcu		BTF_TYPE_TAG(rcu)
 
 # define __chk_user_ptr(x)	(void)0
diff --git a/include/linux/part_stat.h b/include/linux/part_stat.h
index abeba356bc3f..0e02e9d60114 100644
--- a/include/linux/part_stat.h
+++ b/include/linux/part_stat.h
@@ -33,7 +33,7 @@  struct disk_stats {
 
 #define part_stat_read(part, field)					\
 ({									\
-	typeof((part)->bd_stats->field) res = 0;			\
+	__typeof_unqual__((part)->bd_stats->field) res = 0;		\
 	unsigned int _cpu;						\
 	for_each_possible_cpu(_cpu)					\
 		res += per_cpu_ptr((part)->bd_stats, _cpu)->field; \
diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index ec3573119923..4cb667887c81 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -233,13 +233,13 @@  do {									\
 #define per_cpu_ptr(ptr, cpu)						\
 ({									\
 	__verify_pcpu_ptr(ptr);						\
-	SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu)));			\
+	(__typeof_unqual__(*(ptr)) *)(uintptr_t)SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \
 })
 
 #define raw_cpu_ptr(ptr)						\
 ({									\
 	__verify_pcpu_ptr(ptr);						\
-	arch_raw_cpu_ptr(ptr);						\
+	(__typeof_unqual__(*(ptr)) *)(uintptr_t)arch_raw_cpu_ptr(ptr);	\
 })
 
 #ifdef CONFIG_DEBUG_PREEMPT
@@ -315,7 +315,7 @@  static __always_inline void __this_cpu_preempt_check(const char *op) { }
 
 #define __pcpu_size_call_return(stem, variable)				\
 ({									\
-	typeof(variable) pscr_ret__;					\
+	__typeof_unqual__(variable) pscr_ret__;				\
 	__verify_pcpu_ptr(&(variable));					\
 	switch(sizeof(variable)) {					\
 	case 1: pscr_ret__ = stem##1(variable); break;			\
@@ -330,7 +330,7 @@  static __always_inline void __this_cpu_preempt_check(const char *op) { }
 
 #define __pcpu_size_call_return2(stem, variable, ...)			\
 ({									\
-	typeof(variable) pscr2_ret__;					\
+	__typeof_unqual__(variable) pscr2_ret__;			\
 	__verify_pcpu_ptr(&(variable));					\
 	switch(sizeof(variable)) {					\
 	case 1: pscr2_ret__ = stem##1(variable, __VA_ARGS__); break;	\
diff --git a/include/linux/prandom.h b/include/linux/prandom.h
index f7f1e5251c67..f2ed5b72b3d6 100644
--- a/include/linux/prandom.h
+++ b/include/linux/prandom.h
@@ -10,6 +10,7 @@ 
 
 #include <linux/types.h>
 #include <linux/once.h>
+#include <linux/percpu.h>
 #include <linux/random.h>
 
 struct rnd_state {
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6c2cb4e4f48d..d82fe78f0658 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -849,7 +849,7 @@  register_wide_hw_breakpoint(struct perf_event_attr *attr,
 
 	cpu_events = alloc_percpu(typeof(*cpu_events));
 	if (!cpu_events)
-		return (void __percpu __force *)ERR_PTR(-ENOMEM);
+		return (void __percpu __force *)(uintptr_t)ERR_PTR(-ENOMEM);
 
 	cpus_read_lock();
 	for_each_online_cpu(cpu) {
@@ -868,7 +868,7 @@  register_wide_hw_breakpoint(struct perf_event_attr *attr,
 		return cpu_events;
 
 	unregister_wide_hw_breakpoint(cpu_events);
-	return (void __percpu __force *)ERR_PTR(err);
+	return (void __percpu __force *)(uintptr_t)ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
 
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 6083883c4fe0..1c8fca7e6fd6 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -184,7 +184,7 @@  EXPORT_SYMBOL_GPL(__percpu_down_read);
 
 #define per_cpu_sum(var)						\
 ({									\
-	typeof(var) __sum = 0;						\
+	__typeof_unqual__(var) __sum = 0;				\
 	int cpu;							\
 	compiletime_assert_atomic_type(__sum);				\
 	for_each_possible_cpu(cpu)					\
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f397510edc9b..7dd6392c9c52 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -377,7 +377,7 @@  struct workqueue_struct {
 
 	/* hot fields used during command issue, aligned to cacheline */
 	unsigned int		flags ____cacheline_aligned; /* WQ: WQ_* flags */
-	struct pool_workqueue __percpu __rcu **cpu_pwq; /* I: per-cpu pwqs */
+	struct pool_workqueue * __percpu __rcu *cpu_pwq; /* I: per-cpu pwqs */
 	struct wq_node_nr_active *node_nr_active[]; /* I: per-node nr_active */
 };
 
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 44dd133594d4..74f438f469d8 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -175,7 +175,7 @@  int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount,
 		INIT_LIST_HEAD(&fbc[i].list);
 #endif
 		fbc[i].count = amount;
-		fbc[i].counters = (void *)counters + (i * counter_size);
+		fbc[i].counters = (void __percpu *)counters + (i * counter_size);
 
 		debug_percpu_counter_activate(&fbc[i]);
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index 331848eca7d3..4dfc0ea92513 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10651,7 +10651,7 @@  noinline void netdev_core_stats_inc(struct net_device *dev, u32 offset)
 			return;
 	}
 
-	field = (__force unsigned long __percpu *)((__force void *)p + offset);
+	field = (unsigned long __percpu *)(void __percpu *)(p + offset);
 	this_cpu_inc(*field);
 }
 EXPORT_SYMBOL_GPL(netdev_core_stats_inc);