From: Jiri Olsa <olsajiri@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
bpf@vger.kernel.org, Martin KaFai Lau <kafai@fb.com>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>
Subject: Re: [PATCHv2 bpf-next 01/24] bpf: Add multi uprobe link
Date: Fri, 23 Jun 2023 10:19:21 +0200 [thread overview]
Message-ID: <ZJVViQEvUnMQN43b@krava> (raw)
In-Reply-To: <CAEf4BzZnn-m-5sTQEmCSyaQPNNyreE37Vu2GWtdLT=k+zR+kDA@mail.gmail.com>
On Thu, Jun 22, 2023 at 05:18:05PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 20, 2023 at 1:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding new multi uprobe link that allows to attach bpf program
> > to multiple uprobes.
> >
> > Uprobes to attach are specified via new link_create uprobe_multi
> > union:
> >
> > struct {
> > __u32 flags;
> > __u32 cnt;
> > __aligned_u64 path;
> > __aligned_u64 offsets;
> > __aligned_u64 ref_ctr_offsets;
> > } uprobe_multi;
> >
> > Uprobes are defined for single binary specified in path and multiple
> > calling sites specified in offsets array with optional reference
> > counters specified in ref_ctr_offsets array. All specified arrays
> > have length of 'cnt'.
> >
> > The 'flags' supports single bit for now that marks the uprobe as
> > return probe.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > include/linux/trace_events.h | 6 +
> > include/uapi/linux/bpf.h | 14 ++
> > kernel/bpf/syscall.c | 12 +-
> > kernel/trace/bpf_trace.c | 237 +++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 14 ++
> > 5 files changed, 281 insertions(+), 2 deletions(-)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index a75c54b6f8a3..a96e46cd407e 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3516,6 +3516,11 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> > return prog->enforce_expected_attach_type &&
> > prog->expected_attach_type != attach_type ?
> > -EINVAL : 0;
> > + case BPF_PROG_TYPE_KPROBE:
> > + if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI &&
> > + attach_type != BPF_TRACE_KPROBE_MULTI)
>
> should this be UPROBE_MULTI? this looks like your recent bug fix,
> which already landed
>
> > + return -EINVAL;
> > + fallthrough;
>
> and I replaced this with `return 0;` ;)
ugh, yes, will fix
> > default:
> > return 0;
> > }
> > @@ -4681,7 +4686,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
> > break;
> > case BPF_PROG_TYPE_KPROBE:
> > if (attr->link_create.attach_type != BPF_PERF_EVENT &&
> > - attr->link_create.attach_type != BPF_TRACE_KPROBE_MULTI) {
> > + attr->link_create.attach_type != BPF_TRACE_KPROBE_MULTI &&
> > + attr->link_create.attach_type != BPF_TRACE_UPROBE_MULTI) {
> > ret = -EINVAL;
> > goto out;
> > }
>
> should this be moved into bpf_prog_attach_check_attach_type() and
> unify these checks?
ok, perhaps we could move there the whole switch, will check
>
> > @@ -4748,8 +4754,10 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
> > case BPF_PROG_TYPE_KPROBE:
> > if (attr->link_create.attach_type == BPF_PERF_EVENT)
> > ret = bpf_perf_link_attach(attr, prog);
> > - else
> > + else if (attr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI)
> > ret = bpf_kprobe_multi_link_attach(attr, prog);
> > + else if (attr->link_create.attach_type == BPF_TRACE_UPROBE_MULTI)
> > + ret = bpf_uprobe_multi_link_attach(attr, prog);
> > break;
> > default:
> > ret = -EINVAL;
>
> [...]
>
> > +static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
> > + u32 cnt)
> > +{
> > + u32 i;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
> > + &uprobes[i].consumer);
> > + }
> > +}
> > +
> > +static void bpf_uprobe_multi_link_release(struct bpf_link *link)
> > +{
> > + struct bpf_uprobe_multi_link *umulti_link;
> > +
> > + umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
> > + bpf_uprobe_unregister(&umulti_link->path, umulti_link->uprobes, umulti_link->cnt);
> > + path_put(&umulti_link->path);
> > +}
> > +
> > +static void bpf_uprobe_multi_link_dealloc(struct bpf_link *link)
> > +{
> > + struct bpf_uprobe_multi_link *umulti_link;
> > +
> > + umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
> > + kvfree(umulti_link->uprobes);
> > + kfree(umulti_link);
> > +}
> > +
> > +static const struct bpf_link_ops bpf_uprobe_multi_link_lops = {
> > + .release = bpf_uprobe_multi_link_release,
> > + .dealloc = bpf_uprobe_multi_link_dealloc,
> > +};
> > +
> > +static int uprobe_prog_run(struct bpf_uprobe *uprobe,
> > + unsigned long entry_ip,
> > + struct pt_regs *regs)
> > +{
> > + struct bpf_uprobe_multi_link *link = uprobe->link;
> > + struct bpf_uprobe_multi_run_ctx run_ctx = {
> > + .entry_ip = entry_ip,
> > + };
> > + struct bpf_prog *prog = link->link.prog;
> > + struct bpf_run_ctx *old_run_ctx;
> > + int err = 0;
> > +
> > + might_fault();
> > +
> > + rcu_read_lock_trace();
>
> we don't need this if uprobe is not sleepable, right? why unconditional then?
I won't pretend I understand what rcu_read_lock_trace does ;-)
I tried to follow bpf_prog_run_array_sleepable where it's called
unconditionally for both sleepable and non-sleepable progs
there are conditional rcu_read_un/lock calls later on
I will check
>
> > + migrate_disable();
> > +
> > + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1))
> > + goto out;
> > +
> > + old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
> > +
> > + if (!prog->aux->sleepable)
> > + rcu_read_lock();
> > +
> > + err = bpf_prog_run(link->link.prog, regs);
> > +
> > + if (!prog->aux->sleepable)
> > + rcu_read_unlock();
> > +
> > + bpf_reset_run_ctx(old_run_ctx);
> > +
> > +out:
> > + __this_cpu_dec(bpf_prog_active);
> > + migrate_enable();
> > + rcu_read_unlock_trace();
> > + return err;
> > +}
> > +
>
> [...]
>
> > +
> > + err = kern_path(name, LOOKUP_FOLLOW, &path);
> > + kfree(name);
> > + if (err)
> > + return err;
> > +
> > + if (!d_is_reg(path.dentry)) {
> > + err = -EINVAL;
> > + goto error_path_put;
> > + }
> > +
> > + err = -ENOMEM;
> > +
> > + link = kzalloc(sizeof(*link), GFP_KERNEL);
> > + uprobes = kvcalloc(cnt, sizeof(*uprobes), GFP_KERNEL);
> > + ref_ctr_offsets = kvcalloc(cnt, sizeof(*ref_ctr_offsets), GFP_KERNEL);
>
> ref_ctr_offsets is optional, but we'll unconditionally allocate this array?
true :-\ will add the uref_ctr_offsets check
>
> > +
> > + if (!uprobes || !ref_ctr_offsets || !link)
> > + goto error_free;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + if (uref_ctr_offsets && __get_user(ref_ctr_offset, uref_ctr_offsets + i)) {
> > + err = -EFAULT;
> > + goto error_free;
> > + }
> > + if (__get_user(offset, uoffsets + i)) {
> > + err = -EFAULT;
> > + goto error_free;
> > + }
> > +
> > + uprobes[i].offset = offset;
> > + uprobes[i].link = link;
> > +
> > + if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > + uprobes[i].consumer.ret_handler = uprobe_multi_link_ret_handler;
> > + else
> > + uprobes[i].consumer.handler = uprobe_multi_link_handler;
> > +
> > + ref_ctr_offsets[i] = ref_ctr_offset;
> > + }
> > +
> > + link->cnt = cnt;
> > + link->uprobes = uprobes;
> > + link->path = path;
> > +
> > + bpf_link_init(&link->link, BPF_LINK_TYPE_UPROBE_MULTI,
> > + &bpf_uprobe_multi_link_lops, prog);
> > +
> > + err = bpf_link_prime(&link->link, &link_primer);
> > + if (err)
> > + goto error_free;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> > + uprobes[i].offset, ref_ctr_offsets[i],
> > + &uprobes[i].consumer);
> > + if (err) {
> > + bpf_uprobe_unregister(&path, uprobes, i);
>
> bpf_link_cleanup() will do this through
> bpf_uprobe_multi_link_release(), no? So you are double unregistering?
> Either drop cnt to zero, or just don't do this here? Latter is better,
> IMO.
bpf_link_cleanup path won't call release callback so we have to do that
I think I can add simple selftest to have this path covered
thanks,
jirka
>
> > + bpf_link_cleanup(&link_primer);
> > + kvfree(ref_ctr_offsets);
> > + return err;
> > + }
> > + }
> > +
> > + kvfree(ref_ctr_offsets);
> > + return bpf_link_settle(&link_primer);
> > +
> > +error_free:
> > + kvfree(ref_ctr_offsets);
> > + kvfree(uprobes);
> > + kfree(link);
> > +error_path_put:
> > + path_put(&path);
> > + return err;
> > +}
> > +#else /* !CONFIG_UPROBES */
> > +int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > +{
> > + return -EOPNOTSUPP;
> > +}
>
> [...]
next prev parent reply other threads:[~2023-06-23 8:19 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-20 8:35 [PATCHv2 bpf-next 00/24] bpf: Add multi uprobe link Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 01/24] " Jiri Olsa
2023-06-20 17:11 ` Alexei Starovoitov
2023-06-21 8:32 ` Jiri Olsa
2023-06-23 0:18 ` Andrii Nakryiko
2023-06-23 8:19 ` Jiri Olsa [this message]
2023-06-23 16:24 ` Andrii Nakryiko
2023-06-23 16:39 ` Alexei Starovoitov
2023-06-23 17:11 ` Andrii Nakryiko
2023-06-23 17:20 ` Alexei Starovoitov
2023-06-25 1:19 ` Jiri Olsa
2023-06-25 1:18 ` Jiri Olsa
2023-06-26 18:27 ` Andrii Nakryiko
2023-06-26 19:23 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 02/24] bpf: Add cookies support for uprobe_multi link Jiri Olsa
2023-06-23 0:18 ` Andrii Nakryiko
2023-06-23 8:01 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 03/24] bpf: Add pid filter " Jiri Olsa
2023-06-20 12:40 ` Oleg Nesterov
2023-06-20 8:35 ` [PATCHv2 bpf-next 04/24] bpf: Add bpf_get_func_ip helper support for uprobe link Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 05/24] libbpf: Add uprobe_multi attach type and link names Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 06/24] libbpf: Add elf symbol iterator Jiri Olsa
2023-06-23 0:31 ` Andrii Nakryiko
2023-06-23 8:19 ` Jiri Olsa
2023-06-23 16:27 ` Andrii Nakryiko
2023-06-23 16:29 ` Andrii Nakryiko
2023-06-20 8:35 ` [PATCHv2 bpf-next 07/24] libbpf: Add open_elf/close_elf functions Jiri Olsa
2023-06-23 0:33 ` Andrii Nakryiko
2023-06-23 8:21 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 08/24] libbpf: Add elf_find_multi_func_offset function Jiri Olsa
2023-06-23 20:39 ` Andrii Nakryiko
2023-06-25 1:19 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 09/24] libbpf: Add elf_find_pattern_func_offset function Jiri Olsa
2023-06-23 20:39 ` Andrii Nakryiko
2023-06-25 1:19 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 10/24] libbpf: Add bpf_link_create support for multi uprobes Jiri Olsa
2023-06-23 20:40 ` Andrii Nakryiko
2023-06-20 8:35 ` [PATCHv2 bpf-next 11/24] libbpf: Add bpf_program__attach_uprobe_multi_opts function Jiri Olsa
2023-06-23 20:40 ` Andrii Nakryiko
2023-06-25 1:19 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 12/24] libbpf: Add support for u[ret]probe.multi[.s] program sections Jiri Olsa
2023-06-23 20:40 ` Andrii Nakryiko
2023-06-25 1:20 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 13/24] libbpf: Add uprobe multi link detection Jiri Olsa
2023-06-23 20:40 ` Andrii Nakryiko
2023-06-25 1:18 ` Jiri Olsa
2023-06-26 18:21 ` Andrii Nakryiko
2023-06-26 19:22 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 14/24] libbpf: Add uprobe multi link support to bpf_program__attach_usdt Jiri Olsa
2023-06-23 20:40 ` Andrii Nakryiko
2023-06-25 1:18 ` Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 15/24] selftests/bpf: Add uprobe_multi skel test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 16/24] selftests/bpf: Add uprobe_multi api test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 17/24] selftests/bpf: Add uprobe_multi link test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 18/24] selftests/bpf: Add uprobe_multi test program Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 19/24] selftests/bpf: Add uprobe_multi bench test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 20/24] selftests/bpf: Add usdt_multi test program Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 21/24] selftests/bpf: Add usdt_multi bench test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 22/24] selftests/bpf: Add uprobe_multi cookie test Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 23/24] selftests/bpf: Add uprobe_multi pid filter tests Jiri Olsa
2023-06-20 8:35 ` [PATCHv2 bpf-next 24/24] selftests/bpf: Add extra link to uprobe_multi tests Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZJVViQEvUnMQN43b@krava \
--to=olsajiri@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=sdf@google.com \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.