| Message ID | 20241202170359.1475019-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: patching: avoid early page_to_phys() | expand |
Looks like I messed up my text editing and left some bonus words... On Mon, Dec 02, 2024 at 05:03:59PM +0000, Mark Rutland wrote: > When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is > printed from the patching code because patch_map(), e.g. That was meant to say: | When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is | printed from the patching code, e.g. [...] > For historical reasons, the structure of patch_map() is more complicated > than necessary and can be simplified. For kernel image addresses it's > sufficient to use __pa_symbol() directly without converting this to a > page address and back. Aside from kernel image addresses, all executable > code should be allocated from execmem (where all allocations will fall > within the vmalloc area), and the vmalloc area), and so there's no need > for the fallback case when case when CONFIG_EXECMEM=n. That last sentence should have been: | Aside from kernel image addresses, all executable code should be | allocated from execmem (where all allocations will fall within the | vmalloc area), and so there's no need for the fallback case when case | when CONFIG_EXECMEM=n. I can spin a v2 with those fixed if necessary. Mark.
On Mon, Dec 02, 2024 at 05:03:59PM +0000, Mark Rutland wrote: > + phys_addr_t phys; > + > + if (is_image_text((unsigned long)addr)) { > + phys = __pa_symbol(addr); > + } else { > + struct page *page = vmalloc_to_page(addr); > + BUG_ON(!page); > + phys = page_to_phys(page) + offset_in_page(addr); > + } > + > + return (void *)set_fixmap_offset(fixmap, phys); This looks much better. Btw, I recently open coded the vmalloc to phys logic above in an unrelated fix, and noticed that arch/powerpc/mm/pgtable.c has a nice vmalloc_to_phys that could be generalized and move to common code. Does anyone have an opinion on that?
On Tue, Dec 03, 2024 at 01:46:11AM +0100, Christoph Hellwig wrote: > On Mon, Dec 02, 2024 at 05:03:59PM +0000, Mark Rutland wrote: > > + phys_addr_t phys; > > + > > + if (is_image_text((unsigned long)addr)) { > > + phys = __pa_symbol(addr); > > + } else { > > + struct page *page = vmalloc_to_page(addr); > > + BUG_ON(!page); > > + phys = page_to_phys(page) + offset_in_page(addr); > > + } > > + > > + return (void *)set_fixmap_offset(fixmap, phys); > > This looks much better. Btw, I recently open coded the vmalloc to phys > logic above in an unrelated fix, and noticed that > arch/powerpc/mm/pgtable.c has a nice vmalloc_to_phys that could > be generalized and move to common code. Does anyone have an opinion > on that? Maybe fold that check for !page into vmalloc_to_pfn() and make vmalloc_to_phys() a static inline?
On Mon, Dec 02, 2024 at 05:45:11PM +0000, Mark Rutland wrote: > Looks like I messed up my text editing and left some bonus words... > > On Mon, Dec 02, 2024 at 05:03:59PM +0000, Mark Rutland wrote: > > When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is > > printed from the patching code because patch_map(), e.g. > > That was meant to say: > > | When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is > | printed from the patching code, e.g. > > [...] > > > For historical reasons, the structure of patch_map() is more complicated > > than necessary and can be simplified. For kernel image addresses it's > > sufficient to use __pa_symbol() directly without converting this to a > > page address and back. Aside from kernel image addresses, all executable > > code should be allocated from execmem (where all allocations will fall > > within the vmalloc area), and the vmalloc area), and so there's no need > > for the fallback case when case when CONFIG_EXECMEM=n. > > That last sentence should have been: > > | Aside from kernel image addresses, all executable code should be > | allocated from execmem (where all allocations will fall within the > | vmalloc area), and so there's no need for the fallback case when case > | when CONFIG_EXECMEM=n. Still an extra "case when" ;-) > I can spin a v2 with those fixed if necessary. > > Mark.
On Mon, Dec 02, 2024 at 05:03:59PM +0000, Mark Rutland wrote: > When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is > printed from the patching code because patch_map(), e.g. > > | ------------[ cut here ]------------ > | WARNING: CPU: 0 PID: 0 at arch/arm64/kernel/patching.c:45 patch_map.constprop.0+0x120/0xd00 > | CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.13.0-rc1-00002-ge1a5d6c6be55 #1 > | Hardware name: linux,dummy-virt (DT) > | pstate: 800003c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > | pc : patch_map.constprop.0+0x120/0xd00 > | lr : patch_map.constprop.0+0x120/0xd00 > | sp : ffffa9bb312a79a0 > | x29: ffffa9bb312a79a0 x28: 0000000000000001 x27: 0000000000000001 > | x26: 0000000000000000 x25: 0000000000000000 x24: 00000000000402e8 > | x23: ffffa9bb2c94c1c8 x22: ffffa9bb2c94c000 x21: ffffa9bb222e883c > | x20: 0000000000000002 x19: ffffc1ffc100ba40 x18: ffffa9bb2cf0f21c > | x17: 0000000000000006 x16: 0000000000000000 x15: 0000000000000004 > | x14: 1ffff5376625b4ac x13: ffff753766a67fb8 x12: ffff753766919cd1 > | x11: 0000000000000003 x10: 1ffff5376625b4c3 x9 : 1ffff5376625b4af > | x8 : ffff753766254f0a x7 : 0000000041b58ab3 x6 : ffff753766254f18 > | x5 : ffffa9bb312d9bc0 x4 : 0000000000000000 x3 : ffffa9bb29bd90e4 > | x2 : 0000000000000002 x1 : ffffa9bb312d9bc0 x0 : 0000000000000000 > | Call trace: > | patch_map.constprop.0+0x120/0xd00 (P) > | patch_map.constprop.0+0x120/0xd00 (L) > | __aarch64_insn_write+0xa8/0x120 > | aarch64_insn_patch_text_nosync+0x4c/0xb8 > | arch_jump_label_transform_queue+0x7c/0x100 > | jump_label_update+0x154/0x460 > | static_key_enable_cpuslocked+0x1d8/0x280 > | static_key_enable+0x2c/0x48 > | early_randomize_kstack_offset+0x104/0x168 > | do_early_param+0xe4/0x148 > | parse_args+0x3a4/0x838 > | parse_early_options+0x50/0x68 > | parse_early_param+0x58/0xe0 > | setup_arch+0x78/0x1f0 > | start_kernel+0xa0/0x530 > | __primary_switched+0x8c/0xa0 > | irq event stamp: 0 > | hardirqs last enabled at (0): [<0000000000000000>] 0x0 > | hardirqs last disabled at (0): [<0000000000000000>] 0x0 > | softirqs last enabled at (0): [<0000000000000000>] 0x0 > | softirqs last disabled at (0): [<0000000000000000>] 0x0 > | ---[ end trace 0000000000000000 ]--- > > The warning has been produced since commit: > > 3e25d5a49f99b75b ("asm-generic: add an optional pfn_valid check to page_to_phys") > > ... which added a pfn_valid() check into page_to_phys(), and at this > point in boot pfn_valid() will always return false because the vmemmap > has not yet been initialized and there are no valid mem_sections yet. > > Before that commit, the arithmetic performed by page_to_phys() would > give the expected physical address, though it is somewhat dubious to use > vmemmap addresses before the vmemmap has been initialized. > > For historical reasons, the structure of patch_map() is more complicated > than necessary and can be simplified. For kernel image addresses it's > sufficient to use __pa_symbol() directly without converting this to a > page address and back. Aside from kernel image addresses, all executable > code should be allocated from execmem (where all allocations will fall > within the vmalloc area), and the vmalloc area), and so there's no need > for the fallback case when case when CONFIG_EXECMEM=n. > > Simplify patch_map() accordingly, directly converting kernel image > addresses and removing the redundant fallback case. > > Fixes: 3e25d5a49f99b75b ("asm-generic: add an optional pfn_valid check to page_to_phys") > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Mike Rapoport <rppt@kernel.org> > Cc: Thomas Huth <thuth@redhat.com> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/patching.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > Catalin, Will, I wasn't sure whether we'd prefer this or an explicit > check that the address is a vmalloc addr, e.g. > > if (is_image_text(...)) { > phys = ...; > } else if (is_vmalloc_addr(...)) { > phys = ...; > } else { > BUG(); > } > > I went for the style below because it was marginally simpler, I'm more > than happy to respin as above if that's preferable. > > Mark. > > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c > index 7f99723fbb8c4..1041bc67a3eee 100644 > --- a/arch/arm64/kernel/patching.c > +++ b/arch/arm64/kernel/patching.c > @@ -30,20 +30,17 @@ static bool is_image_text(unsigned long addr) > > static void __kprobes *patch_map(void *addr, int fixmap) > { > - unsigned long uintaddr = (uintptr_t) addr; > - bool image = is_image_text(uintaddr); > - struct page *page; > - > - if (image) > - page = phys_to_page(__pa_symbol(addr)); > - else if (IS_ENABLED(CONFIG_EXECMEM)) > - page = vmalloc_to_page(addr); > - else > - return addr; > - > - BUG_ON(!page); > - return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > - (uintaddr & ~PAGE_MASK)); > + phys_addr_t phys; > + > + if (is_image_text((unsigned long)addr)) { > + phys = __pa_symbol(addr); > + } else { > + struct page *page = vmalloc_to_page(addr); > + BUG_ON(!page); Not strictly related to this patch, but is it necessary to BUG() here? Can't patch_map() return NULL and fail the patching more gracefully? > + phys = page_to_phys(page) + offset_in_page(addr); > + } > + > + return (void *)set_fixmap_offset(fixmap, phys); > } > > static void __kprobes patch_unmap(int fixmap) > -- > 2.30.2 >
On Mon, 02 Dec 2024 17:03:59 +0000, Mark Rutland wrote: > When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is > printed from the patching code because patch_map(), e.g. > > | ------------[ cut here ]------------ > | WARNING: CPU: 0 PID: 0 at arch/arm64/kernel/patching.c:45 patch_map.constprop.0+0x120/0xd00 > | CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.13.0-rc1-00002-ge1a5d6c6be55 #1 > | Hardware name: linux,dummy-virt (DT) > | pstate: 800003c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > | pc : patch_map.constprop.0+0x120/0xd00 > | lr : patch_map.constprop.0+0x120/0xd00 > | sp : ffffa9bb312a79a0 > | x29: ffffa9bb312a79a0 x28: 0000000000000001 x27: 0000000000000001 > | x26: 0000000000000000 x25: 0000000000000000 x24: 00000000000402e8 > | x23: ffffa9bb2c94c1c8 x22: ffffa9bb2c94c000 x21: ffffa9bb222e883c > | x20: 0000000000000002 x19: ffffc1ffc100ba40 x18: ffffa9bb2cf0f21c > | x17: 0000000000000006 x16: 0000000000000000 x15: 0000000000000004 > | x14: 1ffff5376625b4ac x13: ffff753766a67fb8 x12: ffff753766919cd1 > | x11: 0000000000000003 x10: 1ffff5376625b4c3 x9 : 1ffff5376625b4af > | x8 : ffff753766254f0a x7 : 0000000041b58ab3 x6 : ffff753766254f18 > | x5 : ffffa9bb312d9bc0 x4 : 0000000000000000 x3 : ffffa9bb29bd90e4 > | x2 : 0000000000000002 x1 : ffffa9bb312d9bc0 x0 : 0000000000000000 > | Call trace: > | patch_map.constprop.0+0x120/0xd00 (P) > | patch_map.constprop.0+0x120/0xd00 (L) > | __aarch64_insn_write+0xa8/0x120 > | aarch64_insn_patch_text_nosync+0x4c/0xb8 > | arch_jump_label_transform_queue+0x7c/0x100 > | jump_label_update+0x154/0x460 > | static_key_enable_cpuslocked+0x1d8/0x280 > | static_key_enable+0x2c/0x48 > | early_randomize_kstack_offset+0x104/0x168 > | do_early_param+0xe4/0x148 > | parse_args+0x3a4/0x838 > | parse_early_options+0x50/0x68 > | parse_early_param+0x58/0xe0 > | setup_arch+0x78/0x1f0 > | start_kernel+0xa0/0x530 > | __primary_switched+0x8c/0xa0 > | irq event stamp: 0 > | hardirqs last enabled at (0): [<0000000000000000>] 0x0 > | hardirqs last disabled at (0): [<0000000000000000>] 0x0 > | softirqs last enabled at (0): [<0000000000000000>] 0x0 > | softirqs last disabled at (0): [<0000000000000000>] 0x0 > | ---[ end trace 0000000000000000 ]--- > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: patching: avoid early page_to_phys() https://git.kernel.org/arm64/c/8d09e2d569f6
diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c index 7f99723fbb8c4..1041bc67a3eee 100644 --- a/arch/arm64/kernel/patching.c +++ b/arch/arm64/kernel/patching.c @@ -30,20 +30,17 @@ static bool is_image_text(unsigned long addr) static void __kprobes *patch_map(void *addr, int fixmap) { - unsigned long uintaddr = (uintptr_t) addr; - bool image = is_image_text(uintaddr); - struct page *page; - - if (image) - page = phys_to_page(__pa_symbol(addr)); - else if (IS_ENABLED(CONFIG_EXECMEM)) - page = vmalloc_to_page(addr); - else - return addr; - - BUG_ON(!page); - return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + - (uintaddr & ~PAGE_MASK)); + phys_addr_t phys; + + if (is_image_text((unsigned long)addr)) { + phys = __pa_symbol(addr); + } else { + struct page *page = vmalloc_to_page(addr); + BUG_ON(!page); + phys = page_to_phys(page) + offset_in_page(addr); + } + + return (void *)set_fixmap_offset(fixmap, phys); } static void __kprobes patch_unmap(int fixmap)
When arm64 is configured with CONFIG_DEBUG_VIRTUAL=y, a warning is printed from the patching code because patch_map(), e.g. | ------------[ cut here ]------------ | WARNING: CPU: 0 PID: 0 at arch/arm64/kernel/patching.c:45 patch_map.constprop.0+0x120/0xd00 | CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.13.0-rc1-00002-ge1a5d6c6be55 #1 | Hardware name: linux,dummy-virt (DT) | pstate: 800003c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) | pc : patch_map.constprop.0+0x120/0xd00 | lr : patch_map.constprop.0+0x120/0xd00 | sp : ffffa9bb312a79a0 | x29: ffffa9bb312a79a0 x28: 0000000000000001 x27: 0000000000000001 | x26: 0000000000000000 x25: 0000000000000000 x24: 00000000000402e8 | x23: ffffa9bb2c94c1c8 x22: ffffa9bb2c94c000 x21: ffffa9bb222e883c | x20: 0000000000000002 x19: ffffc1ffc100ba40 x18: ffffa9bb2cf0f21c | x17: 0000000000000006 x16: 0000000000000000 x15: 0000000000000004 | x14: 1ffff5376625b4ac x13: ffff753766a67fb8 x12: ffff753766919cd1 | x11: 0000000000000003 x10: 1ffff5376625b4c3 x9 : 1ffff5376625b4af | x8 : ffff753766254f0a x7 : 0000000041b58ab3 x6 : ffff753766254f18 | x5 : ffffa9bb312d9bc0 x4 : 0000000000000000 x3 : ffffa9bb29bd90e4 | x2 : 0000000000000002 x1 : ffffa9bb312d9bc0 x0 : 0000000000000000 | Call trace: | patch_map.constprop.0+0x120/0xd00 (P) | patch_map.constprop.0+0x120/0xd00 (L) | __aarch64_insn_write+0xa8/0x120 | aarch64_insn_patch_text_nosync+0x4c/0xb8 | arch_jump_label_transform_queue+0x7c/0x100 | jump_label_update+0x154/0x460 | static_key_enable_cpuslocked+0x1d8/0x280 | static_key_enable+0x2c/0x48 | early_randomize_kstack_offset+0x104/0x168 | do_early_param+0xe4/0x148 | parse_args+0x3a4/0x838 | parse_early_options+0x50/0x68 | parse_early_param+0x58/0xe0 | setup_arch+0x78/0x1f0 | start_kernel+0xa0/0x530 | __primary_switched+0x8c/0xa0 | irq event stamp: 0 | hardirqs last enabled at (0): [<0000000000000000>] 0x0 | hardirqs last disabled at (0): [<0000000000000000>] 0x0 | softirqs last enabled at (0): [<0000000000000000>] 0x0 | softirqs last disabled at (0): [<0000000000000000>] 0x0 | ---[ end trace 0000000000000000 ]--- The warning has been produced since commit: 3e25d5a49f99b75b ("asm-generic: add an optional pfn_valid check to page_to_phys") ... which added a pfn_valid() check into page_to_phys(), and at this point in boot pfn_valid() will always return false because the vmemmap has not yet been initialized and there are no valid mem_sections yet. Before that commit, the arithmetic performed by page_to_phys() would give the expected physical address, though it is somewhat dubious to use vmemmap addresses before the vmemmap has been initialized. For historical reasons, the structure of patch_map() is more complicated than necessary and can be simplified. For kernel image addresses it's sufficient to use __pa_symbol() directly without converting this to a page address and back. Aside from kernel image addresses, all executable code should be allocated from execmem (where all allocations will fall within the vmalloc area), and the vmalloc area), and so there's no need for the fallback case when case when CONFIG_EXECMEM=n. Simplify patch_map() accordingly, directly converting kernel image addresses and removing the redundant fallback case. Fixes: 3e25d5a49f99b75b ("asm-generic: add an optional pfn_valid check to page_to_phys") Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Mike Rapoport <rppt@kernel.org> Cc: Thomas Huth <thuth@redhat.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kernel/patching.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) Catalin, Will, I wasn't sure whether we'd prefer this or an explicit check that the address is a vmalloc addr, e.g. if (is_image_text(...)) { phys = ...; } else if (is_vmalloc_addr(...)) { phys = ...; } else { BUG(); } I went for the style below because it was marginally simpler, I'm more than happy to respin as above if that's preferable. Mark.