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:   Wed, 24 Jun 2020 09:34:52 -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 Wed, Jun 24, 2020 at 7:52 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Jun 23, 2020 at 11:59:40PM -0700, Andrii Nakryiko wrote:
> > 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.
>
> None of the above were mentioned in the commit log.
> And no examples were given where this extra line would actually help.

I used it just 2 days ago trying to understand why bpftool doesn't
show its own bpf_iter program, but shows maps. I discovered with
surprise that we actually don't log FDs of loaded programs.

>
> I think libbpf pr_debug is extremely verbose instead of extremely useful.
> Just typical output:
> ./test_progs -vv -t lsm
> libbpf: loading object 'lsm' from buffer
> libbpf: section(1) .strtab, size 306, link 0, flags 0, type=3
> libbpf: skip section(1) .strtab
> libbpf: section(2) .text, size 0, link 0, flags 6, type=1
> libbpf: skip section(2) .text
> libbpf: section(3) lsm/file_mprotect, size 192, link 0, flags 6, type=1
> libbpf: found program lsm/file_mprotect
> libbpf: section(4) .rellsm/file_mprotect, size 32, link 25, flags 0, type=9
> libbpf: section(5) lsm/bprm_committed_creds, size 104, link 0, flags 6, type=1
> libbpf: found program lsm/bprm_committed_creds
> libbpf: section(6) .rellsm/bprm_committed_creds, size 32, link 25, flags 0, type=9
>
> How's above useful for anyone?
> libbpf says that there are '.strtab' and '.text' sections in the elf file.
> That's wet water. Any elf file has that.
> Then it says it's skipping '.text' ?
> That reads surprising. Why library would skip the code?
> And so on and so forth.

I can pick a few more not-so-useful (usually) pr_debug-level log lines
as well, I don't think it disproves that debug logs are useful.

> That output is useful to only few core libbpf developers.

Yes, and I don't expect typical BPF developers to have them turned on
by default. They are *DEBUG*-level output, after all, users shouldn't
care about them, only INFO and WARN/ERR ones, I'd hope. But it's #1
thing that I ask users to provide when they come with any questions
about BPF or libbpf.

So yeah, as a core libbpf developer and a person helping people with
various (often non-libbpf-specific) BPF problems both online and
within my company, I stand by my claim that libbpf debug logs are
extremely useful and helped debug and understand numerous issues.

Just yesterday (or two days ago, maybe), having those CO-RE relocation
logs, which I fought to keep when I added CO-RE relocs initially,
immediately shown that a person doesn't have bpf_iter compiled in its
running kernel, despite the claims otherwise.

>
> I don't mind more thought through debug prints, but
> saying that existing pr_debugs are 'extremely useful' is a stretch.

Some lines are extremely useful, yes, some less so. But then again,
depending on the situation. Not all parts of the log are relevant 100%
of the time, but sometimes even these ELF parsing logs are important.
How many people add and debug libbpf functionality that deals with
interpreting ELF sections/relocations/etc to be able to claim about
their usefulness anyway?

Regardless, we've spent way too much time on this, I don't care about
this particular pr_debug() enough to argue further.