linux-kernel - Re: [PATCH 6/8] perf/uprobe: split 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: <CAEf4BzYuGo572m+zi3KD51zp82a63mL9f5F2kz1w8ZvEBQB_VA@mail.gmail.com>
Date: Mon, 5 Aug 2024 13:01:33 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: "Liao, Chang" <liaochang1@...wei.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, oleg@...hat.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jolsa@...nel.org, paulmck@...nel.org
Subject: Re: [PATCH 6/8] perf/uprobe: split uprobe_unregister()

On Fri, Aug 2, 2024 at 8:05 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Aug 1, 2024 at 7:41 PM Liao, Chang <liaochang1@...wei.com> wrote:
> >
> >
> >
> > 在 2024/8/1 5:42, Andrii Nakryiko 写道:
> > > From: Peter Zijlstra <peterz@...radead.org>
> > >
> > > With uprobe_unregister() having grown a synchronize_srcu(), it becomes
> > > fairly slow to call. Esp. since both users of this API call it in a
> > > loop.
> > >
> > > Peel off the sync_srcu() and do it once, after the loop.
> > >
> > > With recent uprobe_register()'s error handling reusing full
> > > uprobe_unregister() call, we need to be careful about returning to the
> > > caller before we have a guarantee that partially attached consumer won't
> > > be called anymore. So add uprobe_unregister_sync() in the error handling
> > > path. This is an unlikely slow path and this should be totally fine to
> > > be slow in the case of an failed attach.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > > Co-developed-by: Andrii Nakryiko <andrii@...nel.org>
> > > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > > ---
> > >  include/linux/uprobes.h                        |  8 ++++++--
> > >  kernel/events/uprobes.c                        | 18 ++++++++++++++----
> > >  kernel/trace/bpf_trace.c                       |  5 ++++-
> > >  kernel/trace/trace_uprobe.c                    |  6 +++++-
> > >  .../selftests/bpf/bpf_testmod/bpf_testmod.c    |  3 ++-
> > >  5 files changed, 31 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > index a1686c1ebcb6..8f1999eb9d9f 100644
> > > --- a/include/linux/uprobes.h
> > > +++ b/include/linux/uprobes.h
> > > @@ -105,7 +105,8 @@ extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
> > >  extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
> > >  extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
> > >  extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool);
> > > -extern void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc);
> > > +extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc);
> > > +extern void uprobe_unregister_sync(void);
> >
> > [...]
> >
> > >  static inline void
> > > -uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > > +uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > > +{
> > > +}
> > > +static inline void uprobes_unregister_sync(void)
> >
> > *uprobes*_unregister_sync, is it a typo?
> >
>
> I think the idea behind this is that you do a lot of individual uprobe
> unregistrations with multiple uprobe_unregister() calls, and then
> follow with a single *uprobes*_unregister_sync(), because in general
> it is meant to sync multiple uprobes unregistrations.

Ah, I think you were trying to say that only static inline
implementation here is called uprobes_unregister_sync, while all the
other ones are uprobe_unregister_sync(). I fixed it up, kept it as
singular uprobe_unregister_sync().

>
> > >  {
> > >  }
> > >  static inline int uprobe_mmap(struct vm_area_struct *vma)
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index 3b42fd355256..b0488d356399 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c

[...]