Re: [PATCH v4 3/7] vmalloc: Add text_alloc() and text_free()
From: Masami Hiramatsu
Date: Mon Jul 20 2020 - 08:01:55 EST
On Sat, 18 Jul 2020 19:23:59 +0300
Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> On Fri, Jul 17, 2020 at 06:04:17AM +0300, Jarkko Sakkinen wrote:
> > Introduce functions for allocating memory for dynamic trampolines, such
> > as kprobes. An arch can promote the availability of these functions with
> > CONFIG_ARCH_HAS_TEXT_ALLOC. Provide default/fallback implementation
> > wrapping module_alloc() and module_memfree().
> >
> > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> > ---
> > include/linux/vmalloc.h | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> > index 0221f852a7e1..e981436e30b6 100644
> > --- a/include/linux/vmalloc.h
> > +++ b/include/linux/vmalloc.h
> > @@ -9,6 +9,7 @@
> > #include <asm/page.h> /* pgprot_t */
> > #include <linux/rbtree.h>
> > #include <linux/overflow.h>
> > +#include <linux/moduleloader.h>
> >
> > #include <asm/vmalloc.h>
> >
> > @@ -249,4 +250,26 @@ pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms)
> > int register_vmap_purge_notifier(struct notifier_block *nb);
> > int unregister_vmap_purge_notifier(struct notifier_block *nb);
> >
> > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
> > +/*
> > + * Allocate memory to be used for dynamic trampoline code.
> > + */
> > +void *text_alloc(unsigned long size);
> > +
> > +/*
> > + * Free memory returned from text_alloc().
> > + */
> > +void text_free(void *region);
> > +#else
> > +static inline void *text_alloc(unsigned long size)
> > +{
> > + return module_alloc(size);
> > +}
> > +
> > +static inline void text_free(void *region)
> > +{
> > + module_memfree(region);
> > +}
>
> Using module_alloc() as the default implementation of generic
> text_alloc() does not sound right to me.
>
> I would suggest rename module_alloc() to text_alloc() on x86, as Peter
> proposed and then add text_alloc_kprobes() that can be overridden by the
> architectures. x86 could use text_alloc(), arm64 vmalloc() with options
> of their choice and the fallback would remain module_alloc(). Something
> like (untested) patch below:
Hmm, of course we need some more work on this patch (e.g. MODULE_STATE_*
for callbacks), but basically, this direction seems good to me for the
first step.
If other dynamic trampoline code (ftrace and bpf) are interested in replacing
the module_alloc() with text_alloc_*(), we should revisit how we can unify
it (or at least share the common parts.)
BTW, this changes CONFIG_MODULES dependency according to the architecture
implementation. It also should be noted somewhere.
Thank you,
> From 928b6903e76ebf5790fc415f9eed390e400e5bc3 Mon Sep 17 00:00:00 2001
> From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> Date: Sat, 18 Jul 2020 19:13:02 +0300
> Subject: [PATCH] kprobes: introduce text_alloc_kprobes() and
> text_free_kprobes()
>
> Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> ---
> arch/Kconfig | 5 ++++-
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/probes/kprobes.c | 10 ++++++++--
> arch/x86/Kconfig | 3 ++-
> arch/x86/kernel/kprobes/core.c | 9 ++++-----
> include/linux/kprobes.h | 17 +++++++++++++++--
> kernel/kprobes.c | 8 ++++----
> 7 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8cc35dc556c7..c0589b3b3225 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -61,7 +61,7 @@ config OPROFILE_NMI_TIMER
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> + depends on MODULES || HAVE_KPROBES_TEXT_ALLOC
> depends on HAVE_KPROBES
> select KALLSYMS
> help
> @@ -186,6 +186,9 @@ config HAVE_IOREMAP_PROT
> config HAVE_KPROBES
> bool
>
> +config HAVE_KPROBES_TEXT_ALLOC
> + bool
> +
> config HAVE_KRETPROBES
> bool
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 66dc41fd49f2..abc0538b13ef 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -177,6 +177,7 @@ config ARM64
> select HAVE_STACKPROTECTOR
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_KPROBES
> + select HAVE_KPROBES_TEXT_ALLOC
> select HAVE_KRETPROBES
> select HAVE_GENERIC_VDSO
> select IOMMU_DMA if IOMMU_SUPPORT
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 5290f17a4d80..fac8c6020040 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -118,13 +118,19 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> return 0;
> }
>
> -void *alloc_insn_page(void)
> +void *text_alloc_kprobes(unsigned long size)
> {
> - return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> + return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> NUMA_NO_NODE, __builtin_return_address(0));
> }
>
> +void text_free_kprobes(void *mem)
> +{
> + lockdep_assert_irqs_enabled();
> + vfree(mem);
> +}
> +
> /* arm kprobe: install breakpoint in text */
> void __kprobes arch_arm_kprobe(struct kprobe *p)
> {
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 883da0abf779..d08b92f91531 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -190,6 +190,7 @@ config X86
> select HAVE_KERNEL_XZ
> select HAVE_KPROBES
> select HAVE_KPROBES_ON_FTRACE
> + select HAVE_KPROBES_TEXT_ALLOC
> select HAVE_FUNCTION_ERROR_INJECTION
> select HAVE_KRETPROBES
> select HAVE_KVM
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index ada39ddbc922..a3e8f01c62d4 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -418,12 +418,11 @@ static int prepare_boost(kprobe_opcode_t *buf, struct kprobe *p,
> return len;
> }
>
> -/* Make page to RO mode when allocate it */
> -void *alloc_insn_page(void)
> +void *text_alloc_kprobes(unsigned long size)
> {
> void *page;
>
> - page = module_alloc(PAGE_SIZE);
> + page = text_alloc(size);
> if (!page)
> return NULL;
>
> @@ -444,9 +443,9 @@ void *alloc_insn_page(void)
> }
>
> /* Recover page to RW mode before releasing it */
> -void free_insn_page(void *page)
> +void text_free_kprobes(void *page)
> {
> - module_memfree(page);
> + text_free(page);
> }
>
> static int arch_copy_kprobe(struct kprobe *p)
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 6adf90f248d7..dbc9c71d4ec4 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -27,6 +27,7 @@
> #include <linux/rcupdate.h>
> #include <linux/mutex.h>
> #include <linux/ftrace.h>
> +#include <linux/moduleloader.h>
> #include <asm/kprobes.h>
>
> #ifdef CONFIG_KPROBES
> @@ -374,8 +375,20 @@ int enable_kprobe(struct kprobe *kp);
>
> void dump_kprobe(struct kprobe *kp);
>
> -void *alloc_insn_page(void);
> -void free_insn_page(void *page);
> +#ifdef CONFIG_HAVE_KPROBES_TEXT_ALLOC
> +void *text_alloc_kprobes(unsigned long size);
> +void text_free_kprobes(void *page);
> +#else
> +static inline void *text_alloc_kprobes(unsigned long size)
> +{
> + return module_alloc(size);
> +}
> +
> +static inline void text_free_kprobes(void *page)
> +{
> + module_memfree(page);
> +}
> +#endif
>
> #else /* !CONFIG_KPROBES: */
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2e97febeef77..c4f107682250 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -109,14 +109,14 @@ enum kprobe_slot_state {
> SLOT_USED = 2,
> };
>
> -void __weak *alloc_insn_page(void)
> +static void *alloc_insn_page(void)
> {
> - return module_alloc(PAGE_SIZE);
> + return text_alloc_kprobes(PAGE_SIZE);
> }
>
> -void __weak free_insn_page(void *page)
> +static void free_insn_page(void *page)
> {
> - module_memfree(page);
> + text_free_kprobes(page);
> }
>
> struct kprobe_insn_cache kprobe_insn_slots = {
> --
> 2.26.2
>
>
> > +#endif
> > +
> > #endif /* _LINUX_VMALLOC_H */
> > --
> > 2.25.1
> >
>
> --
> Sincerely yours,
> Mike.
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>