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

[Andrii Nakryiko]

On Wed, Jul 10, 2024 at 1:18 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 07/10, Andrii Nakryiko wrote:
> >
> > On Wed, Jul 10, 2024 at 9:33 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > This way uprobe_unregister() and uprobe_apply() do not need find_uprobe() +
> > > put_uprobe(). And to me this change simplifies the code a bit.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> > > ---
> > >  include/linux/uprobes.h     | 14 ++++++------
> > >  kernel/events/uprobes.c     | 45 ++++++++++++-------------------------
> > >  kernel/trace/bpf_trace.c    | 12 +++++-----
> > >  kernel/trace/trace_uprobe.c | 28 +++++++++++------------
> > >  4 files changed, 41 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > index aa89a8b67039..399509befcf4 100644
> > > --- a/include/linux/uprobes.h
> > > +++ b/include/linux/uprobes.h
> >
> > I don't see struct uprobe forward-declared in this header, maybe we
> > should add it?
>
> Probably yes, thanks... Although the current code already uses
> struct uprobes * without forward-declaration at least if CONFIG_UPROBES=y.
> Thanks, will add.
>

Yep, I saw that and was wondering as well.

> > >  static inline int
> > > -uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool add)
> > > +uprobe_apply(struct uprobe* uprobe, struct uprobe_consumer *uc, bool add)
> > >  {
> > >         return -ENOSYS;
> > >  }
> >
> > complete aside, when I was looking at this code I was wondering why we
> > even need uprobe_apply, it looks like some hacky variant of
> > uprobe_register and uprobe_unregister.
>
> All I can say is that
>
>         - I can hardly recall this logic, I'll try to do this tomorrow
>           and write another email
>
>         - in any case this logic is ugly and needs more cleanups
>
>         - but this patch only tries to simplify this code without any
>           visible changes.

yep, that's why it's an aside, up to you

>
> > > @@ -1133,41 +1126,39 @@ EXPORT_SYMBOL_GPL(uprobe_unregister);
> > >   * refcount is released when the last @uc for the @uprobe
> > >   * unregisters. Caller of uprobe_register() is required to keep @inode
> > >   * (and the containing mount) referenced.
> > > - *
> > > - * Return errno if it cannot successully install probes
> > > - * else return 0 (success)
> >
> > mention that it never returns NULL, but rather encodes error code
> > inside the pointer on error? It's an important part of the contract.
>
> OK...
>
> > >  /*
> >
> > this should be /** for doccomment checking (you'd get a warning for
> > missing @uprobe if there was this extra star)
>
> Well, this is what we have before this patch, but OK
>
> > >   * uprobe_apply - unregister an already registered probe.
> > > - * @inode: the file in which the probe has to be removed.
> > > - * @offset: offset from the start of the file.
> >
> > add @uprobe description now?
>
> If only I knew what this @uprobe description can say ;)

I'm pointing this out because I accidentally used /** for comment for
some function, and I got some bot report about missing argument. I
think /** makes sense for documenting "public API" function, so which
is why all the above.

>
> > > @@ -3180,10 +3181,8 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
> > >  {
> > >         u32 i;
> > >
> > > -       for (i = 0; i < cnt; i++) {
> > > -               uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
> > > -                                 &uprobes[i].consumer);
> > > -       }
> > > +       for (i = 0; i < cnt; i++)
> >
> > you'll now need !IS_ERR_OR_NULL(uprobes[i].uprobe) check (or just NULL
> > check if you null-out it below)
>
> Hmm... are you sure? I'll re-check... See also the end of my email.

no, you are right, it should be fine

>
> > > @@ -3477,11 +3476,12 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > >                       &bpf_uprobe_multi_link_lops, prog);
> > >
> > >         for (i = 0; i < cnt; i++) {
> > > -               err = uprobe_register(d_real_inode(link->path.dentry),
> > > +               uprobes[i].uprobe = uprobe_register(d_real_inode(link->path.dentry),
> >
> > will uprobe keep inode alive as long as uprobe is attached?
>
> Why? No, this patch doesn't (shouldn't) change the current behaviour.
>
> The users still need kern_path/path_put to, say, prevent umount.

ok, then link->path has to stay, I was just wondering if we need to
keep it alive

>
> > we can NULL-out uprobe on error for bpf_uprobe_unregister() to handle
> > only NULL vs non-NULL case
>
> Not sure I understand...
>
> > or maybe better yet let's just have local struct uprobe variable and
> > only assign it if registration succeeded
>
> We can, but
>
> > (still need NULL check in
> > bpf_uprobe_unregister above)
>
> Why?
>
> If bpf_uprobe_unregister() is called by bpf_uprobe_multi_link_attach() on
> error, it passes cnt = i where is the 1st failed uprobe index. IOW,
> uptobes[i].uprobe can't be IS_ERR_OR_NULL() in the 0..cnt-1 range.
>
> If it is called by bpf_uprobe_multi_link_release() then the whole
> 0..umulti_link->cnt-1 range should be fine?

yes, you are absolutely right, I missed that for this partial failure
case we pass i as count into bpf_uprobe_unregister(), sorry about the
noise!


please feel free to add my ack for the next revision:

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

>
> Oleg.
>