Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text_free()

[Ard Biesheuvel]

On Sun, 26 Jul 2020 at 11:14, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> wrote:
> > >
> > > > Use text_alloc() and text_free() instead of module_alloc() and
> > > > module_memfree() when an arch provides them.
> > > >
> > > > Cc: linux-mm@xxxxxxxxx
> > > > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxx/
> >
> > 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@xxxxxxxxxx/
> >
> > 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.
>

Masami or Peter should correct me if I am wrong, but it seems to me
that the way kprobes uses these pages does not require them to be in
relative branching range of the core kernel on any architecture, given
that they are populated with individual instruction opcodes that are
executed in single step mode, and relative branches are emulated (when
needed)

So for kprobes in particular, we should be able to come up with a
generic sequence that does not involve module_alloc(), and therefore
removes the kprobes dependency on module support entirely (with the
exception of power which maps the vmalloc space nx when module support
is disabled). Renaming alloc_insn_page() to something more descriptive
makes sense imo, but is a separate issue.