[PATCH 1/3] percpu: Merge VERIFY_PERCPU_PTR() into its only user

[PATCH 1/3] percpu: Merge VERIFY_PERCPU_PTR() into its only user

[Uros Bizjak]

Merge VERIFY_PERCPU_PTR() into non-CONFIG_SMP per_cpu_ptr()
to make macro similar to CONFIG_SMP per_cpu_ptr(). This will
allow a follow-up patch to refactor common code to a macro.

No functional changes, non-CONFIG_SMP per_cpu_ptr() was
the only user of VERIFY_PERCPU_PTR().

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/percpu-defs.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index 8efce7414fad..7fa88c5f4b26 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -254,13 +254,13 @@ do {									\
 
 #else	/* CONFIG_SMP */
 
-#define VERIFY_PERCPU_PTR(__p)						\
+#define per_cpu_ptr(ptr, cpu)						\
 ({									\
-	__verify_pcpu_ptr(__p);						\
-	(typeof(*(__p)) __kernel __force *)(__p);			\
+	(void)(cpu);							\
+	__verify_pcpu_ptr(ptr);						\
+	(typeof(*(ptr)) __kernel __force *)(ptr);			\
 })
 
-#define per_cpu_ptr(ptr, cpu)	({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); })
 #define raw_cpu_ptr(ptr)	per_cpu_ptr(ptr, 0)
 #define this_cpu_ptr(ptr)	raw_cpu_ptr(ptr)
 
-- 
2.47.0

[PATCH 2/3] percpu: Introduce PERCPU_PTR() macro

[Uros Bizjak]

Introduce PERCPU_PTR() macro to cast the percpu pointer from
the percpu address space to a generic (kernel) address space.
Use it in per_cpu_ptr() and related SHIFT_PERCPU_PTR() macros.

Also remove common knowledge from SHIFT_PERCPU_PTR() comment,
"weird cast" is just a standard way to inform sparse of a cast
from the percpu address space to a generic address space.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/percpu-defs.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index 7fa88c5f4b26..e1cf7982424f 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -220,15 +220,17 @@ do {									\
 	(void)__vpp_verify;						\
 } while (0)
 
+#define PERCPU_PTR(__p)							\
+	(typeof(*(__p)) __force __kernel *)(__p);
+
 #ifdef CONFIG_SMP
 
 /*
- * Add an offset to a pointer but keep the pointer as-is.  Use RELOC_HIDE()
- * to prevent the compiler from making incorrect assumptions about the
- * pointer value.  The weird cast keeps both GCC and sparse happy.
+ * Add an offset to a pointer.  Use RELOC_HIDE() to prevent the compiler
+ * from making incorrect assumptions about the pointer value.
  */
 #define SHIFT_PERCPU_PTR(__p, __offset)					\
-	RELOC_HIDE((typeof(*(__p)) __kernel __force *)(__p), (__offset))
+	RELOC_HIDE(PERCPU_PTR(__p), (__offset))
 
 #define per_cpu_ptr(ptr, cpu)						\
 ({									\
@@ -258,7 +260,7 @@ do {									\
 ({									\
 	(void)(cpu);							\
 	__verify_pcpu_ptr(ptr);						\
-	(typeof(*(ptr)) __kernel __force *)(ptr);			\
+	PERCPU_PTR(ptr);						\
 })
 
 #define raw_cpu_ptr(ptr)	per_cpu_ptr(ptr, 0)
-- 
2.47.0

[PATCH 3/3] percpu: Cast percpu pointer in PERCPU_PTR() via unsigned long

[Uros Bizjak]

Cast pointer from percpu address space to generic (kernel) address
space in PERCPU_PTR() macro via unsigned long intermediate cast [1].
This intermediate cast is also required to avoid build failure
when GCC's strict named address space checks for x86 targets [2]
are enabled.

Found by GCC's named address space checks.

[1] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
[2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/percpu-defs.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
index e1cf7982424f..35842d1e3879 100644
--- a/include/linux/percpu-defs.h
+++ b/include/linux/percpu-defs.h
@@ -221,7 +221,10 @@ do {									\
 } while (0)
 
 #define PERCPU_PTR(__p)							\
-	(typeof(*(__p)) __force __kernel *)(__p);
+({									\
+	unsigned long __pcpu_ptr = (__force unsigned long)(__p);	\
+	(typeof(*(__p)) __force __kernel *)(__pcpu_ptr);		\
+})
 
 #ifdef CONFIG_SMP
 
-- 
2.47.0

Re: [PATCH 2/3] percpu: Introduce PERCPU_PTR() macro

[Christoph Lameter (Ampere)]

On Mon, 21 Oct 2024, Uros Bizjak wrote:

> Also remove common knowledge from SHIFT_PERCPU_PTR() comment,
> "weird cast" is just a standard way to inform sparse of a cast
> from the percpu address space to a generic address space.

Ok. Thanks for enlightening my ignorance after all these years.

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH 3/3] percpu: Cast percpu pointer in PERCPU_PTR() via unsigned long

[Gal Pressman]

On 21/10/2024 11:07, Uros Bizjak wrote:
> Cast pointer from percpu address space to generic (kernel) address
> space in PERCPU_PTR() macro via unsigned long intermediate cast [1].
> This intermediate cast is also required to avoid build failure
> when GCC's strict named address space checks for x86 targets [2]
> are enabled.
> 
> Found by GCC's named address space checks.
> 
> [1] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
> [2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Dennis Zhou <dennis@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  include/linux/percpu-defs.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> index e1cf7982424f..35842d1e3879 100644
> --- a/include/linux/percpu-defs.h
> +++ b/include/linux/percpu-defs.h
> @@ -221,7 +221,10 @@ do {									\
>  } while (0)
>  
>  #define PERCPU_PTR(__p)							\
> -	(typeof(*(__p)) __force __kernel *)(__p);
> +({									\
> +	unsigned long __pcpu_ptr = (__force unsigned long)(__p);	\
> +	(typeof(*(__p)) __force __kernel *)(__pcpu_ptr);		\
> +})
>  
>  #ifdef CONFIG_SMP
>  

Hello Uros,

We've encountered a kernel panic on boot [1] bisected to this patch.
I believe the patch is fine and the issue is caused by a compiler bug.
The panic reproduces when compiling the kernel with gcc 11.3.1, but does
not reproduce with latest gcc/clang.

I have a patch that workarounds the issue by ditching the intermediate
variable and does the casting in a single line. Will that be enough to
solve the sparse/build issues?
Do you have anything else in mind?

Thanks

[1]

 [   46.055628] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] SMP KASAN
 [   46.057809] KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
 [   46.059429] CPU: 0 UID: 0 PID: 547 Comm: iptables Not tainted 6.13.0-rc1_external_tested-master #1
 [   46.061243] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
 [   46.063453] RIP: 0010:nf_ct_netns_do_get+0x139/0x540
 [   46.064513] Code: 03 00 00 48 81 c4 88 00 00 00 5b 5d 41 5c 41 5d 41 5e 41 5f c3 4d 8d 75 08 48 b8 00 00 00 00 00 fc ff df 4c 89 f2 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 27 03 00 00 41 8b 45 08 83 c0
 [   46.068081] RSP: 0018:ffff888116df75e8 EFLAGS: 00010207
 [   46.069186] RAX: dffffc0000000000 RBX: 1ffff11022dbeebe RCX: ffffffff839a2382
 [   46.070662] RDX: 0000000000000003 RSI: 0000000000000008 RDI: ffff88842ec46d10
 [   46.072086] RBP: 0000000000000002 R08: 0000000000000000 R09: fffffbfff0b0860c
 [   46.073513] R10: ffff888116df75e8 R11: 0000000000000001 R12: ffffffff879d6a80
 [   46.075021] R13: 0000000000000016 R14: 000000000000001e R15: ffff888116df7908
 [   46.076447] FS:  00007fba01646740(0000) GS:ffff88842ec00000(0000) knlGS:0000000000000000
 [   46.078159] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [   46.079353] CR2: 000055bd901800d8 CR3: 00000001205f0003 CR4: 0000000000172eb0
 [   46.080772] Call Trace:
 [   46.081417]  <TASK>
 [   46.082014]  ? die_addr+0x3d/0xa0
 [   46.082855]  ? exc_general_protection+0x144/0x220
 [   46.083893]  ? asm_exc_general_protection+0x22/0x30
 [   46.084934]  ? __mutex_lock+0x2c2/0x1d70
 [   46.085859]  ? nf_ct_netns_do_get+0x139/0x540
 [   46.086858]  ? nf_ct_netns_do_get+0xb5/0x540
 [   46.087812]  ? net_generic+0x1f0/0x1f0
 [   46.088663]  ? __create_object+0x5e/0x80
 [   46.089559]  xt_check_target+0x1f0/0x930
 [   46.090520]  ? textify_hooks.constprop.0+0x110/0x110
 [   46.091581]  ? pcpu_alloc_noprof+0x7cd/0xcf0
 [   46.092535]  ? xt_find_target+0x148/0x1e0
 [   46.093434]  find_check_entry.constprop.0+0x6c0/0x920
 [   46.094582]  ? get_info+0x380/0x380
 [   46.095408]  ? __virt_addr_valid+0x1df/0x3b0
 [   46.096363]  ? kasan_quarantine_put+0xe3/0x200
 [   46.097341]  ? kfree+0x13e/0x3d0
 [   46.098171]  ? translate_table+0xaf5/0x1750
 [   46.099114]  translate_table+0xbd8/0x1750
 [   46.100021]  ? ipt_unregister_table_exit+0x30/0x30
 [   46.101047]  ? __might_fault+0xbb/0x170
 [   46.101937]  do_ipt_set_ctl+0x408/0x1340
 [   46.102856]  ? nf_sockopt_find.constprop.0+0x17b/0x1f0
 [   46.103943]  ? lock_downgrade+0x680/0x680
 [   46.104849]  ? lockdep_hardirqs_on_prepare+0x284/0x400
 [   46.105973]  ? ipt_register_table+0x440/0x440
 [   46.106978]  ? bit_wait_timeout+0x160/0x160
 [   46.107916]  nf_setsockopt+0x6f/0xd0
 [   46.108745]  raw_setsockopt+0x7e/0x200
 [   46.109608]  ? raw_bind+0x590/0x590
 [   46.110462]  ? do_user_addr_fault+0x812/0xd20
 [   46.111425]  do_sock_setsockopt+0x1e2/0x3f0
 [   46.112360]  ? move_addr_to_user+0x90/0x90
 [   46.113279]  ? lock_downgrade+0x680/0x680
 [   46.114225]  __sys_setsockopt+0x9e/0x100
 [   46.115420]  __x64_sys_setsockopt+0xb9/0x150
 [   46.116413]  ? do_syscall_64+0x33/0x140
 [   46.117296]  do_syscall_64+0x6d/0x140
 [   46.118194]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
 [   46.119289] RIP: 0033:0x7fba015134ce
 [   46.120129] Code: 0f 1f 40 00 48 8b 15 59 69 0e 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b1 0f 1f 00 f3 0f 1e fa 49 89 ca b8 36 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 0a c3 66 0f 1f 84 00 00 00 00 00 48 8b 15 21
 [   46.123951] RSP: 002b:00007ffd9de6f388 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
 [   46.125530] RAX: ffffffffffffffda RBX: 000055bd9017f490 RCX: 00007fba015134ce
 [   46.127088] RDX: 0000000000000040 RSI: 0000000000000000 RDI: 0000000000000004
 [   46.128503] RBP: 0000000000000500 R08: 0000000000000560 R09: 0000000000000052
 [   46.129920] R10: 000055bd901800e0 R11: 0000000000000246 R12: 000055bd90180140
 [   46.131445] R13: 000055bd901800e0 R14: 000055bd9017f498 R15: 000055bd9017ff10
 [   46.133422]  </TASK>
 [   46.134351] Modules linked in: xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat br_netfilter rpcsec_gss_krb5 auth_rpcgss oid_registry overlay zram zsmalloc mlx4_ib mlx4_en mlx4_core rpcrdma rdma_ucm ib_uverbs ib_iser libiscsi scsi_transport_iscsi fuse ib_umad rdma_cm ib_ipoib iw_cm ib_cm ib_core
 [   46.140665] ---[ end trace 0000000000000000 ]---

Re: [PATCH 3/3] percpu: Cast percpu pointer in PERCPU_PTR() via unsigned long

[Uros Bizjak]

On Wed, Dec 18, 2024 at 8:54 AM Gal Pressman <gal@nvidia.com> wrote:
>
> On 21/10/2024 11:07, Uros Bizjak wrote:
> > Cast pointer from percpu address space to generic (kernel) address
> > space in PERCPU_PTR() macro via unsigned long intermediate cast [1].
> > This intermediate cast is also required to avoid build failure
> > when GCC's strict named address space checks for x86 targets [2]
> > are enabled.
> >
> > Found by GCC's named address space checks.
> >
> > [1] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
> > [2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces
> >
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > Cc: Dennis Zhou <dennis@kernel.org>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  include/linux/percpu-defs.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> > index e1cf7982424f..35842d1e3879 100644
> > --- a/include/linux/percpu-defs.h
> > +++ b/include/linux/percpu-defs.h
> > @@ -221,7 +221,10 @@ do {                                                                     \
> >  } while (0)
> >
> >  #define PERCPU_PTR(__p)                                                      \
> > -     (typeof(*(__p)) __force __kernel *)(__p);
> > +({                                                                   \
> > +     unsigned long __pcpu_ptr = (__force unsigned long)(__p);        \
> > +     (typeof(*(__p)) __force __kernel *)(__pcpu_ptr);                \
> > +})
> >
> >  #ifdef CONFIG_SMP
> >
>
> Hello Uros,
>
> We've encountered a kernel panic on boot [1] bisected to this patch.
> I believe the patch is fine and the issue is caused by a compiler bug.
> The panic reproduces when compiling the kernel with gcc 11.3.1, but does
> not reproduce with latest gcc/clang.
>
> I have a patch that workarounds the issue by ditching the intermediate
> variable and does the casting in a single line. Will that be enough to
> solve the sparse/build issues?

Yes, single line like:

(typeof(*(__p)) __force __kernel *)(__force unsigned long)(__pcpu_ptr);

should be OK.

Thanks,
Uros.

Re: [PATCH 3/3] percpu: Cast percpu pointer in PERCPU_PTR() via unsigned long

[Gal Pressman]

On 18/12/2024 10:40, Uros Bizjak wrote:
> On Wed, Dec 18, 2024 at 8:54 AM Gal Pressman <gal@nvidia.com> wrote:
>>
>> On 21/10/2024 11:07, Uros Bizjak wrote:
>>> Cast pointer from percpu address space to generic (kernel) address
>>> space in PERCPU_PTR() macro via unsigned long intermediate cast [1].
>>> This intermediate cast is also required to avoid build failure
>>> when GCC's strict named address space checks for x86 targets [2]
>>> are enabled.
>>>
>>> Found by GCC's named address space checks.
>>>
>>> [1] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
>>> [2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces
>>>
>>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>>> Cc: Dennis Zhou <dennis@kernel.org>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>>  include/linux/percpu-defs.h | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
>>> index e1cf7982424f..35842d1e3879 100644
>>> --- a/include/linux/percpu-defs.h
>>> +++ b/include/linux/percpu-defs.h
>>> @@ -221,7 +221,10 @@ do {                                                                     \
>>>  } while (0)
>>>
>>>  #define PERCPU_PTR(__p)                                                      \
>>> -     (typeof(*(__p)) __force __kernel *)(__p);
>>> +({                                                                   \
>>> +     unsigned long __pcpu_ptr = (__force unsigned long)(__p);        \
>>> +     (typeof(*(__p)) __force __kernel *)(__pcpu_ptr);                \
>>> +})
>>>
>>>  #ifdef CONFIG_SMP
>>>
>>
>> Hello Uros,
>>
>> We've encountered a kernel panic on boot [1] bisected to this patch.
>> I believe the patch is fine and the issue is caused by a compiler bug.
>> The panic reproduces when compiling the kernel with gcc 11.3.1, but does
>> not reproduce with latest gcc/clang.
>>
>> I have a patch that workarounds the issue by ditching the intermediate
>> variable and does the casting in a single line. Will that be enough to
>> solve the sparse/build issues?
> 
> Yes, single line like:
> 
> (typeof(*(__p)) __force __kernel *)(__force unsigned long)(__pcpu_ptr);
> 
> should be OK.
> 
> Thanks,
> Uros.

Awesome, I'll submit a patch unless I hear any objections.
Thanks Uros!

Re: [PATCH 3/3] percpu: Cast percpu pointer in PERCPU_PTR() via unsigned long

[Uros Bizjak]

On Wed, Dec 18, 2024 at 9:46 AM Gal Pressman <gal@nvidia.com> wrote:
>
> On 18/12/2024 10:40, Uros Bizjak wrote:
> > On Wed, Dec 18, 2024 at 8:54 AM Gal Pressman <gal@nvidia.com> wrote:
> >>
> >> On 21/10/2024 11:07, Uros Bizjak wrote:
> >>> Cast pointer from percpu address space to generic (kernel) address
> >>> space in PERCPU_PTR() macro via unsigned long intermediate cast [1].
> >>> This intermediate cast is also required to avoid build failure
> >>> when GCC's strict named address space checks for x86 targets [2]
> >>> are enabled.
> >>>
> >>> Found by GCC's named address space checks.
> >>>
> >>> [1] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
> >>> [2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces
> >>>
> >>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> >>> Cc: Dennis Zhou <dennis@kernel.org>
> >>> Cc: Tejun Heo <tj@kernel.org>
> >>> Cc: Christoph Lameter <cl@linux.com>
> >>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>> ---
> >>>  include/linux/percpu-defs.h | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> >>> index e1cf7982424f..35842d1e3879 100644
> >>> --- a/include/linux/percpu-defs.h
> >>> +++ b/include/linux/percpu-defs.h
> >>> @@ -221,7 +221,10 @@ do {                                                                     \
> >>>  } while (0)
> >>>
> >>>  #define PERCPU_PTR(__p)                                                      \
> >>> -     (typeof(*(__p)) __force __kernel *)(__p);
> >>> +({                                                                   \
> >>> +     unsigned long __pcpu_ptr = (__force unsigned long)(__p);        \
> >>> +     (typeof(*(__p)) __force __kernel *)(__pcpu_ptr);                \
> >>> +})
> >>>
> >>>  #ifdef CONFIG_SMP
> >>>
> >>
> >> Hello Uros,
> >>
> >> We've encountered a kernel panic on boot [1] bisected to this patch.
> >> I believe the patch is fine and the issue is caused by a compiler bug.
> >> The panic reproduces when compiling the kernel with gcc 11.3.1, but does
> >> not reproduce with latest gcc/clang.
> >>
> >> I have a patch that workarounds the issue by ditching the intermediate
> >> variable and does the casting in a single line. Will that be enough to
> >> solve the sparse/build issues?
> >
> > Yes, single line like:
> >
> > (typeof(*(__p)) __force __kernel *)(__force unsigned long)(__pcpu_ptr);
> >
> > should be OK.
> >
> > Thanks,
> > Uros.
>
> Awesome, I'll submit a patch unless I hear any objections.

Please note that this is/was a preparation patch for further changes,
applied to akpm's mm branch (typeof() -> TYPEOF_UNQUAL()). I'll take
care to forward-port your patch to mm branch.

AFAICS, the issue triggers only when KASAN is enabled.

Thanks,
Uros.