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

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

[Quentin Monnet]

2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> 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@fb.com>
> ---

[...]

> 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.

> +		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?

[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";
> 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) */

[...]

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

[Andrii Nakryiko]

On Wed, Jun 17, 2020 at 5:24 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> > 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@fb.com>
> > ---
>
> [...]
>
> > 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) */
>
> [...]
>

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

[Quentin Monnet]

2020-06-17 23:01 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Jun 17, 2020 at 5:24 PM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
>>> 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@fb.com>
>>> ---
>>
>> [...]
>>
>>> 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?

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

[Andrii Nakryiko]

On Thu, Jun 18, 2020 at 12:51 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2020-06-17 23:01 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Wed, Jun 17, 2020 at 5:24 PM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> >>> 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@fb.com>
> >>> ---
> >>
> >> [...]
> >>
> >>> 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.

Re: [PATCH bpf-next 9/9] tools/bpftool: add documentation and sample output for process info

[Quentin Monnet]

2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> Add statements about bpftool being able to discover process info, holding
> reference to BPF map, prog, link, or BTF. Show example output as well.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-btf.rst  |  5 +++++
>  tools/bpf/bpftool/Documentation/bpftool-link.rst | 13 ++++++++++++-
>  tools/bpf/bpftool/Documentation/bpftool-map.rst  |  8 +++++++-
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst | 11 +++++++++++
>  4 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> index ce3a724f50c1..85f7c82ebb28 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> @@ -36,6 +36,11 @@ DESCRIPTION
>  		  otherwise list all BTF objects currently loaded on the
>  		  system.
>  
> +		  Since Linux 5.8 bpftool is able to discover information about
> +		  processes that hold open file descriptors (FDs) against BPF
> +		  links. On such kernels bpftool will automatically emit this

Copy-paste error: s/BPF links/BTF objects/

> +		  information as well.
> +
>  	**bpftool btf dump** *BTF_SRC*
>  		  Dump BTF entries from a given *BTF_SRC*.
>  
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-link.rst b/tools/bpf/bpftool/Documentation/bpftool-link.rst
> index 0e43d7b06c11..1da7ef65b514 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-link.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-link.rst
> @@ -37,6 +37,11 @@ DESCRIPTION
>  		  zero or more named attributes, some of which depend on type
>  		  of link.
>  
> +		  Since Linux 5.8 bpftool is able to discover information about
> +		  processes that hold open file descriptors (FDs) against BPF
> +		  links. On such kernels bpftool will automatically emit this
> +		  information as well.
> +
>  	**bpftool link pin** *LINK* *FILE*
>  		  Pin link *LINK* as *FILE*.
>  
> @@ -82,6 +87,7 @@ EXAMPLES
>  
>      10: cgroup  prog 25
>              cgroup_id 614  attach_type egress
> +            pids test_progs(2238417)

(That's a big PID. Maybe something below the default max pid (32768)
might be less confusing for users, but also maybe that's just me
nitpicking too much.)

>  
>  **# bpftool --json --pretty link show**
>  
> @@ -91,7 +97,12 @@ EXAMPLES
>              "type": "cgroup",
>              "prog_id": 25,
>              "cgroup_id": 614,
> -            "attach_type": "egress"
> +            "attach_type": "egress",
> +            "pids": [{
> +                    "pid": 2238417,
> +                    "comm": "test_progs"
> +                }
> +            ]
>          }
>      ]
>  

Re: [PATCH bpf-next 9/9] tools/bpftool: add documentation and sample output for process info

[Andrii Nakryiko]

On Wed, Jun 17, 2020 at 5:25 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> > Add statements about bpftool being able to discover process info, holding
> > reference to BPF map, prog, link, or BTF. Show example output as well.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/bpf/bpftool/Documentation/bpftool-btf.rst  |  5 +++++
> >  tools/bpf/bpftool/Documentation/bpftool-link.rst | 13 ++++++++++++-
> >  tools/bpf/bpftool/Documentation/bpftool-map.rst  |  8 +++++++-
> >  tools/bpf/bpftool/Documentation/bpftool-prog.rst | 11 +++++++++++
> >  4 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > index ce3a724f50c1..85f7c82ebb28 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
> > @@ -36,6 +36,11 @@ DESCRIPTION
> >                 otherwise list all BTF objects currently loaded on the
> >                 system.
> >
> > +               Since Linux 5.8 bpftool is able to discover information about
> > +               processes that hold open file descriptors (FDs) against BPF
> > +               links. On such kernels bpftool will automatically emit this
>
> Copy-paste error: s/BPF links/BTF objects/
>

