netdev - Re: [PATCH bpf-next] libbpf: add debug message for each created program

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]

Date:   Tue, 23 Jun 2020 23:59:40 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next] libbpf: add debug message for each created program

On Tue, Jun 23, 2020 at 11:47 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Jun 23, 2020 at 5:34 PM Andrii Nakryiko <andriin@...com> wrote:
> >
> > Similar message for map creation is extremely useful, so add similar for BPF
> > programs.
>
> 'extremely useful' is quite subjective.
> If we land this patch then everyone will be allowed to add pr_debug()
> everywhere in libbpf with the same reasoning: "it's extremely useful pr_debug".

We print this for maps, making it clear which maps and with which FD
were created. Having this for programs is just as useful. It doesn't
overwhelm output (and it's debug one either way). "everyone will be
allowed to add pr_debug()" is a big stretch, you can't just sneak in
or force random pr_debug, we do review patches and if something
doesn't make sense we can and we do reject it, regardless of claimed
usefulness by the patch author.

So far, libbpf debug logs were extremely helpful (subjective, of
course, but what isn't?) to debug "remotely" various issues that BPF
users had. They don't feel overwhelmingly verbose and don't have a lot
of unnecessary info. Adding a few lines (how many BPF programs are
there per each BPF object?) for listing BPF programs is totally ok.

But I'm not going to fight it, up to you, of course.

>
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> >  tools/lib/bpf/libbpf.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 18461deb1b19..f24a90c86c58 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5379,8 +5379,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >         }
> >
> >         ret = bpf_load_program_xattr(&load_attr, log_buf, log_buf_size);
> > -
> >         if (ret >= 0) {
> > +               pr_debug("prog '%s' ('%s'): created successfully, fd=%d\n",
> > +                        prog->name, prog->section_name, ret);
> >                 if (log_buf && load_attr.log_level)
> >                         pr_debug("verifier log:\n%s", log_buf);
> >                 *pfd = ret;
> > --
> > 2.24.1
> >