linux-kernel - Re: [PATCH 3/3] uprobes: make 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: <CAEf4BzaSDUWiSywUNrDtd-yW6p53vFYkZkr5mb461jmUgWV_2g@mail.gmail.com>
Date: Wed, 10 Jul 2024 11:23:10 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Oleg Nesterov <oleg@...hat.com>, andrii@...nel.org, mhiramat@...nel.org, 
	peterz@...radead.org, clm@...a.com, mingo@...nel.org, paulmck@...nel.org, 
	rostedt@...dmis.org, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] uprobes: make uprobe_register() return struct uprobe *

On Wed, Jul 10, 2024 at 9:49 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Wed, Jul 10, 2024 at 06:31:33PM +0200, Oleg Nesterov wrote:
>
> SNIP
>
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 467f358c8ce7..7571811127a2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3157,6 +3157,7 @@ struct bpf_uprobe {
> >       loff_t offset;
> >       unsigned long ref_ctr_offset;
> >       u64 cookie;
> > +     struct uprobe *uprobe;
> >       struct uprobe_consumer consumer;
> >  };
> >
> > @@ -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);
> > -     }
>
> nice, we could also drop path argument now

see my comments to Oleg, I think we can/should get rid of link->path
altogether if uprobe itself keeps inode alive.

BTW, Jiri, do we have any test for multi-uprobe that simulates partial
attachment success/failure (whichever way you want to look at it). It
would be super useful to have to check at least some error handling
code in the uprobe code base. If we don't, do you mind adding
something simple to BPF selftests?

>
> jirka
>
> > +     for (i = 0; i < cnt; i++)
> > +             uprobe_unregister(uprobes[i].uprobe, &uprobes[i].consumer);
> >  }
> >
> >  static void bpf_uprobe_multi_link_release(struct bpf_link *link)
> > @@ -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),
> >                                            uprobes[i].offset,
> >                                            uprobes[i].ref_ctr_offset,
> >                                            &uprobes[i].consumer);
> > -             if (err) {
> > +             if (IS_ERR(uprobes[i].uprobe)) {
> > +                     err = PTR_ERR(uprobes[i].uprobe);
> >                       bpf_uprobe_unregister(&path, uprobes, i);
> >                       goto error_free;
> >               }