Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Andrii Nakryiko]

On Wed, Jul 31, 2024 at 9:56 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> > On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *"
> > > rather than inode + offset. This simplifies the code and allows to avoid
> > > the unnecessary find_uprobe() + put_uprobe() in these functions.
> > >
> > > TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that
> > > this uprobe can't be freed before up_write(&uprobe->register_rwsem).
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> > > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > >  include/linux/uprobes.h     | 15 +++++-----
> > >  kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
> > >  kernel/trace/bpf_trace.c    | 25 ++++++++---------
> > >  kernel/trace/trace_uprobe.c | 26 ++++++++---------
> > >  4 files changed, 55 insertions(+), 67 deletions(-)
> > >
> >
> > You'll need something like below to not break our bpf_testmod. And
> > please send pull patch sets, not individually updated patches, it's a
> > PITA to deal with. Thanks!
>
> Do I stuff this on top of Oleg's patch or do you want me to fold it in
> one of them?

Please fold so we have better (potential) bisectability of BPF
selftests, thanks!

>
> > commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)
> > Author: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > Date:   Wed Jul 31 09:15:46 2024 -0700
> >
> >     uprobes: fix bpf_testmod after uprobe_register/uprobe_unregister API change
> >
> >     Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 5f152afdec2f..73a6b041bcce 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -431,6 +431,7 @@ uprobe_ret_handler(struct uprobe_consumer *self,
> > unsigned long func,
> >  }
> >
> >  struct testmod_uprobe {
> > +       struct uprobe *uprobe;
> >         struct path path;
> >         loff_t offset;
> >         struct uprobe_consumer consumer;
> > @@ -458,12 +459,14 @@ static int testmod_register_uprobe(loff_t offset)
> >         if (err)
> >                 goto out;
> >
> > -       err = uprobe_register(d_real_inode(uprobe.path.dentry),
> > -                             offset, 0, &uprobe.consumer);
> > -       if (err)
> > +       uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
> > +                                       offset, 0, &uprobe.consumer);
> > +       if (IS_ERR(uprobe.uprobe)) {
> >                 path_put(&uprobe.path);
> > -       else
> > +               uprobe.uprobe = NULL;
> > +       } else {
> >                 uprobe.offset = offset;
> > +       }
> >
> >  out:
> >         mutex_unlock(&testmod_uprobe_mutex);
> > @@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
> >  {
> >         mutex_lock(&testmod_uprobe_mutex);
> >
> > -       if (uprobe.offset) {
> > -               uprobe_unregister(d_real_inode(uprobe.path.dentry),
> > -                                 uprobe.offset, &uprobe.consumer);
> > +       if (uprobe.uprobe) {
> > +               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
> >                 uprobe.offset = 0;
> > +               uprobe.uprobe = NULL;
> >         }
> >
> >         mutex_unlock(&testmod_uprobe_mutex);
> >
> >
> > [...]