On Wed, Jun 17, 2020 at 5:24 PM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@xxxxxx>
> > Add bpf_iter-based way to find all the processes that hold open FDs against
> > BPF object (map, prog, link, btf). bpftool always attempts to discover this,
> > but will silently give up if kernel doesn't yet support bpf_iter BPF programs.
> > Process name and PID are emitted for each process (task group).
> >
> > Sample output for each of 4 BPF objects:
> >
> > $ sudo ./bpftool prog show
> > 2694: cgroup_device tag 8c42dee26e8cd4c2 gpl
> > loaded_at 2020-06-16T15:34:32-0700 uid 0
> > xlated 648B jited 409B memlock 4096B
> > pids systemd(1)
> > 2907: cgroup_skb name egress tag 9ad187367cf2b9e8 gpl
> > loaded_at 2020-06-16T18:06:54-0700 uid 0
> > xlated 48B jited 59B memlock 4096B map_ids 2436
> > btf_id 1202
> > pids test_progs(2238417), test_progs(2238445)
> >
> > $ sudo ./bpftool map show
> > 2436: array name test_cgr.bss flags 0x400
> > key 4B value 8B max_entries 1 memlock 8192B
> > btf_id 1202
> > pids test_progs(2238417), test_progs(2238445)
> > 2445: array name pid_iter.rodata flags 0x480
> > key 4B value 4B max_entries 1 memlock 8192B
> > btf_id 1214 frozen
> > pids bpftool(2239612)
> >
> > $ sudo ./bpftool link show
> > 61: cgroup prog 2908
> > cgroup_id 375301 attach_type egress
> > pids test_progs(2238417), test_progs(2238445)
> > 62: cgroup prog 2908
> > cgroup_id 375344 attach_type egress
> > pids test_progs(2238417), test_progs(2238445)
> >
> > $ sudo ./bpftool btf show
> > 1202: size 1527B prog_ids 2908,2907 map_ids 2436
> > pids test_progs(2238417), test_progs(2238445)
> > 1242: size 34684B
> > pids bpftool(2258892)
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
>
> [...]
>
> > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> > new file mode 100644
> > index 000000000000..3474a91743ff
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/pids.c
> > @@ -0,0 +1,229 @@
>
> [...]
>
> > +int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type)
> > +{
> > + char buf[4096];
> > + struct pid_iter_bpf *skel;
> > + struct pid_iter_entry *e;
> > + int err, ret, fd = -1, i;
> > + libbpf_print_fn_t default_print;
> > +
> > + hash_init(table->table);
> > + set_max_rlimit();
> > +
> > + skel = pid_iter_bpf__open();
> > + if (!skel) {
> > + p_err("failed to open PID iterator skeleton");
> > + return -1;
> > + }
> > +
> > + skel->rodata->obj_type = type;
> > +
> > + /* we don't want output polluted with libbpf errors if bpf_iter is not
> > + * supported
> > + */
> > + default_print = libbpf_set_print(libbpf_print_none);
> > + err = pid_iter_bpf__load(skel);
> > + libbpf_set_print(default_print);
> > + if (err) {
> > + /* too bad, kernel doesn't support BPF iterators yet */
> > + err = 0;
> > + goto out;
> > + }
> > + err = pid_iter_bpf__attach(skel);
> > + if (err) {
> > + /* if we loaded above successfully, attach has to succeed */
> > + p_err("failed to attach PID iterator: %d", err);
>
> Nit: What about using strerror(err) for the error messages, here and
> below? It's easier to read than an integer value.
I'm actually against it. Just a pure string message for error is often
quite confusing. It's an extra step, and sometimes quite a quest in
itself, to find what's the integer value of errno it was, just to try
to understand what kind of error it actually is. So I certainly prefer
having integer value, optionally with a string error message.
But that's too much hassle for this "shouldn't happen" type of errors.
If they happen, the user is unlikely to infer anything useful and fix
the problem by themselves. They will most probably have to ask on the
mailing list and paste error messages verbatim and let people like me
and you try to guess what's going on. In such cases, having an errno
number is much more helpful.
>
> > + goto out;
> > + }
> > +
> > + fd = bpf_iter_create(bpf_link__fd(skel->links.iter));
> > + if (fd < 0) {
> > + err = -errno;
> > + p_err("failed to create PID iterator session: %d", err);
> > + goto out;
> > + }
> > +
> > + while (true) {
> > + ret = read(fd, buf, sizeof(buf));
> > + if (ret < 0) {
> > + err = -errno;
> > + p_err("failed to read PID iterator output: %d", err);
> > + goto out;
> > + }
> > + if (ret == 0)
> > + break;
> > + if (ret % sizeof(*e)) {
> > + err = -EINVAL;
> > + p_err("invalid PID iterator output format");
> > + goto out;
> > + }
> > + ret /= sizeof(*e);
> > +
> > + e = (void *)buf;
> > + for (i = 0; i < ret; i++, e++) {
> > + add_ref(table, e);
> > + }
> > + }
> > + err = 0;
> > +out:
> > + if (fd >= 0)
> > + close(fd);
> > + pid_iter_bpf__destroy(skel);
> > + return err;
> > +}
>
> [...]
>
> > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > new file mode 100644
> > index 000000000000..f560e48add07
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> This would make it the only file not dual-licensed GPL/BSD in bpftool.
> We've had issues with that before [0], although linking to libbfd is no
> more a hard requirement. But I see you used a dual-license in the
> corresponding header file pid_iter.h, so is the single license
> intentional here? Or would you consider GPL/BSD?
>
The other BPF program (skeleton/profiler.bpf.c) is also GPL-2.0, we
should probably switch both.
> [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=896165#38
>
> > +// Copyright (c) 2020 Facebook
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_core_read.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include "pid_iter.h"
>
> [...]
>
> > +
> > +char LICENSE[] SEC("license") = "GPL";
I wonder if leaving this as GPL would be ok, if the source code itself
is dual GPL/BSD?
> > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
> > new file mode 100644
> > index 000000000000..5692cf257adb
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/skeleton/pid_iter.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>
> [...]
>
