From: Ard Biesheuvel <ardb@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mike Rapoport <rppt@kernel.org>,
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
Ingo Molnar <mingo@kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-mm@kvack.org, Andi Kleen <ak@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
"David S. Miller" <davem@davemloft.net>,
Jessica Yu <jeyu@kernel.org>
Subject: Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text_free()
Date: Tue, 28 Jul 2020 13:56:43 +0300 [thread overview]
Message-ID: <CAMj1kXGJ_7mUtFHWsLaBj-MSK_VxpBet=wi1Z7frkKRVEgozpQ@mail.gmail.com> (raw)
In-Reply-To: <20200728171715.0800093e2226e3d72b04a3ae@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?
next prev parent reply other threads:[~2020-07-28 10:56 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-24 5:05 [PATCH v5 0/6] arch/x86: kprobes: Remove MODULES dependency Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex Jarkko Sakkinen
2020-07-24 9:13 ` Ingo Molnar
2020-07-25 2:36 ` Jarkko Sakkinen
2020-07-24 9:17 ` Ingo Molnar
2020-07-25 3:01 ` Jarkko Sakkinen
2020-07-25 10:21 ` Ingo Molnar
2020-07-28 7:34 ` Masami Hiramatsu
2020-08-17 21:22 ` Jarkko Sakkinen
2020-07-24 10:22 ` Mike Rapoport
2020-07-25 2:42 ` Jarkko Sakkinen
2020-07-24 14:46 ` Masami Hiramatsu
2020-07-25 2:48 ` Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 2/6] vmalloc: Add text_alloc() and text_free() Jarkko Sakkinen
2020-07-24 10:22 ` Mike Rapoport
2020-07-25 2:20 ` Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 3/6] arch/x86: Implement " Jarkko Sakkinen
2020-07-24 9:22 ` Ingo Molnar
2020-07-25 2:03 ` Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 4/6] arch/x86: kprobes: Use " Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 5/6] " Jarkko Sakkinen
2020-07-24 9:27 ` Ingo Molnar
2020-07-24 12:16 ` Ard Biesheuvel
2020-07-25 3:19 ` [PATCH v5 5/6] kprobes: Use text_alloc() and text_free()] Jarkko Sakkinen
2020-07-25 3:16 ` [PATCH v5 5/6] kprobes: Use text_alloc() and text_free() Jarkko Sakkinen
2020-07-26 8:14 ` Mike Rapoport
2020-07-26 16:06 ` Ard Biesheuvel
2020-07-28 8:17 ` Masami Hiramatsu
2020-07-28 10:56 ` Ard Biesheuvel [this message]
2020-07-28 13:35 ` Masami Hiramatsu
2020-07-28 17:51 ` Ard Biesheuvel
2020-07-29 1:50 ` Masami Hiramatsu
2020-07-29 6:13 ` Ard Biesheuvel
2020-07-30 1:09 ` Masami Hiramatsu
2020-08-18 5:30 ` Jarkko Sakkinen
2020-08-18 11:51 ` Mike Rapoport
2020-08-18 16:30 ` Jarkko Sakkinen
2020-08-19 6:47 ` Mike Rapoport
2020-08-19 21:07 ` Jarkko Sakkinen
2020-07-24 10:27 ` Mike Rapoport
2020-07-24 14:57 ` Masami Hiramatsu
2020-07-24 23:38 ` Jarkko Sakkinen
2020-07-24 5:05 ` [PATCH v5 6/6] kprobes: Remove CONFIG_MODULES dependency Jarkko Sakkinen
2020-07-24 7:01 ` [PATCH v5 0/6] arch/x86: kprobes: Remove MODULES dependency Jarkko Sakkinen
2020-07-24 10:26 ` Mike Rapoport
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMj1kXGJ_7mUtFHWsLaBj-MSK_VxpBet=wi1Z7frkKRVEgozpQ@mail.gmail.com' \
--to=ardb@kernel.org \
--cc=ak@linux.intel.com \
--cc=anil.s.keshavamurthy@intel.com \
--cc=davem@davemloft.net \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jeyu@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=naveen.n.rao@linux.ibm.com \
--cc=peterz@infradead.org \
--cc=rppt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).