| Message ID | 1476311522-15381-2-git-send-email-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote: > > ptdump_register currently initializes a set of page table information and > registers debugfs. There are uses for the ptdump option without wanting the > debugfs options. Split this out to make it a separate option. > > Signed-off-by: Laura Abbott <labbott@redhat.com> > --- > v2: Minor style fixups per Mark Rutland, intialization is now separate from > register since it never needed to be combined in the first place, EFI > page table registration. > --- > arch/arm64/Kconfig.debug | 6 +++++- > arch/arm64/include/asm/ptdump.h | 13 ++++++++----- > arch/arm64/mm/Makefile | 3 ++- > arch/arm64/mm/dump.c | 26 +++++--------------------- > arch/arm64/mm/ptdump_debugfs.c | 31 +++++++++++++++++++++++++++++++ > drivers/firmware/efi/arm-runtime.c | 5 ++--- > 6 files changed, 53 insertions(+), 31 deletions(-) > create mode 100644 arch/arm64/mm/ptdump_debugfs.c > > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug > index b661fe7..21a5b74 100644 > --- a/arch/arm64/Kconfig.debug > +++ b/arch/arm64/Kconfig.debug > @@ -2,9 +2,13 @@ menu "Kernel hacking" > > source "lib/Kconfig.debug" > > -config ARM64_PTDUMP > +config ARM64_PTDUMP_CORE > + def_bool n > + > +config ARM64_PTDUMP_DEBUGFS > bool "Export kernel pagetable layout to userspace via debugfs" I think this can be a tristate now, yes? (I did this on x86 so I could load ptdump as a module for Chrome OS image testing but then leave the module off the read-only partition for release images...) https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a -Kees > depends on DEBUG_KERNEL > + select ARM64_PTDUMP_CORE > select DEBUG_FS > help > Say Y here if you want to show the kernel pagetable layout in a
On 10/12/2016 03:45 PM, Kees Cook wrote: > On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote: >> >> ptdump_register currently initializes a set of page table information and >> registers debugfs. There are uses for the ptdump option without wanting the >> debugfs options. Split this out to make it a separate option. >> >> Signed-off-by: Laura Abbott <labbott@redhat.com> >> --- >> v2: Minor style fixups per Mark Rutland, intialization is now separate from >> register since it never needed to be combined in the first place, EFI >> page table registration. >> --- >> arch/arm64/Kconfig.debug | 6 +++++- >> arch/arm64/include/asm/ptdump.h | 13 ++++++++----- >> arch/arm64/mm/Makefile | 3 ++- >> arch/arm64/mm/dump.c | 26 +++++--------------------- >> arch/arm64/mm/ptdump_debugfs.c | 31 +++++++++++++++++++++++++++++++ >> drivers/firmware/efi/arm-runtime.c | 5 ++--- >> 6 files changed, 53 insertions(+), 31 deletions(-) >> create mode 100644 arch/arm64/mm/ptdump_debugfs.c >> >> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug >> index b661fe7..21a5b74 100644 >> --- a/arch/arm64/Kconfig.debug >> +++ b/arch/arm64/Kconfig.debug >> @@ -2,9 +2,13 @@ menu "Kernel hacking" >> >> source "lib/Kconfig.debug" >> >> -config ARM64_PTDUMP >> +config ARM64_PTDUMP_CORE >> + def_bool n >> + >> +config ARM64_PTDUMP_DEBUGFS >> bool "Export kernel pagetable layout to userspace via debugfs" > > I think this can be a tristate now, yes? (I did this on x86 so I could > load ptdump as a module for Chrome OS image testing but then leave the > module off the read-only partition for release images...) > > https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a > > -Kees > I saw the module for x86. arm64 works slightly differently since there are multiple debugfs dumping sources, the init_mm and EFI page tables. These are built in and can call register but then have no way to actually register the debugfs entry when the module is inserted since the current config covers both/all of the exporters. I decided against trying to add the infrastructure to make modularity work because I was spending more time on that than the actual W^X checks. Thanks, Laura >> depends on DEBUG_KERNEL >> + select ARM64_PTDUMP_CORE >> select DEBUG_FS >> help >> Say Y here if you want to show the kernel pagetable layout in a > > >
On Wed, Oct 12, 2016 at 3:57 PM, Laura Abbott <labbott@redhat.com> wrote: > On 10/12/2016 03:45 PM, Kees Cook wrote: >> >> On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote: >>> >>> >>> ptdump_register currently initializes a set of page table information and >>> registers debugfs. There are uses for the ptdump option without wanting >>> the >>> debugfs options. Split this out to make it a separate option. >>> >>> Signed-off-by: Laura Abbott <labbott@redhat.com> >>> --- >>> v2: Minor style fixups per Mark Rutland, intialization is now separate >>> from >>> register since it never needed to be combined in the first place, EFI >>> page table registration. >>> --- >>> arch/arm64/Kconfig.debug | 6 +++++- >>> arch/arm64/include/asm/ptdump.h | 13 ++++++++----- >>> arch/arm64/mm/Makefile | 3 ++- >>> arch/arm64/mm/dump.c | 26 +++++--------------------- >>> arch/arm64/mm/ptdump_debugfs.c | 31 +++++++++++++++++++++++++++++++ >>> drivers/firmware/efi/arm-runtime.c | 5 ++--- >>> 6 files changed, 53 insertions(+), 31 deletions(-) >>> create mode 100644 arch/arm64/mm/ptdump_debugfs.c >>> >>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug >>> index b661fe7..21a5b74 100644 >>> --- a/arch/arm64/Kconfig.debug >>> +++ b/arch/arm64/Kconfig.debug >>> @@ -2,9 +2,13 @@ menu "Kernel hacking" >>> >>> source "lib/Kconfig.debug" >>> >>> -config ARM64_PTDUMP >>> +config ARM64_PTDUMP_CORE >>> + def_bool n >>> + >>> +config ARM64_PTDUMP_DEBUGFS >>> bool "Export kernel pagetable layout to userspace via debugfs" >> >> >> I think this can be a tristate now, yes? (I did this on x86 so I could >> load ptdump as a module for Chrome OS image testing but then leave the >> module off the read-only partition for release images...) >> >> https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a >> >> -Kees >> > > I saw the module for x86. arm64 works slightly differently since there > are multiple debugfs dumping sources, the init_mm and EFI page tables. > These are built in and can call register but then have no way to > actually register the debugfs entry when the module is inserted since > the current config covers both/all of the exporters. I > decided against trying to add the infrastructure to make modularity > work because I was spending more time on that than the actual W^X checks. Okay, sounds fine. I'll see if I can take a look at this in the future if no one else beats me to it. In the meantime, this whole series looks good to me. :) Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
Hi Laura, In looking at this, I realised I was confused about ptdump_initialize() previously, and now see why we can't decouple the debugfs registration of the kernel page tables from the rest of the ptdump init. Sorry for the noise on that. Aside from one issue below, this looks good to me. On Wed, Oct 12, 2016 at 03:31:59PM -0700, Laura Abbott wrote: > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > index 07b8ed0..7c35689 100644 > --- a/arch/arm64/include/asm/ptdump.h > +++ b/arch/arm64/include/asm/ptdump.h > @@ -16,9 +16,10 @@ > #ifndef __ASM_PTDUMP_H > #define __ASM_PTDUMP_H > > -#ifdef CONFIG_ARM64_PTDUMP > +#ifdef CONFIG_ARM64_PTDUMP_CORE > > #include <linux/mm_types.h> > +#include <linux/seq_file.h> > > struct addr_marker { > unsigned long start_address; > @@ -32,13 +33,15 @@ struct ptdump_info { > unsigned long max_addr; > }; > > -int ptdump_register(struct ptdump_info *info, const char *name); > - > +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info); > +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS > +int ptdump_debugfs_register(struct ptdump_info *info, const char *name); > #else > -static inline int ptdump_register(struct ptdump_info *info, const char *name) > +static inline int ptdump_debugfs_register(struct ptdump_info *info, > + const char *name) > { > return 0; > } > -#endif /* CONFIG_ARM64_PTDUMP */ > +#endif I think you didn't mean to remove the existing endif here? It's still needed to guard the CONFIG_ARM64_PTDUMP_CORE case, and the new one is needed for the new #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS. Without it, I get a build error with this patch atop of v4.9-rc1 with CONFIG_ARM64_PTDUMP_DEBUGFS selected: [mark@leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j10 -s In file included from arch/arm64/mm/ptdump_debugfs.c:4:0: ./arch/arm64/include/asm/ptdump.h:16:0: error: unterminated #ifndef #ifndef __ASM_PTDUMP_H ^ make[1]: *** [arch/arm64/mm/ptdump_debugfs.o] Error 1 make[1]: *** Waiting for unfinished jobs.... With that #endif restored, everything works fine. So FWIW, with that: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Tested-by: Mark Rutland <mark.rutland@arm.com> [...] > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 7c75a8d..33d35e8 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = { > .mmlist = LIST_HEAD_INIT(efi_mm.mmlist), > }; > > -#ifdef CONFIG_ARM64_PTDUMP > +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS > #include <asm/ptdump.h> > > static struct ptdump_info efi_ptdump_info = { > @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = { > > static int __init ptdump_init(void) > { > - return ptdump_register(&efi_ptdump_info, "efi_page_tables"); > + return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables"); > } > device_initcall(ptdump_init); > - > #endif For the EFI changes, we'll need an ack from Ard or Matt; this should probably be Cc'd to the linux-efi list for that. Thanks, Mark.
On 10/17/2016 03:52 AM, Mark Rutland wrote: > Hi Laura, > > In looking at this, I realised I was confused about ptdump_initialize() > previously, and now see why we can't decouple the debugfs registration > of the kernel page tables from the rest of the ptdump init. Sorry for > the noise on that. > > Aside from one issue below, this looks good to me. > > On Wed, Oct 12, 2016 at 03:31:59PM -0700, Laura Abbott wrote: >> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h >> index 07b8ed0..7c35689 100644 >> --- a/arch/arm64/include/asm/ptdump.h >> +++ b/arch/arm64/include/asm/ptdump.h >> @@ -16,9 +16,10 @@ >> #ifndef __ASM_PTDUMP_H >> #define __ASM_PTDUMP_H >> >> -#ifdef CONFIG_ARM64_PTDUMP >> +#ifdef CONFIG_ARM64_PTDUMP_CORE >> >> #include <linux/mm_types.h> >> +#include <linux/seq_file.h> >> >> struct addr_marker { >> unsigned long start_address; >> @@ -32,13 +33,15 @@ struct ptdump_info { >> unsigned long max_addr; >> }; >> >> -int ptdump_register(struct ptdump_info *info, const char *name); >> - >> +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info); >> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS >> +int ptdump_debugfs_register(struct ptdump_info *info, const char *name); >> #else >> -static inline int ptdump_register(struct ptdump_info *info, const char *name) >> +static inline int ptdump_debugfs_register(struct ptdump_info *info, >> + const char *name) >> { >> return 0; >> } >> -#endif /* CONFIG_ARM64_PTDUMP */ >> +#endif > > I think you didn't mean to remove the existing endif here? > > It's still needed to guard the CONFIG_ARM64_PTDUMP_CORE case, and the > new one is needed for the new #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS. > Without it, I get a build error with this patch atop of v4.9-rc1 with > CONFIG_ARM64_PTDUMP_DEBUGFS selected: > > [mark@leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j10 -s > In file included from arch/arm64/mm/ptdump_debugfs.c:4:0: > ./arch/arm64/include/asm/ptdump.h:16:0: error: unterminated #ifndef > #ifndef __ASM_PTDUMP_H > ^ > make[1]: *** [arch/arm64/mm/ptdump_debugfs.o] Error 1 > make[1]: *** Waiting for unfinished jobs.... > > With that #endif restored, everything works fine. So FWIW, with that: > Ugh rebase/refactor failure on my part. > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com> > > [...] > >> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c >> index 7c75a8d..33d35e8 100644 >> --- a/drivers/firmware/efi/arm-runtime.c >> +++ b/drivers/firmware/efi/arm-runtime.c >> @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = { >> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist), >> }; >> >> -#ifdef CONFIG_ARM64_PTDUMP >> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS >> #include <asm/ptdump.h> >> >> static struct ptdump_info efi_ptdump_info = { >> @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = { >> >> static int __init ptdump_init(void) >> { >> - return ptdump_register(&efi_ptdump_info, "efi_page_tables"); >> + return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables"); >> } >> device_initcall(ptdump_init); >> - >> #endif > > For the EFI changes, we'll need an ack from Ard or Matt; this should > probably be Cc'd to the linux-efi list for that. Good point, I'll do that for v3. > > Thanks, > Mark. > Thanks, Laura
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug index b661fe7..21a5b74 100644 --- a/arch/arm64/Kconfig.debug +++ b/arch/arm64/Kconfig.debug @@ -2,9 +2,13 @@ menu "Kernel hacking" source "lib/Kconfig.debug" -config ARM64_PTDUMP +config ARM64_PTDUMP_CORE + def_bool n + +config ARM64_PTDUMP_DEBUGFS bool "Export kernel pagetable layout to userspace via debugfs" depends on DEBUG_KERNEL + select ARM64_PTDUMP_CORE select DEBUG_FS help Say Y here if you want to show the kernel pagetable layout in a diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h index 07b8ed0..7c35689 100644 --- a/arch/arm64/include/asm/ptdump.h +++ b/arch/arm64/include/asm/ptdump.h @@ -16,9 +16,10 @@ #ifndef __ASM_PTDUMP_H #define __ASM_PTDUMP_H -#ifdef CONFIG_ARM64_PTDUMP +#ifdef CONFIG_ARM64_PTDUMP_CORE #include <linux/mm_types.h> +#include <linux/seq_file.h> struct addr_marker { unsigned long start_address; @@ -32,13 +33,15 @@ struct ptdump_info { unsigned long max_addr; }; -int ptdump_register(struct ptdump_info *info, const char *name); - +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info); +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS +int ptdump_debugfs_register(struct ptdump_info *info, const char *name); #else -static inline int ptdump_register(struct ptdump_info *info, const char *name) +static inline int ptdump_debugfs_register(struct ptdump_info *info, + const char *name) { return 0; } -#endif /* CONFIG_ARM64_PTDUMP */ +#endif #endif /* __ASM_PTDUMP_H */ diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile index 54bb209..e703fb9 100644 --- a/arch/arm64/mm/Makefile +++ b/arch/arm64/mm/Makefile @@ -3,7 +3,8 @@ obj-y := dma-mapping.o extable.o fault.o init.o \ ioremap.o mmap.o pgd.o mmu.o \ context.o proc.o pageattr.o obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o -obj-$(CONFIG_ARM64_PTDUMP) += dump.o +obj-$(CONFIG_ARM64_PTDUMP_CORE) += dump.o +obj-$(CONFIG_ARM64_PTDUMP_DEBUGFS) += ptdump_debugfs.o obj-$(CONFIG_NUMA) += numa.o obj-$(CONFIG_KASAN) += kasan_init.o diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c index 9c3e75d..f0f0be7 100644 --- a/arch/arm64/mm/dump.c +++ b/arch/arm64/mm/dump.c @@ -304,9 +304,8 @@ static void walk_pgd(struct pg_state *st, struct mm_struct *mm, } } -static int ptdump_show(struct seq_file *m, void *v) +void ptdump_walk_pgd(struct seq_file *m, struct ptdump_info *info) { - struct ptdump_info *info = m->private; struct pg_state st = { .seq = m, .marker = info->markers, @@ -315,33 +314,16 @@ static int ptdump_show(struct seq_file *m, void *v) walk_pgd(&st, info->mm, info->base_addr); note_page(&st, 0, 0, 0); - return 0; } -static int ptdump_open(struct inode *inode, struct file *file) +static void ptdump_initialize(void) { - return single_open(file, ptdump_show, inode->i_private); -} - -static const struct file_operations ptdump_fops = { - .open = ptdump_open, - .read = seq_read, - .llseek = seq_lseek, - .release = single_release, -}; - -int ptdump_register(struct ptdump_info *info, const char *name) -{ - struct dentry *pe; unsigned i, j; for (i = 0; i < ARRAY_SIZE(pg_level); i++) if (pg_level[i].bits) for (j = 0; j < pg_level[i].num; j++) pg_level[i].mask |= pg_level[i].bits[j].mask; - - pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); - return pe ? 0 : -ENOMEM; } static struct ptdump_info kernel_ptdump_info = { @@ -352,6 +334,8 @@ static struct ptdump_info kernel_ptdump_info = { static int ptdump_init(void) { - return ptdump_register(&kernel_ptdump_info, "kernel_page_tables"); + ptdump_initialize(); + return ptdump_debugfs_register(&kernel_ptdump_info, + "kernel_page_tables"); } device_initcall(ptdump_init); diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c new file mode 100644 index 0000000..eee4d86 --- /dev/null +++ b/arch/arm64/mm/ptdump_debugfs.c @@ -0,0 +1,31 @@ +#include <linux/debugfs.h> +#include <linux/seq_file.h> + +#include <asm/ptdump.h> + +static int ptdump_show(struct seq_file *m, void *v) +{ + struct ptdump_info *info = m->private; + ptdump_walk_pgd(m, info); + return 0; +} + +static int ptdump_open(struct inode *inode, struct file *file) +{ + return single_open(file, ptdump_show, inode->i_private); +} + +static const struct file_operations ptdump_fops = { + .open = ptdump_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +int ptdump_debugfs_register(struct ptdump_info *info, const char *name) +{ + struct dentry *pe; + pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); + return pe ? 0 : -ENOMEM; + +} diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c index 7c75a8d..33d35e8 100644 --- a/drivers/firmware/efi/arm-runtime.c +++ b/drivers/firmware/efi/arm-runtime.c @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = { .mmlist = LIST_HEAD_INIT(efi_mm.mmlist), }; -#ifdef CONFIG_ARM64_PTDUMP +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS #include <asm/ptdump.h> static struct ptdump_info efi_ptdump_info = { @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = { static int __init ptdump_init(void) { - return ptdump_register(&efi_ptdump_info, "efi_page_tables"); + return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables"); } device_initcall(ptdump_init); - #endif static bool __init efi_virtmap_init(void)
ptdump_register currently initializes a set of page table information and registers debugfs. There are uses for the ptdump option without wanting the debugfs options. Split this out to make it a separate option. Signed-off-by: Laura Abbott <labbott@redhat.com> --- v2: Minor style fixups per Mark Rutland, intialization is now separate from register since it never needed to be combined in the first place, EFI page table registration. --- arch/arm64/Kconfig.debug | 6 +++++- arch/arm64/include/asm/ptdump.h | 13 ++++++++----- arch/arm64/mm/Makefile | 3 ++- arch/arm64/mm/dump.c | 26 +++++--------------------- arch/arm64/mm/ptdump_debugfs.c | 31 +++++++++++++++++++++++++++++++ drivers/firmware/efi/arm-runtime.c | 5 ++--- 6 files changed, 53 insertions(+), 31 deletions(-) create mode 100644 arch/arm64/mm/ptdump_debugfs.c