Re: [PATCH v3 3/4] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime

[Russell Currey <ruscur@russell.cc>]

On Tue, 2019-10-08 at 08:21 +0200, Christophe Leroy wrote:
> 
> Le 04/10/2019 à 09:50, Russell Currey a écrit :
> > Very rudimentary, just
> > 
> > 	echo 1 > [debugfs]/check_wx_pages
> > 
> > and check the kernel log.  Useful for testing strict module RWX.
> > 
> > Also fixed a typo.
> > 
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> >   arch/powerpc/mm/ptdump/ptdump.c | 31 +++++++++++++++++++++++++---
> > ---
> >   1 file changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/powerpc/mm/ptdump/ptdump.c
> > b/arch/powerpc/mm/ptdump/ptdump.c
> > index 2f9ddc29c535..0547cd9f264e 100644
> > --- a/arch/powerpc/mm/ptdump/ptdump.c
> > +++ b/arch/powerpc/mm/ptdump/ptdump.c
> > @@ -4,7 +4,7 @@
> >    *
> >    * This traverses the kernel pagetables and dumps the
> >    * information about the used sections of memory to
> > - * /sys/kernel/debug/kernel_pagetables.
> > + * /sys/kernel/debug/kernel_page_tables.
> >    *
> >    * Derived from the arm64 implementation:
> >    * Copyright (c) 2014, The Linux Foundation, Laura Abbott.
> > @@ -409,16 +409,35 @@ void ptdump_check_wx(void)
> >   	else
> >   		pr_info("Checked W+X mappings: passed, no W+X pages
> > found\n");
> >   }
> > +
> > +static int check_wx_debugfs_set(void *data, u64 val)
> > +{
> > +	if (val != 1ULL)
> > +		return -EINVAL;
> > +
> > +	ptdump_check_wx();
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set,
> > "%llu\n");
> >   #endif
> >   
> >   static int ptdump_init(void)
> >   {
> > -	struct dentry *debugfs_file;
> > -
> >   	populate_markers();
> >   	build_pgtable_complete_mask();
> > -	debugfs_file = debugfs_create_file("kernel_page_tables", 0400,
> > NULL,
> > -			NULL, &ptdump_fops);
> > -	return debugfs_file ? 0 : -ENOMEM;
> > +
> > +	if (!debugfs_create_file("kernel_page_tables", 0400, NULL,
> > +				 NULL, &ptdump_fops))
> > +		return -ENOMEM;
> > +
> > +#ifdef CONFIG_PPC_DEBUG_WX
> > +	if (!debugfs_create_file("check_wx_pages", 0200, NULL,
> > +				 NULL, &check_wx_fops))
> > +		return -ENOMEM;
> > +#endif
> 
> The above seems to be completely independant from everything else in 
> ptdump_init().
> 
> Could we avoid this #ifdef block inside ptdump_init() by creating a 
> selfstanding device_initcall() for that through a function called 
> ptdump_check_wx_init() defined inside the same #ifdef block as 
> ptdump_check_wx() ?

Yes that would be nicer, thanks

> 
> Christophe
> 
> > +
> > +	return 0;
> >   }
> >   device_initcall(ptdump_init);
> >