oops, will fix

> > +               information as well.
> > +
> >       **bpftool btf dump** *BTF_SRC*
> >                 Dump BTF entries from a given *BTF_SRC*.
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-link.rst b/tools/bpf/bpftool/Documentation/bpftool-link.rst
> > index 0e43d7b06c11..1da7ef65b514 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-link.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-link.rst
> > @@ -37,6 +37,11 @@ DESCRIPTION
> >                 zero or more named attributes, some of which depend on type
> >                 of link.
> >
> > +               Since Linux 5.8 bpftool is able to discover information about
> > +               processes that hold open file descriptors (FDs) against BPF
> > +               links. On such kernels bpftool will automatically emit this
> > +               information as well.
> > +
> >       **bpftool link pin** *LINK* *FILE*
> >                 Pin link *LINK* as *FILE*.
> >
> > @@ -82,6 +87,7 @@ EXAMPLES
> >
> >      10: cgroup  prog 25
> >              cgroup_id 614  attach_type egress
> > +            pids test_progs(2238417)
>
> (That's a big PID. Maybe something below the default max pid (32768)
> might be less confusing for users, but also maybe that's just me
> nitpicking too much.)

heh, real system, but yeah, I can make up a smaller PID :)

>
> >
> >  **# bpftool --json --pretty link show**
> >
> > @@ -91,7 +97,12 @@ EXAMPLES
> >              "type": "cgroup",
> >              "prog_id": 25,
> >              "cgroup_id": 614,
> > -            "attach_type": "egress"
> > +            "attach_type": "egress",
> > +            "pids": [{
> > +                    "pid": 2238417,
> > +                    "comm": "test_progs"
> > +                }
> > +            ]
> >          }
> >      ]
> >

Re: [PATCH bpf-next 6/9] tools/bpftool: generalize BPF skeleton support and generate vmlinux.h

[Quentin Monnet]

2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> Adapt Makefile to support BPF skeleton generation beyond single profiler.bpf.c
> case. Also add vmlinux.h generation and switch profiler.bpf.c to use it.
> 
> clang-bpf-global-var feature is extended and renamed to clang-bpf-co-re to
> check for support of preserve_access_index attribute, which, together with BTF
> for global variables, is the minimum requirement for modern BPF programs.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Reviewed-by: Quentin Monnet <quentin@isovalent.com>

Re: [PATCH bpf-next 5/9] tools/bpftool: minimize bootstrap bpftool

[Quentin Monnet]

2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> Build minimal "bootstrap mode" bpftool to enable skeleton (and, later,
> vmlinux.h generation), instead of building almost complete, but slightly
> different (w/o skeletons, etc) bpftool to bootstrap complete bpftool build.
> 
> Current approach doesn't scale well (engineering-wise) when adding more BPF
> programs to bpftool and other complicated functionality, as it requires
> constant adjusting of the code to work in both bootstrapped mode and normal
> mode.
> 
> So it's better to build only minimal bpftool version that supports only BPF
> skeleton code generation and BTF-to-C conversion. Thankfully, this is quite
> easy to accomplish due to internal modularity of bpftool commands. This will
> also allow to keep adding new functionality to bpftool in general, without the
> need to care about bootstrap mode for those new parts of bpftool.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Reviewed-by: Quentin Monnet <quentin@isovalent.com>

Re: [PATCH bpf-next 4/9] tools/bpftool: move map/prog parsing logic into common

[Quentin Monnet]

2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
> Move functions that parse map and prog by id/tag/name/etc outside of
> map.c/prog.c, respectively. These functions are used outside of those files
> and are generic enough to be in common. This also makes heavy-weight map.c and
> prog.c more decoupled from the rest of bpftool files and facilitates more
> lightweight bootstrap bpftool variant.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Reviewed-by: Quentin Monnet <quentin@isovalent.com>

Re: [PATCH bpf-next 1/9] libbpf: generalize libbpf externs support

[Hao Luo]

