| Message ID | 20241021080856.48746-3-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] percpu: Merge VERIFY_PERCPU_PTR() into its only user | expand |
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 ]---
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.
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!
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.
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
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(-)