linux-kernel - Re: [PATCH bpf-next 06/13] uprobes/x86: Add uprobe syscall to speed up uprobe

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]

Message-ID: <Z1_e1OPwP-qUPb1_@krava>
Date: Mon, 16 Dec 2024 09:03:32 +0100
From: Jiri Olsa <olsajiri@...il.com>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: Jiri Olsa <olsajiri@...il.com>, Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	Hao Luo <haoluo@...gle.com>, Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Alan Maguire <alan.maguire@...cle.com>,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next 06/13] uprobes/x86: Add uprobe syscall to speed
 up uprobe

On Sat, Dec 14, 2024 at 02:21:43PM +0100, Thomas Weißschuh wrote:
> On 2024-12-13 22:52:15+0100, Jiri Olsa wrote:
> > On Fri, Dec 13, 2024 at 04:12:46PM +0100, Thomas Weißschuh wrote:
> > 
> > SNIP
> > 
> > > > > > +static int __init arch_uprobes_init(void)
> > > > > > +{
> > > > > > +	unsigned long size = uprobe_trampoline_end - uprobe_trampoline_entry;
> > > > > > +	static struct page *pages[2];
> > > > > > +	struct page *page;
> > > > > > +
> > > > > > +	page = alloc_page(GFP_HIGHUSER);
> > > > > 
> > > > > That page could be in static memory, removing the need for the explicit
> > > > > allocation. It could also be __ro_after_init.
> > > > > Then tramp_mapping itself can be const.
> > > > 
> > > > hum, how would that look like? I think that to get proper page object
> > > > you have to call alloc_page or some other page alloc family function..
> > > > what do I miss?
> > > 
> > > static u8 trampoline_page[PAGE_SIZE] __ro_after_init __aligned(PAGE_SIZE);
> > > static struct page *tramp_mapping_pages[2] __ro_after_init;
> > > 
> > > static const struct vm_special_mapping tramp_mapping = {
> > > 	.name   = "[uprobes-trampoline]",
> > > 	.pages  = tramp_mapping_pages,
> > > 	.mremap = tramp_mremap,
> > > };
> > > 
> > > static int __init arch_uprobes_init(void)
> > > {
> > > 	...
> > > 	trampoline_pages[0] = virt_to_page(trampoline_page);
> > > 	...
> > > }
> > > 
> > > Untested, but it's similar to the stuff the vDSO implementations are
> > > doing which I am working with at the moment.
> > 
> > nice idea, better than allocating the page, will do that
> 
> Or even better yet, just allocate the whole page already in the inline
> asm and avoid the copying, too:
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index b2420eeee23a..c5e6ca7f998a 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -462,7 +462,7 @@ SYSCALL_DEFINE0(uprobe)
> 
>  asm (
>         ".pushsection .rodata\n"
> -       ".global uprobe_trampoline_entry\n"
> +       ".balign " __stringify(PAGE_SIZE) "\n"
>         "uprobe_trampoline_entry:\n"
>         "endbr64\n"
>         "push %rcx\n"
> @@ -474,13 +474,11 @@ asm (
>         "pop %r11\n"
>         "pop %rcx\n"
>         "ret\n"
> -       ".global uprobe_trampoline_end\n"
> -       "uprobe_trampoline_end:\n"
> +       ".balign " __stringify(PAGE_SIZE) "\n"
>         ".popsection\n"
>  );
> 
> -extern __visible u8 uprobe_trampoline_entry[];
> -extern __visible u8 uprobe_trampoline_end[];
> +extern u8 uprobe_trampoline_entry[];
> 
> 
> If you want to keep the copying for some reason, the asm code should be
> in the section ".init.rodata" as its not used afterwards.

perfect, no need for copy, I'll do what you propose above

> 
> (A bit bikesheddy, I admit)

thanks for the review,

jirka