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

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 28 Jul 2020 at 11:17, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Sun, 26 Jul 2020 19:06:20 +0300
> Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > On Sun, 26 Jul 2020 at 11:14, Mike Rapoport <rppt@kernel.org> 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@linux.intel.com> wrote:
> > > > >
> > > > > > Use text_alloc() and text_free() instead of module_alloc() and
> > > > > > module_memfree() when an arch provides them.
> > > > > >
> > > > > > Cc: linux-mm@kvack.org
> > > > > > Cc: Andi Kleen <ak@linux.intel.com>
> > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.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.
>
> The name of the insn_pages came from the struct kprobe_insn_page, so
> if there is a text_alloc_kprobe(), I'm OK to rename it. (anyway, that
> is an allocation operator, we don't call it directly.)
>
> > 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)
>
> Actually, x86 and arm has the "relative branching range" requirements
> for the jump optimized kprobes. For the other architectures, I think
> we don't need it. Only executable text buffer is needed.
>

Thanks for the explanation. Today, arm64 uses the definition below.

void *alloc_insn_page(void)
{
  return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
    GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
    NUMA_NO_NODE, __builtin_return_address(0));
}

Do you think we could use that as the generic implementation if we use
MODULES_START/_END as the allocation window?