From patchwork Mon Apr 29 21:30:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Uros Bizjak X-Patchwork-Id: 13647759 Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DF5431836DE; Mon, 29 Apr 2024 21:30:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.174 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714426226; cv=none; b=lt3gKh1WEnkDL1m4IgopVdiY6oJwGWt7UwpS1Hsx8NFs3AcRdk7QAEvAJ+H+GL4dEZ04B23M5VLpZdzQUhbzBgIr4hEwRtPaTFngJIpBMiFRalL8Wc7Lnj7nH4klqDast+d/p8gi8CGeJv1OZDiI8F+gv5SQ3mLiEOHEPYWdEGo= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714426226; c=relaxed/simple; bh=kpvICPZMlsZfPJroQtt4wrumVDC1dFOAcBXHJhfrORg=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=EcmsWIJ8NqzaZqxC2TTuI+8NlDYItDrnUpl5AD45RnkRhf9kq3O/+mRUB5JDhjwZMqrqyjLBQ7gqeiwg63l1Hjt+tjkFcHZcZ4Y/4qGzSS8T6nQgqCETK9xzAR8yNooLupJYEDjSOLsPIGHlwcm6tFuic4AwILnQdP67A1r/M3s= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=D8sSBLeL; arc=none smtp.client-ip=209.85.208.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="D8sSBLeL" Received: by mail-lj1-f174.google.com with SMTP id 38308e7fff4ca-2e060d49f87so11553201fa.0; Mon, 29 Apr 2024 14:30:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714426222; x=1715031022; darn=vger.kernel.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OIprfau9HNmVx8+h0zObXjXi6FS5vux/SKaDT2brkWE=; b=D8sSBLeLB6QgC9VLfrx2FJktuj2Tfp0v7AZzgR2NmTlXR+p50bQKZduODlIjgEpdDe s1MG/urzwT9qpjw+zZmZgLYE0SkSRiC6Tuc+4Ak6sGNcuH8E4G6mLheVNo79FTLZrYpd /V2eaqaZDc8SK50hJP98SrUuSdd7Oukws+6+MLogRJ0/vYUB9X+3mT1scz5j9PoM2xf5 RpW/Bw3hcOsM+PrLUSoWZlLk7+bvddA99LfHWw5xJAZ+0P2mVh3+uqaY+/alV3rcqMH8 fhZd9yG4mm5epE+Mjm+qSIonp4FGiBcJEJGD3vdnvNe5wsg+L1W2Rb7aXYKcuOP+1KIc iBhA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714426222; x=1715031022; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OIprfau9HNmVx8+h0zObXjXi6FS5vux/SKaDT2brkWE=; b=A+cpRxBSCwA9+YBKrZePQHPQf7GWZ0avHYhEGHO/CEUAayCIguWfhoSwMPzejyXW1M O/GDsKRO4QMYnrKuR7JppmhaXPhLY00Rn4SitqFld9IgItzIol6V6FLwrTgYSDFVIal9 d1TfR7nunIj/xNjcC6NbUDIPdP9t0VvuYy5b6JwQ0r1cyfq4QOJHSLKrhs+e+V8G8pwY Pw0AERLk+hjmdNkU0N0WlMZQLsfaUt7lIZUZ+8Et8VrvIkRfCtVCLwgXcYvg9gago5GF hrzMb+YK5uNhGw/uN4d+PzRsUyQhuvFYj9uRtwzENsCBsf8P322WUgWUceFV9q8fxQ+E 5KiA== X-Forwarded-Encrypted: i=1; AJvYcCVXVepzQL45nUyMPg3ZvW1Nh5SSucSwuXEV6C7LdC+dL0J0BSIQhndHPEvj+SH/SgGthrKbaG3g4h4GVlwwzybA5GIOCpRTHtoOjGGwVYUf8bTThouW0CGwaJR5L+E0rvfyqMwyz2L/4UCX X-Gm-Message-State: AOJu0YzMx3jHMNwsGG5y5xDRmP/uDHopPr6nqxp8OKunBfSaSMdJN1g0 HOioXa3WGJcP3LyGb7SPgmmkcyVPzOef2333GfsZokXtZYHuYT3zVw/iOnXnmmMkYQzx3GN9tY2 00UhrOwC9dVc2EYLc/D0RTlVg+z0= X-Google-Smtp-Source: AGHT+IGUjTrac3O9enRphCIrmfCciEBg6iGpG+lO8D6luCCSLtsdNIPzi1Qdj7I+7ZQiQPc/8jJM6aICVlp3GzakuzI= X-Received: by 2002:a05:651c:198c:b0:2df:1c6a:214b with SMTP id bx12-20020a05651c198c00b002df1c6a214bmr322490ljb.13.1714426221796; Mon, 29 Apr 2024 14:30:21 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-sparse@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <202403020457.RCJoQ3ts-lkp@intel.com> <87edctwr6y.ffs@tglx> <87a5nhwpus.ffs@tglx> <87y1b0vp8m.ffs@tglx> <87sf18vdsq.ffs@tglx> <87le70uwf0.ffs@tglx> <87edcruvja.ffs@tglx> <87bk7vuldh.ffs@tglx> <87bk7ux4e9.ffs@tglx> In-Reply-To: <87bk7ux4e9.ffs@tglx> From: Uros Bizjak Date: Mon, 29 Apr 2024 23:30:09 +0200 Message-ID: Subject: [RFC PATCH] Use x86 named address spaces to catch "sparse: incorrect type in initializer (different address spaces)" __percpu errors To: Thomas Gleixner Cc: Linus Torvalds , LKML , Arjan van de Ven , X86 ML , Luc Van Oostenryck , Sparse Mailing-list , "Paul E. McKenney" , Ingo Molnar , Borislav Petkov , Dave Hansen , "H. Peter Anvin" , Dennis Zhou , Tejun Heo , Christoph Lameter On Mon, Mar 4, 2024 at 12:49 AM Thomas Gleixner 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. 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 #include +#include #include 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);