linux-kernel - Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 26 Jul 2020 11:14:08 +0300
From:   Mike Rapoport <rppt@...nel.org>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Andi Kleen <ak@...ux.intel.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jessica Yu <jeyu@...nel.org>, Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text_free()

On Sat, Jul 25, 2020 at 06:16:48AM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 24, 2020 at 11:27:46AM +0200, Ingo Molnar wrote:
> > 
> > * Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com> wrote:
> > 
> > > Use text_alloc() and text_free() instead of module_alloc() and
> > > module_memfree() when an arch provides them.
> > > 
> > > Cc: linux-mm@...ck.org
> > > Cc: Andi Kleen <ak@...ux.intel.com>
> > > Cc: Masami Hiramatsu <mhiramat@...nel.org>
> > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> > > ---
> > >  kernel/kprobes.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > index 4e46d96d4e16..611fcda9f6bf 100644
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -40,6 +40,7 @@
> > >  #include <asm/cacheflush.h>
> > >  #include <asm/errno.h>
> > >  #include <linux/uaccess.h>
> > > +#include <linux/vmalloc.h>
> > >  
> > >  #define KPROBE_HASH_BITS 6
> > >  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
> > > @@ -111,12 +112,20 @@ enum kprobe_slot_state {
> > >  
> > >  void __weak *alloc_insn_page(void)
> > >  {
> > > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > > +	return text_alloc(PAGE_SIZE);
> > > +#else
> > >  	return module_alloc(PAGE_SIZE);
> > > +#endif
> > >  }
> > >  
> > >  void __weak free_insn_page(void *page)
> > >  {
> > > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > > +	text_free(page);
> > > +#else
> > >  	module_memfree(page);
> > > +#endif
> > >  }
> > 
> > I've read the observations in the other threads, but this #ifdef 
> > jungle is silly, it's a de-facto open coded text_alloc() with a 
> > module_alloc() fallback...
> 
> In the previous version I had:
> 
>   https://lore.kernel.org/lkml/20200717030422.679972-4-jarkko.sakkinen@linux.intel.com/
> 
> and I had just calls to text_alloc() and text_free() in corresponding
> snippet to the above.
> 
> I got this feedback from Mike:
> 
>   https://lore.kernel.org/lkml/20200718162359.GA2919062@kernel.org/
> 
> I'm not still sure that I fully understand this feedback as I don't see
> any inherent and obvious difference to the v4. In that version fallbacks
> are to module_alloc() and module_memfree() and text_alloc() and
> text_memfree() can be overridden by arch.

Let me try to elaborate.

There are several subsystems that need to allocate memory for executable
text. As it happens, they use module_alloc() with some abilities for
architectures to override this behaviour.

For many architectures, it would be enough to rename modules_alloc() to
text_alloc(), make it built-in and this way allow removing dependency on
MODULES.

Yet, some architectures have different restrictions for code allocation
for different subsystems so it would make sense to have more than one
variant of text_alloc() and a single config option ARCH_HAS_TEXT_ALLOC
won't be sufficient.

I liked Mark's suggestion to have text_alloc_<something>() and proposed
a way to introduce text_alloc_kprobes() along with
HAVE_KPROBES_TEXT_ALLOC to enable arch overrides of this function.

The major difference between your v4 and my suggestion is that I'm not
trying to impose a single ARCH_HAS_TEXT_ALLOC as an alternative to
MODULES but rather to use per subsystem config option, e.g.
HAVE_KPROBES_TEXT_ALLOC.

Another thing, which might be worth doing regardless of the outcome of
this discussion is to rename alloc_insn_pages() to text_alloc_kprobes()
because the former is way too generic and does not emphasize that the 
instruction page is actually used by kprobes only.

> /Jarkko
> 

-- 
Sincerely yours,
Mike.