On Wed, Jun 17, 2020 at 9:21 AM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Switch existing Kconfig externs to be just one of few possible kinds of more
> generic externs. This refactoring is in preparation for ksymbol extern
> support, added in the follow up patch. There are no functional changes
> intended.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---

[...]

> @@ -5572,30 +5635,33 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>  {
>         bool need_config = false;
>         struct extern_desc *ext;
> +       void *kcfg_data;
>         int err, i;
> -       void *data;
>
>         if (obj->nr_extern == 0)
>                 return 0;
>
> -       data = obj->maps[obj->kconfig_map_idx].mmaped;
> +       if (obj->kconfig_map_idx >= 0)
> +               kcfg_data = obj->maps[obj->kconfig_map_idx].mmaped;
>
>         for (i = 0; i < obj->nr_extern; i++) {
>                 ext = &obj->externs[i];
>
> -               if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> -                       void *ext_val = data + ext->data_off;
> +               if (ext->type == EXT_KCFG &&
> +                   strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> +                       void *ext_val = kcfg_data + ext->kcfg.data_off;
>                         __u32 kver = get_kernel_version();
>
>                         if (!kver) {
>                                 pr_warn("failed to get kernel version\n");
>                                 return -EINVAL;
>                         }
> -                       err = set_ext_value_num(ext, ext_val, kver);
> +                       err = set_kcfg_value_num(ext, ext_val, kver);
>                         if (err)
>                                 return err;
> -                       pr_debug("extern %s=0x%x\n", ext->name, kver);
> -               } else if (strncmp(ext->name, "CONFIG_", 7) == 0) {
> +                       pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> +               } else if (ext->type == EXT_KCFG &&
> +                          strncmp(ext->name, "CONFIG_", 7) == 0) {
>                         need_config = true;
>                 } else {
>                         pr_warn("unrecognized extern '%s'\n", ext->name);

Ah, we need to initialize kcfg_data, otherwise the compiler will give
a warning on potentially uninitialized data.

Re: [PATCH bpf-next 1/9] libbpf: generalize libbpf externs support

[Andrii Nakryiko]

On Thu, Jun 18, 2020 at 12:38 AM Hao Luo <haoluo@google.com> wrote:
>
> On Wed, Jun 17, 2020 at 9:21 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Switch existing Kconfig externs to be just one of few possible kinds of more
> > generic externs. This refactoring is in preparation for ksymbol extern
> > support, added in the follow up patch. There are no functional changes
> > intended.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
>
> [...]
>
> > @@ -5572,30 +5635,33 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
> >  {
> >         bool need_config = false;
> >         struct extern_desc *ext;
> > +       void *kcfg_data;
> >         int err, i;
> > -       void *data;
> >
> >         if (obj->nr_extern == 0)
> >                 return 0;
> >
> > -       data = obj->maps[obj->kconfig_map_idx].mmaped;
> > +       if (obj->kconfig_map_idx >= 0)
> > +               kcfg_data = obj->maps[obj->kconfig_map_idx].mmaped;
> >
> >         for (i = 0; i < obj->nr_extern; i++) {
> >                 ext = &obj->externs[i];
> >
> > -               if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > -                       void *ext_val = data + ext->data_off;
> > +               if (ext->type == EXT_KCFG &&
> > +                   strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > +                       void *ext_val = kcfg_data + ext->kcfg.data_off;
> >                         __u32 kver = get_kernel_version();
> >
> >                         if (!kver) {
> >                                 pr_warn("failed to get kernel version\n");
> >                                 return -EINVAL;
> >                         }
> > -                       err = set_ext_value_num(ext, ext_val, kver);
> > +                       err = set_kcfg_value_num(ext, ext_val, kver);
> >                         if (err)
> >                                 return err;
> > -                       pr_debug("extern %s=0x%x\n", ext->name, kver);
> > -               } else if (strncmp(ext->name, "CONFIG_", 7) == 0) {
> > +                       pr_debug("extern (kcfg) %s=0x%x\n", ext->name, kver);
> > +               } else if (ext->type == EXT_KCFG &&
> > +                          strncmp(ext->name, "CONFIG_", 7) == 0) {
> >                         need_config = true;
> >                 } else {
> >                         pr_warn("unrecognized extern '%s'\n", ext->name);
>
> Ah, we need to initialize kcfg_data, otherwise the compiler will give
> a warning on potentially uninitialized data.

yep, good catch