Re: [PATCH bpf-next 8/9] tools/bpftool: show info for processes holding BPF map/prog/link/btf FDs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 18, 2020 at 12:51 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> 2020-06-17 23:01 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> > 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.
>
> Ok, fair enough.
>
> >>> +             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.
>
> Oh I missed that one :(. Yeah, if this is possible, that would be great!
>
> >> [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?
>
> If the concern is to pass the verifier, it accepts a handful of
> different strings (see include/linux/license.h), one of which is "Dual
> BSD/GPL" and should probably be used in that case. Or did you have
> something else in mind?

Oh, awesome, wasn't aware of this. I'll use "Dual BSD/GPL" then.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux