Message ID | a42260e77d609a10667872c2e7bf14ceabcce050.1544562518.git.christophe.leroy@c-s.fr (mailing list archive) |
---|---|
State | Accepted |
Commit | 6bf752daca07c85c181159f75dcf65b12056883b |
Headers | show |
Series | [v3] powerpc: implement CONFIG_DEBUG_VIRTUAL | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
snowpatch_ozlabs/build-ppc64le | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64be | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64e | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-pmac32 | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
Christophe Leroy <christophe.leroy@c-s.fr> writes: > This patch implements CONFIG_DEBUG_VIRTUAL to warn about > incorrect use of virt_to_phys() and page_to_phys() > > Below is the result of test_debug_virtual: > > [ 1.438746] WARNING: CPU: 0 PID: 1 at ./arch/powerpc/include/asm/io.h:808 test_debug_virtual_init+0x3c/0xd4 > [ 1.448156] CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc5-00560-g6bfb52e23a00-dirty #532 > [ 1.457259] NIP: c066c550 LR: c0650ccc CTR: c066c514 > [ 1.462257] REGS: c900bdb0 TRAP: 0700 Not tainted (4.20.0-rc5-00560-g6bfb52e23a00-dirty) > [ 1.471184] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 48000422 XER: 20000000 > [ 1.477811] > [ 1.477811] GPR00: c0650ccc c900be60 c60d0000 00000000 006000c0 c9000000 00009032 c7fa0020 > [ 1.477811] GPR08: 00002400 00000001 09000000 00000000 c07b5d04 00000000 c00037d8 00000000 > [ 1.477811] GPR16: 00000000 00000000 00000000 00000000 c0760000 c0740000 00000092 c0685bb0 > [ 1.477811] GPR24: c065042c c068a734 c0685b8c 00000006 00000000 c0760000 c075c3c0 ffffffff > [ 1.512711] NIP [c066c550] test_debug_virtual_init+0x3c/0xd4 > [ 1.518315] LR [c0650ccc] do_one_initcall+0x8c/0x1cc > [ 1.523163] Call Trace: > [ 1.525595] [c900be60] [c0567340] 0xc0567340 (unreliable) > [ 1.530954] [c900be90] [c0650ccc] do_one_initcall+0x8c/0x1cc > [ 1.536551] [c900bef0] [c0651000] kernel_init_freeable+0x1f4/0x2cc > [ 1.542658] [c900bf30] [c00037ec] kernel_init+0x14/0x110 > [ 1.547913] [c900bf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c > [ 1.553971] Instruction dump: > [ 1.556909] 3ca50100 bfa10024 54a5000e 3fa0c076 7c0802a6 3d454000 813dc204 554893be > [ 1.564566] 7d294010 7d294910 90010034 39290001 <0f090000> 7c3e0b78 955e0008 3fe0c062 > [ 1.572425] ---[ end trace 6f6984225b280ad6 ]--- > [ 1.577467] PA: 0x09000000 for VA: 0xc9000000 > [ 1.581799] PA: 0x061e8f50 for VA: 0xc61e8f50 > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > v3: Added missing linux/mm.h > I realised that a driver may use DMA on stack after checking with virt_addr_valid(), so the new > verification might induce false positives. I remove it for now, will add it again later in a more > controled way. What is this comment referring to? I can't see any difference to v2 except the linux/mm.h include. cheers
Le 12/12/2018 à 01:23, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@c-s.fr> writes: > >> This patch implements CONFIG_DEBUG_VIRTUAL to warn about >> incorrect use of virt_to_phys() and page_to_phys() >> >> Below is the result of test_debug_virtual: >> >> [ 1.438746] WARNING: CPU: 0 PID: 1 at ./arch/powerpc/include/asm/io.h:808 test_debug_virtual_init+0x3c/0xd4 >> [ 1.448156] CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc5-00560-g6bfb52e23a00-dirty #532 >> [ 1.457259] NIP: c066c550 LR: c0650ccc CTR: c066c514 >> [ 1.462257] REGS: c900bdb0 TRAP: 0700 Not tainted (4.20.0-rc5-00560-g6bfb52e23a00-dirty) >> [ 1.471184] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 48000422 XER: 20000000 >> [ 1.477811] >> [ 1.477811] GPR00: c0650ccc c900be60 c60d0000 00000000 006000c0 c9000000 00009032 c7fa0020 >> [ 1.477811] GPR08: 00002400 00000001 09000000 00000000 c07b5d04 00000000 c00037d8 00000000 >> [ 1.477811] GPR16: 00000000 00000000 00000000 00000000 c0760000 c0740000 00000092 c0685bb0 >> [ 1.477811] GPR24: c065042c c068a734 c0685b8c 00000006 00000000 c0760000 c075c3c0 ffffffff >> [ 1.512711] NIP [c066c550] test_debug_virtual_init+0x3c/0xd4 >> [ 1.518315] LR [c0650ccc] do_one_initcall+0x8c/0x1cc >> [ 1.523163] Call Trace: >> [ 1.525595] [c900be60] [c0567340] 0xc0567340 (unreliable) >> [ 1.530954] [c900be90] [c0650ccc] do_one_initcall+0x8c/0x1cc >> [ 1.536551] [c900bef0] [c0651000] kernel_init_freeable+0x1f4/0x2cc >> [ 1.542658] [c900bf30] [c00037ec] kernel_init+0x14/0x110 >> [ 1.547913] [c900bf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c >> [ 1.553971] Instruction dump: >> [ 1.556909] 3ca50100 bfa10024 54a5000e 3fa0c076 7c0802a6 3d454000 813dc204 554893be >> [ 1.564566] 7d294010 7d294910 90010034 39290001 <0f090000> 7c3e0b78 955e0008 3fe0c062 >> [ 1.572425] ---[ end trace 6f6984225b280ad6 ]--- >> [ 1.577467] PA: 0x09000000 for VA: 0xc9000000 >> [ 1.581799] PA: 0x061e8f50 for VA: 0xc61e8f50 >> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> --- >> v3: Added missing linux/mm.h >> I realised that a driver may use DMA on stack after checking with virt_addr_valid(), so the new >> verification might induce false positives. I remove it for now, will add it again later in a more >> controled way. > > What is this comment referring to? > > I can't see any difference to v2 except the linux/mm.h include. v2 was: @@ -804,6 +806,11 @@ extern void __iounmap_at(void *ea, unsigned long size); */ static inline unsigned long virt_to_phys(volatile void * address) { + if (IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && + !WARN_ON(IS_ENABLED(CONFIG_HAVE_ARCH_VMAP_STACK) && current->pid && + object_is_on_stack((const void*)address))) + WARN_ON(!virt_addr_valid(address)); + return __pa((unsigned long)address); } v3 is: (same as v1) @@ -804,6 +806,8 @@ extern void __iounmap_at(void *ea, unsigned long size); */ static inline unsigned long virt_to_phys(volatile void * address) { + WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !virt_addr_valid(address)); + return __pa((unsigned long)address); } The idea in v2 was to detect objects on stack used for DMA before activating CONFIG_VMAP_STACK, but if the driver uses virt_addr_valid() to decide if it can DMA map it, then we'll get false positives. So I think this should be added with a dedicated DEBUG CONFIG option, not implicitely. Christophe
Christophe Leroy <christophe.leroy@c-s.fr> writes: > Le 12/12/2018 à 01:23, Michael Ellerman a écrit : >> Christophe Leroy <christophe.leroy@c-s.fr> writes: >> >>> This patch implements CONFIG_DEBUG_VIRTUAL to warn about >>> incorrect use of virt_to_phys() and page_to_phys() >>> >>> Below is the result of test_debug_virtual: >>> >>> [ 1.438746] WARNING: CPU: 0 PID: 1 at ./arch/powerpc/include/asm/io.h:808 test_debug_virtual_init+0x3c/0xd4 >>> [ 1.448156] CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc5-00560-g6bfb52e23a00-dirty #532 >>> [ 1.457259] NIP: c066c550 LR: c0650ccc CTR: c066c514 >>> [ 1.462257] REGS: c900bdb0 TRAP: 0700 Not tainted (4.20.0-rc5-00560-g6bfb52e23a00-dirty) >>> [ 1.471184] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 48000422 XER: 20000000 >>> [ 1.477811] >>> [ 1.477811] GPR00: c0650ccc c900be60 c60d0000 00000000 006000c0 c9000000 00009032 c7fa0020 >>> [ 1.477811] GPR08: 00002400 00000001 09000000 00000000 c07b5d04 00000000 c00037d8 00000000 >>> [ 1.477811] GPR16: 00000000 00000000 00000000 00000000 c0760000 c0740000 00000092 c0685bb0 >>> [ 1.477811] GPR24: c065042c c068a734 c0685b8c 00000006 00000000 c0760000 c075c3c0 ffffffff >>> [ 1.512711] NIP [c066c550] test_debug_virtual_init+0x3c/0xd4 >>> [ 1.518315] LR [c0650ccc] do_one_initcall+0x8c/0x1cc >>> [ 1.523163] Call Trace: >>> [ 1.525595] [c900be60] [c0567340] 0xc0567340 (unreliable) >>> [ 1.530954] [c900be90] [c0650ccc] do_one_initcall+0x8c/0x1cc >>> [ 1.536551] [c900bef0] [c0651000] kernel_init_freeable+0x1f4/0x2cc >>> [ 1.542658] [c900bf30] [c00037ec] kernel_init+0x14/0x110 >>> [ 1.547913] [c900bf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c >>> [ 1.553971] Instruction dump: >>> [ 1.556909] 3ca50100 bfa10024 54a5000e 3fa0c076 7c0802a6 3d454000 813dc204 554893be >>> [ 1.564566] 7d294010 7d294910 90010034 39290001 <0f090000> 7c3e0b78 955e0008 3fe0c062 >>> [ 1.572425] ---[ end trace 6f6984225b280ad6 ]--- >>> [ 1.577467] PA: 0x09000000 for VA: 0xc9000000 >>> [ 1.581799] PA: 0x061e8f50 for VA: 0xc61e8f50 >>> >>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >>> --- >>> v3: Added missing linux/mm.h >>> I realised that a driver may use DMA on stack after checking with virt_addr_valid(), so the new >>> verification might induce false positives. I remove it for now, will add it again later in a more >>> controled way. >> >> What is this comment referring to? >> >> I can't see any difference to v2 except the linux/mm.h include. > > v2 was: > > > @@ -804,6 +806,11 @@ extern void __iounmap_at(void *ea, unsigned long size); > */ > static inline unsigned long virt_to_phys(volatile void * address) > { > + if (IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && > + !WARN_ON(IS_ENABLED(CONFIG_HAVE_ARCH_VMAP_STACK) && current->pid && > + object_is_on_stack((const void*)address))) > + WARN_ON(!virt_addr_valid(address)); > + > return __pa((unsigned long)address); > } > > > v3 is: (same as v1) > > > @@ -804,6 +806,8 @@ extern void __iounmap_at(void *ea, unsigned long size); > */ > static inline unsigned long virt_to_phys(volatile void * address) > { > + WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !virt_addr_valid(address)); > + > return __pa((unsigned long)address); > } Right, sorry I must have been looking at v1 (which was already applied in my tree). > The idea in v2 was to detect objects on stack used for DMA before > activating CONFIG_VMAP_STACK, but if the driver uses virt_addr_valid() > to decide if it can DMA map it, then we'll get false positives. > So I think this should be added with a dedicated DEBUG CONFIG option, > not implicitely. Sounds good. I'll take v3. cheers
Christophe Leroy <christophe.leroy@c-s.fr> writes: > This patch implements CONFIG_DEBUG_VIRTUAL to warn about > incorrect use of virt_to_phys() and page_to_phys() This commit is breaking my p5020ds booting a 32-bit kernel with: smp: Bringing up secondary CPUs ... __ioremap(): phys addr 0x7fef5000 is RAM lr ioremap_coherent Unable to handle kernel paging request for data at address 0x00000000 Faulting instruction address: 0xc002e950 Oops: Kernel access of bad area, sig: 11 [#1] BE SMP NR_CPUS=24 CoreNet Generic Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9 #148 NIP: c002e950 LR: c002eb20 CTR: 00000001 REGS: e804bd20 TRAP: 0300 Not tainted (4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9) MSR: 00021002 <CE,ME> CR: 28004222 XER: 00000000 DEAR: 00000000 ESR: 00000000 GPR00: c002eb20 e804bdd0 e8050000 00000000 00021002 00000000 00000050 00021002 GPR08: 2d3f0000 00000001 00000000 00000004 24000842 00000000 c00026d0 00000000 GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000001 GPR24: 00029002 7fef5140 30000000 00000000 00000000 00000040 00000001 00000000 NIP [c002e950] smp_85xx_kick_cpu+0x120/0x410 LR [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 Call Trace: [e804bdd0] [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 (unreliable) [e804be20] [c0012e38] __cpu_up+0xc8/0x230 [e804be50] [c0040b34] bringup_cpu+0x34/0x110 [e804be70] [c00418a8] cpu_up+0x128/0x250 [e804beb0] [c0b84b14] smp_init+0xc4/0x10c [e804bee0] [c0b75c1c] kernel_init_freeable+0xc8/0x250 [e804bf20] [c00026e8] kernel_init+0x18/0x120 [e804bf40] [c0011298] ret_from_kernel_thread+0x14/0x1c Instruction dump: 7fb3e850 57bdd1be 2e1d0000 41d20250 57bd3032 393dffc0 7e6a9b78 5529d1be 39290001 7d2903a6 60000000 60000000 <7c0050ac> 394a0040 4200fff8 7c0004ac ---[ end trace edcab2a1dfd5b38c ]--- Which is obviously this hunk: > diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c > index 4fc77a99c9bf..68d204a45cd0 100644 > --- a/arch/powerpc/mm/pgtable_32.c > +++ b/arch/powerpc/mm/pgtable_32.c > @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call > * Don't allow anybody to remap normal RAM that we're using. > * mem_init() sets high_memory so only do the check after that. > */ > - if (slab_is_available() && (p < virt_to_phys(high_memory)) && > + if (slab_is_available() && virt_addr_valid(p) && > page_is_ram(__phys_to_pfn(p))) { > printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", > (unsigned long long)p, __builtin_return_address(0)); I'll try and come up with a fix tomorrow. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Christophe Leroy <christophe.leroy@c-s.fr> writes: > >> This patch implements CONFIG_DEBUG_VIRTUAL to warn about >> incorrect use of virt_to_phys() and page_to_phys() > > This commit is breaking my p5020ds booting a 32-bit kernel with: > > smp: Bringing up secondary CPUs ... > __ioremap(): phys addr 0x7fef5000 is RAM lr ioremap_coherent > Unable to handle kernel paging request for data at address 0x00000000 > Faulting instruction address: 0xc002e950 > Oops: Kernel access of bad area, sig: 11 [#1] > BE SMP NR_CPUS=24 CoreNet Generic > Modules linked in: > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9 #148 > NIP: c002e950 LR: c002eb20 CTR: 00000001 > REGS: e804bd20 TRAP: 0300 Not tainted (4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9) > MSR: 00021002 <CE,ME> CR: 28004222 XER: 00000000 > DEAR: 00000000 ESR: 00000000 > GPR00: c002eb20 e804bdd0 e8050000 00000000 00021002 00000000 00000050 00021002 > GPR08: 2d3f0000 00000001 00000000 00000004 24000842 00000000 c00026d0 00000000 > GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000001 > GPR24: 00029002 7fef5140 30000000 00000000 00000000 00000040 00000001 00000000 > NIP [c002e950] smp_85xx_kick_cpu+0x120/0x410 > LR [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 > Call Trace: > [e804bdd0] [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 (unreliable) > [e804be20] [c0012e38] __cpu_up+0xc8/0x230 > [e804be50] [c0040b34] bringup_cpu+0x34/0x110 > [e804be70] [c00418a8] cpu_up+0x128/0x250 > [e804beb0] [c0b84b14] smp_init+0xc4/0x10c > [e804bee0] [c0b75c1c] kernel_init_freeable+0xc8/0x250 > [e804bf20] [c00026e8] kernel_init+0x18/0x120 > [e804bf40] [c0011298] ret_from_kernel_thread+0x14/0x1c > Instruction dump: > 7fb3e850 57bdd1be 2e1d0000 41d20250 57bd3032 393dffc0 7e6a9b78 5529d1be > 39290001 7d2903a6 60000000 60000000 <7c0050ac> 394a0040 4200fff8 7c0004ac > ---[ end trace edcab2a1dfd5b38c ]--- > > > Which is obviously this hunk: > >> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c >> index 4fc77a99c9bf..68d204a45cd0 100644 >> --- a/arch/powerpc/mm/pgtable_32.c >> +++ b/arch/powerpc/mm/pgtable_32.c >> @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call >> * Don't allow anybody to remap normal RAM that we're using. >> * mem_init() sets high_memory so only do the check after that. >> */ >> - if (slab_is_available() && (p < virt_to_phys(high_memory)) && >> + if (slab_is_available() && virt_addr_valid(p) && >> page_is_ram(__phys_to_pfn(p))) { >> printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", >> (unsigned long long)p, __builtin_return_address(0)); > > > I'll try and come up with a fix tomorrow. Actually I think that change is just wrong. virt_addr_valid() takes a virtual address, but p is a physical address. So I'll drop this hunk for now, which makes the patch a no-op when DEBUG_VIRTUAL is n which is probably the way it should be. cheers
Le 19/12/2018 à 01:26, Michael Ellerman a écrit : > Michael Ellerman <mpe@ellerman.id.au> writes: >> Christophe Leroy <christophe.leroy@c-s.fr> writes: >> >>> This patch implements CONFIG_DEBUG_VIRTUAL to warn about >>> incorrect use of virt_to_phys() and page_to_phys() >> >> This commit is breaking my p5020ds booting a 32-bit kernel with: >> >> smp: Bringing up secondary CPUs ... >> __ioremap(): phys addr 0x7fef5000 is RAM lr ioremap_coherent >> Unable to handle kernel paging request for data at address 0x00000000 >> Faulting instruction address: 0xc002e950 >> Oops: Kernel access of bad area, sig: 11 [#1] >> BE SMP NR_CPUS=24 CoreNet Generic >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9 #148 >> NIP: c002e950 LR: c002eb20 CTR: 00000001 >> REGS: e804bd20 TRAP: 0300 Not tainted (4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9) >> MSR: 00021002 <CE,ME> CR: 28004222 XER: 00000000 >> DEAR: 00000000 ESR: 00000000 >> GPR00: c002eb20 e804bdd0 e8050000 00000000 00021002 00000000 00000050 00021002 >> GPR08: 2d3f0000 00000001 00000000 00000004 24000842 00000000 c00026d0 00000000 >> GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000001 >> GPR24: 00029002 7fef5140 30000000 00000000 00000000 00000040 00000001 00000000 >> NIP [c002e950] smp_85xx_kick_cpu+0x120/0x410 >> LR [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 >> Call Trace: >> [e804bdd0] [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 (unreliable) >> [e804be20] [c0012e38] __cpu_up+0xc8/0x230 >> [e804be50] [c0040b34] bringup_cpu+0x34/0x110 >> [e804be70] [c00418a8] cpu_up+0x128/0x250 >> [e804beb0] [c0b84b14] smp_init+0xc4/0x10c >> [e804bee0] [c0b75c1c] kernel_init_freeable+0xc8/0x250 >> [e804bf20] [c00026e8] kernel_init+0x18/0x120 >> [e804bf40] [c0011298] ret_from_kernel_thread+0x14/0x1c >> Instruction dump: >> 7fb3e850 57bdd1be 2e1d0000 41d20250 57bd3032 393dffc0 7e6a9b78 5529d1be >> 39290001 7d2903a6 60000000 60000000 <7c0050ac> 394a0040 4200fff8 7c0004ac >> ---[ end trace edcab2a1dfd5b38c ]--- >> >> >> Which is obviously this hunk: >> >>> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c >>> index 4fc77a99c9bf..68d204a45cd0 100644 >>> --- a/arch/powerpc/mm/pgtable_32.c >>> +++ b/arch/powerpc/mm/pgtable_32.c >>> @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call >>> * Don't allow anybody to remap normal RAM that we're using. >>> * mem_init() sets high_memory so only do the check after that. >>> */ >>> - if (slab_is_available() && (p < virt_to_phys(high_memory)) && >>> + if (slab_is_available() && virt_addr_valid(p) && >>> page_is_ram(__phys_to_pfn(p))) { >>> printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", >>> (unsigned long long)p, __builtin_return_address(0)); >> >> >> I'll try and come up with a fix tomorrow. > > Actually I think that change is just wrong. virt_addr_valid() takes a > virtual address, but p is a physical address. > > So I'll drop this hunk for now, which makes the patch a no-op when > DEBUG_VIRTUAL is n which is probably the way it should be. The hunk is obviously wrong for sure. Anyway there's a problem, most likely high_memory is not a valid virtual address, so without this hunk I get the following warning at every ioremap(): [ 0.000000] WARNING: CPU: 0 PID: 0 at ./arch/powerpc/include/asm/io.h:809 __ioremap_caller+0x9c/0x180 [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.20.0-rc6-s3k-dev-00677-g9c98dcab6203-dirty #615 [ 0.000000] NIP: c000fcd0 LR: c000fc64 CTR: 00000000 [ 0.000000] REGS: c073de50 TRAP: 0700 Not tainted (4.20.0-rc6-s3k-dev-00677-g9c98dcab6203-dirty) [ 0.000000] MSR: 00021032 <ME,IR,DR,RI> CR: 28944422 XER: 0000f940 [ 0.000000] [ 0.000000] GPR00: c000fe04 c073df00 c06e1450 00000001 00004023 c073df38 c0018f50 00000001 [ 0.000000] GPR08: 00002000 08000000 00002000 00000000 88944224 00600000 00000000 07ff9580 [ 0.000000] GPR16: 00000000 07ffb94c 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.000000] GPR24: 00000000 c0760000 0000019f ff000000 ff000000 c000fe04 00004000 c0018f50 [ 0.000000] NIP [c000fcd0] __ioremap_caller+0x9c/0x180 [ 0.000000] LR [c000fc64] __ioremap_caller+0x30/0x180 [ 0.000000] Call Trace: [ 0.000000] [c073df00] [c02fc23c] of_address_to_resource+0x114/0x154 (unreliable) [ 0.000000] [c073df30] [c000fe04] ioremap_wt+0x20/0x30 [ 0.000000] [c073df40] [c0018f50] mpc8xx_pic_init+0x70/0xf8 [ 0.000000] [c073df80] [c0655b84] mpc8xx_pics_init+0x10/0x6c [ 0.000000] [c073df90] [c0675080] cmpc885_pics_init+0x14/0x118 [ 0.000000] [c073dfa0] [c0652eb0] init_IRQ+0x24/0x38 [ 0.000000] [c073dfb0] [c0650b10] start_kernel+0x2a8/0x3d4 [ 0.000000] [c073dff0] [c0002258] start_here+0x44/0x98 [ 0.000000] Instruction dump: [ 0.000000] 419e00b8 7f83e378 480013fd 7c7d1b79 41820030 576304be 7c63ea14 80010034 [ 0.000000] bb410018 7c0803a6 38210030 4e800020 <0fe00000> 7f9c4840 409cffc4 480000a8 [ 0.000000] random: get_random_bytes called from print_oops_end_marker+0x60/0x84 with crng_init=0 [ 0.000000] ---[ end trace 25d7f28ce013ad37 ]--- I'll try and come with solution during the day. Christophe > > cheers >
On 12/19/2018 06:57 AM, Christophe Leroy wrote: > > > Le 19/12/2018 à 01:26, Michael Ellerman a écrit : >> Michael Ellerman <mpe@ellerman.id.au> writes: >>> Christophe Leroy <christophe.leroy@c-s.fr> writes: >>> >>>> This patch implements CONFIG_DEBUG_VIRTUAL to warn about >>>> incorrect use of virt_to_phys() and page_to_phys() >>> >>> This commit is breaking my p5020ds booting a 32-bit kernel with: >>> >>> smp: Bringing up secondary CPUs ... >>> __ioremap(): phys addr 0x7fef5000 is RAM lr ioremap_coherent >>> Unable to handle kernel paging request for data at address 0x00000000 >>> Faulting instruction address: 0xc002e950 >>> Oops: Kernel access of bad area, sig: 11 [#1] >>> BE SMP NR_CPUS=24 CoreNet Generic >>> Modules linked in: >>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted >>> 4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9 #148 >>> NIP: c002e950 LR: c002eb20 CTR: 00000001 >>> REGS: e804bd20 TRAP: 0300 Not tainted >>> (4.20.0-rc2-gcc-7.0.1-00138-g9a0380d299e9) >>> MSR: 00021002 <CE,ME> CR: 28004222 XER: 00000000 >>> DEAR: 00000000 ESR: 00000000 >>> GPR00: c002eb20 e804bdd0 e8050000 00000000 00021002 00000000 >>> 00000050 00021002 >>> GPR08: 2d3f0000 00000001 00000000 00000004 24000842 00000000 >>> c00026d0 00000000 >>> GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 >>> 00000000 00000001 >>> GPR24: 00029002 7fef5140 30000000 00000000 00000000 00000040 >>> 00000001 00000000 >>> NIP [c002e950] smp_85xx_kick_cpu+0x120/0x410 >>> LR [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 >>> Call Trace: >>> [e804bdd0] [c002eb20] smp_85xx_kick_cpu+0x2f0/0x410 (unreliable) >>> [e804be20] [c0012e38] __cpu_up+0xc8/0x230 >>> [e804be50] [c0040b34] bringup_cpu+0x34/0x110 >>> [e804be70] [c00418a8] cpu_up+0x128/0x250 >>> [e804beb0] [c0b84b14] smp_init+0xc4/0x10c >>> [e804bee0] [c0b75c1c] kernel_init_freeable+0xc8/0x250 >>> [e804bf20] [c00026e8] kernel_init+0x18/0x120 >>> [e804bf40] [c0011298] ret_from_kernel_thread+0x14/0x1c >>> Instruction dump: >>> 7fb3e850 57bdd1be 2e1d0000 41d20250 57bd3032 393dffc0 7e6a9b78 >>> 5529d1be >>> 39290001 7d2903a6 60000000 60000000 <7c0050ac> 394a0040 4200fff8 >>> 7c0004ac >>> ---[ end trace edcab2a1dfd5b38c ]--- >>> >>> >>> Which is obviously this hunk: >>> >>>> diff --git a/arch/powerpc/mm/pgtable_32.c >>>> b/arch/powerpc/mm/pgtable_32.c >>>> index 4fc77a99c9bf..68d204a45cd0 100644 >>>> --- a/arch/powerpc/mm/pgtable_32.c >>>> +++ b/arch/powerpc/mm/pgtable_32.c >>>> @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long >>>> size, pgprot_t prot, void *call >>>> * Don't allow anybody to remap normal RAM that we're using. >>>> * mem_init() sets high_memory so only do the check after that. >>>> */ >>>> - if (slab_is_available() && (p < virt_to_phys(high_memory)) && >>>> + if (slab_is_available() && virt_addr_valid(p) && >>>> page_is_ram(__phys_to_pfn(p))) { >>>> printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", >>>> (unsigned long long)p, __builtin_return_address(0)); >>> >>> >>> I'll try and come up with a fix tomorrow. >> >> Actually I think that change is just wrong. virt_addr_valid() takes a >> virtual address, but p is a physical address. >> >> So I'll drop this hunk for now, which makes the patch a no-op when >> DEBUG_VIRTUAL is n which is probably the way it should be. > > The hunk is obviously wrong for sure. Anyway there's a problem, most > likely high_memory is not a valid virtual address, so without this hunk > I get the following warning at every ioremap(): > > [ 0.000000] WARNING: CPU: 0 PID: 0 at > ./arch/powerpc/include/asm/io.h:809 __ioremap_caller+0x9c/0x180 > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted > 4.20.0-rc6-s3k-dev-00677-g9c98dcab6203-dirty #615 > [ 0.000000] NIP: c000fcd0 LR: c000fc64 CTR: 00000000 > [ 0.000000] REGS: c073de50 TRAP: 0700 Not tainted > (4.20.0-rc6-s3k-dev-00677-g9c98dcab6203-dirty) > [ 0.000000] MSR: 00021032 <ME,IR,DR,RI> CR: 28944422 XER: 0000f940 > [ 0.000000] > [ 0.000000] GPR00: c000fe04 c073df00 c06e1450 00000001 00004023 > c073df38 c0018f50 00000001 > [ 0.000000] GPR08: 00002000 08000000 00002000 00000000 88944224 > 00600000 00000000 07ff9580 > [ 0.000000] GPR16: 00000000 07ffb94c 00000000 00000000 00000000 > 00000000 00000000 00000000 > [ 0.000000] GPR24: 00000000 c0760000 0000019f ff000000 ff000000 > c000fe04 00004000 c0018f50 > [ 0.000000] NIP [c000fcd0] __ioremap_caller+0x9c/0x180 > [ 0.000000] LR [c000fc64] __ioremap_caller+0x30/0x180 > [ 0.000000] Call Trace: > [ 0.000000] [c073df00] [c02fc23c] of_address_to_resource+0x114/0x154 > (unreliable) > [ 0.000000] [c073df30] [c000fe04] ioremap_wt+0x20/0x30 > [ 0.000000] [c073df40] [c0018f50] mpc8xx_pic_init+0x70/0xf8 > [ 0.000000] [c073df80] [c0655b84] mpc8xx_pics_init+0x10/0x6c > [ 0.000000] [c073df90] [c0675080] cmpc885_pics_init+0x14/0x118 > [ 0.000000] [c073dfa0] [c0652eb0] init_IRQ+0x24/0x38 > [ 0.000000] [c073dfb0] [c0650b10] start_kernel+0x2a8/0x3d4 > [ 0.000000] [c073dff0] [c0002258] start_here+0x44/0x98 > [ 0.000000] Instruction dump: > [ 0.000000] 419e00b8 7f83e378 480013fd 7c7d1b79 41820030 576304be > 7c63ea14 80010034 > [ 0.000000] bb410018 7c0803a6 38210030 4e800020 <0fe00000> 7f9c4840 > 409cffc4 480000a8 > [ 0.000000] random: get_random_bytes called from > print_oops_end_marker+0x60/0x84 with crng_init=0 > [ 0.000000] ---[ end trace 25d7f28ce013ad37 ]--- > > I'll try and come with solution during the day. In fact the solution is the following: diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c index 4fc77a99c9bf..60401af2bc8f 100644 --- a/arch/powerpc/mm/pgtable_32.c +++ b/arch/powerpc/mm/pgtable_32.c @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call * Don't allow anybody to remap normal RAM that we're using. * mem_init() sets high_memory so only do the check after that. */ - if (slab_is_available() && (p < virt_to_phys(high_memory)) && + if (slab_is_available() && (p <= virt_to_phys(high_memory - 1)) && page_is_ram(__phys_to_pfn(p))) { printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", (unsigned long long)p, __builtin_return_address(0)); I'll send an updated patch in a few minutes. Christophe
Christophe Leroy <christophe.leroy@c-s.fr> writes: > On 12/19/2018 06:57 AM, Christophe Leroy wrote: ... > > In fact the solution is the following: > > diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c > index 4fc77a99c9bf..60401af2bc8f 100644 > --- a/arch/powerpc/mm/pgtable_32.c > +++ b/arch/powerpc/mm/pgtable_32.c > @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long > size, pgprot_t prot, void *call > * Don't allow anybody to remap normal RAM that we're using. > * mem_init() sets high_memory so only do the check after that. > */ > - if (slab_is_available() && (p < virt_to_phys(high_memory)) && > + if (slab_is_available() && (p <= virt_to_phys(high_memory - 1)) && > page_is_ram(__phys_to_pfn(p))) { > printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", > (unsigned long long)p, __builtin_return_address(0)); > > > I'll send an updated patch in a few minutes. Awesome, thanks. I'll take v4. cheers
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index e312e92e3381..94b46624068d 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -128,6 +128,7 @@ config PPC # # Please keep this list sorted alphabetically. # + select ARCH_HAS_DEBUG_VIRTUAL select ARCH_HAS_DEVMEM_IS_ALLOWED select ARCH_HAS_DMA_SET_COHERENT_MASK select ARCH_HAS_ELF_RANDOMIZE diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index e746becd9d6f..7f19fbd3ba55 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -29,12 +29,14 @@ extern struct pci_dev *isa_bridge_pcidev; #include <linux/device.h> #include <linux/compiler.h> +#include <linux/mm.h> #include <asm/page.h> #include <asm/byteorder.h> #include <asm/synch.h> #include <asm/delay.h> #include <asm/mmu.h> #include <asm/ppc_asm.h> +#include <asm/pgtable.h> #ifdef CONFIG_PPC64 #include <asm/paca.h> @@ -804,6 +806,8 @@ extern void __iounmap_at(void *ea, unsigned long size); */ static inline unsigned long virt_to_phys(volatile void * address) { + WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !virt_addr_valid(address)); + return __pa((unsigned long)address); } @@ -827,7 +831,14 @@ static inline void * phys_to_virt(unsigned long address) /* * Change "struct page" to physical address. */ -#define page_to_phys(page) ((phys_addr_t)page_to_pfn(page) << PAGE_SHIFT) +static inline phys_addr_t page_to_phys(struct page *page) +{ + unsigned long pfn = page_to_pfn(page); + + WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !pfn_valid(pfn)); + + return PFN_PHYS(pfn); +} /* * 32 bits still uses virt_to_bus() for it's implementation of DMA diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c index 4fc77a99c9bf..68d204a45cd0 100644 --- a/arch/powerpc/mm/pgtable_32.c +++ b/arch/powerpc/mm/pgtable_32.c @@ -143,7 +143,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call * Don't allow anybody to remap normal RAM that we're using. * mem_init() sets high_memory so only do the check after that. */ - if (slab_is_available() && (p < virt_to_phys(high_memory)) && + if (slab_is_available() && virt_addr_valid(p) && page_is_ram(__phys_to_pfn(p))) { printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n", (unsigned long long)p, __builtin_return_address(0));
This patch implements CONFIG_DEBUG_VIRTUAL to warn about incorrect use of virt_to_phys() and page_to_phys() Below is the result of test_debug_virtual: [ 1.438746] WARNING: CPU: 0 PID: 1 at ./arch/powerpc/include/asm/io.h:808 test_debug_virtual_init+0x3c/0xd4 [ 1.448156] CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc5-00560-g6bfb52e23a00-dirty #532 [ 1.457259] NIP: c066c550 LR: c0650ccc CTR: c066c514 [ 1.462257] REGS: c900bdb0 TRAP: 0700 Not tainted (4.20.0-rc5-00560-g6bfb52e23a00-dirty) [ 1.471184] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 48000422 XER: 20000000 [ 1.477811] [ 1.477811] GPR00: c0650ccc c900be60 c60d0000 00000000 006000c0 c9000000 00009032 c7fa0020 [ 1.477811] GPR08: 00002400 00000001 09000000 00000000 c07b5d04 00000000 c00037d8 00000000 [ 1.477811] GPR16: 00000000 00000000 00000000 00000000 c0760000 c0740000 00000092 c0685bb0 [ 1.477811] GPR24: c065042c c068a734 c0685b8c 00000006 00000000 c0760000 c075c3c0 ffffffff [ 1.512711] NIP [c066c550] test_debug_virtual_init+0x3c/0xd4 [ 1.518315] LR [c0650ccc] do_one_initcall+0x8c/0x1cc [ 1.523163] Call Trace: [ 1.525595] [c900be60] [c0567340] 0xc0567340 (unreliable) [ 1.530954] [c900be90] [c0650ccc] do_one_initcall+0x8c/0x1cc [ 1.536551] [c900bef0] [c0651000] kernel_init_freeable+0x1f4/0x2cc [ 1.542658] [c900bf30] [c00037ec] kernel_init+0x14/0x110 [ 1.547913] [c900bf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c [ 1.553971] Instruction dump: [ 1.556909] 3ca50100 bfa10024 54a5000e 3fa0c076 7c0802a6 3d454000 813dc204 554893be [ 1.564566] 7d294010 7d294910 90010034 39290001 <0f090000> 7c3e0b78 955e0008 3fe0c062 [ 1.572425] ---[ end trace 6f6984225b280ad6 ]--- [ 1.577467] PA: 0x09000000 for VA: 0xc9000000 [ 1.581799] PA: 0x061e8f50 for VA: 0xc61e8f50 Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- v3: Added missing linux/mm.h I realised that a driver may use DMA on stack after checking with virt_addr_valid(), so the new verification might induce false positives. I remove it for now, will add it again later in a more controled way. v2: Using asm/pgtable.h to avoid build failure on ppc64e. Added a verification that the object is not in stack to catch problems before activing VMAP_STACK. arch/powerpc/Kconfig | 1 + arch/powerpc/include/asm/io.h | 13 ++++++++++++- arch/powerpc/mm/pgtable_32.c | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-)