On Wed, Jul 10, 2024 at 1:18 PM Oleg Nesterov <o...@redhat.com> wrote:
>
> On 07/10, Andrii Nakryiko wrote:
> >
> > On Wed, Jul 10, 2024 at 9:33 AM Oleg Nesterov <o...@redhat.com> 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 <o...@redhat.com>
> > > ---
> > > 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 <and...@kernel.org>
>
> Oleg.
>
