On Fri, Jun 27, 2025 at 03:01:45PM +0900, Masami Hiramatsu wrote:
SNIP
> > >
> > > > + return tramp;
> > > > + }
> > > > +
> > > > + tramp = create_uprobe_trampoline(vaddr);
> > > > + if (!tramp)
> > > > + return NULL;
> > > > +
> > > > + *new = true;
> > > > + hlist_add_head(&tramp->node, &state->head_tramps);
> > > > + return tramp;
> > > > +}
> > > > +
> > > > +static void destroy_uprobe_trampoline(struct uprobe_trampoline *tramp)
> > > > +{
> > > > + hlist_del(&tramp->node);
> > > > + kfree(tramp);
> > >
> > > Don't we need to unmap the tramp->vaddr?
> >
> > that's tricky because we have no way to make sure the application is
> > no longer executing the trampoline, it's described in the changelog
> > of following patch:
> >
> > uprobes/x86: Add support to optimize uprobes
> >
> > ...
> >
> > We do not unmap and release uprobe trampoline when it's no longer
> > needed,
> > because there's no easy way to make sure none of the threads is still
> > inside the trampoline. But we do not waste memory, because there's just
> > single page for all the uprobe trampoline mappings.
> >
>
> I think we should put this as a code comment.
ok
>
> > We do waste frame on page mapping for every 4GB by keeping the uprobe
> > trampoline page mapped, but that seems ok.
>
> Hmm, this is not right with the current find_nearest_page(), because
> it always finds a page from the farthest +2GB range until it is full.
> Thus, in the worst case, if we hits uprobes with the order of
> uprobe0 -> 1 -> 2 which is put as below;
>
> 0x0abc0004 [uprobe2]
> ...
> 0x0abc2004 [uprobe1]
> ...
> 0x0abc4004 [uprobe0]
>
> Then the trampoline pages can be allocated as below.
>
> 0x8abc0000 [uprobe_tramp2]
> [gap]
> 0x8abc2000 [uprobe_tramp1]
> [gap]
> 0x8abc4000 [uprobe_tramp0]
>
> Using true "find_nearest_page()", this will be mitigated. But not
> allocated for "every 4GB". So I think we should drop that part
> from the comment :)
I think you're right, it's better to start with nearest page,
will change it in new version
SNIP
> > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > > index 5080619560d4..b40d33aae016 100644
> > > > --- a/include/linux/uprobes.h
> > > > +++ b/include/linux/uprobes.h
> > > > @@ -17,6 +17,7 @@
> > > > #include <linux/wait.h>
> > > > #include <linux/timer.h>
> > > > #include <linux/seqlock.h>
> > > > +#include <linux/mutex.h>
> > > >
> > > > struct uprobe;
> > > > struct vm_area_struct;
> > > > @@ -185,6 +186,9 @@ struct xol_area;
> > > >
> > > > struct uprobes_state {
> > > > struct xol_area *xol_area;
> > > > +#ifdef CONFIG_X86_64
> > >
> > > Maybe we can introduce struct arch_uprobe_state{} here?
> >
> > ok, on top of that Andrii also asked for [1]:
> > - alloc 'struct uprobes_state' for mm_struct only when needed
> >
> > this could be part of that follow up? I'd rather not complicate this
> > patchset any further
> >
> > [1]
> > https://lore.kernel.org/bpf/CAEf4BzY2zKPM9JHgn_wa8yCr8q5KntE5w8g=aot2mnrd2dx...@mail.gmail.com/
>
> Hmm, OK. But if you need to avoid #ifdef CONFIG_<arch>,
> you can use include/asm-generic to override macros.
>
> struct uprobes_state {
> struct xol_area *xol_area;
> uprobe_arch_specific_data
> };
>
>
> --- include/asm-generic/uprobes.h
>
> #define uprobe_arch_specific_data
>
> --- arch/x86/include/asm/uprobes.h
>
> #undef uprobe_arch_specific_data
> #define uprobe_arch_specific_data \
> struct hlist_head head_tramps;
ok
SNIP
> > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > index 1ee8eb11f38b..7108ca558518 100644
> > > > --- a/kernel/fork.c
> > > > +++ b/kernel/fork.c
> > > > @@ -1010,6 +1010,7 @@ static void mm_init_uprobes_state(struct
> > > > mm_struct *mm)
> > > > {
> > > > #ifdef CONFIG_UPROBES
> > > > mm->uprobes_state.xol_area = NULL;
> > > > + arch_uprobe_init_state(mm);
> > > > #endif
> > >
> > > Can't we make this uprobe_init_state(mm)?
> >
> > hum, there are other mm_init_* functions around, I guess we should keep
> > the same pattern?
> >
> > unless you mean s/arch_uprobe_init_state/uprobe_init_state/ but that's
> > arch code.. so probably not sure what you mean ;-)
>
> Ah, I misunderstood. Yeah, this part is good to me.
ok, thanks
jirka
