netdev - Re: [PATCH v2 bpf-next 09/16] libbpf: Support for fd

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]

Message-ID: <20210428013242.2iqeygfpmoyzwvxh@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 27 Apr 2021 18:32:42 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 09/16] libbpf: Support for fd_idx

On Tue, Apr 27, 2021 at 09:36:54AM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 26, 2021 at 7:53 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 10:14:45AM -0700, Andrii Nakryiko wrote:
> > > On Thu, Apr 22, 2021 at 5:27 PM Alexei Starovoitov
> > > <alexei.starovoitov@...il.com> wrote:
> > > >
> > > > From: Alexei Starovoitov <ast@...nel.org>
> > > >
> > > > Add support for FD_IDX make libbpf prefer that approach to loading programs.
> > > >
> > > > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> > > > ---
> > > >  tools/lib/bpf/bpf.c             |  1 +
> > > >  tools/lib/bpf/libbpf.c          | 70 +++++++++++++++++++++++++++++----
> > > >  tools/lib/bpf/libbpf_internal.h |  1 +
> > > >  3 files changed, 65 insertions(+), 7 deletions(-)
> > > >
> > >
> 
> [...]
> 
> > > >         for (i = 0; i < obj->nr_programs; i++) {
> > > >                 prog = &obj->programs[i];
> > > >                 if (prog_is_subprog(obj, prog))
> > > > @@ -7256,10 +7308,14 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> > > >                         continue;
> > > >                 }
> > > >                 prog->log_level |= log_level;
> > > > +               prog->fd_array = fd_array;
> > >
> > > you are not freeing this memory on success, as far as I can see.
> >
> > hmm. there is free on success below.
> 
> right, my bad, I somehow understood as if it was only for error case
> 
> >
> > > And
> > > given multiple programs are sharing fd_array, it's a bit problematic
> > > for prog to have fd_array. This is per-object properly, so let's add
> > > it at bpf_object level and clean it up on bpf_object__close()? And by
> > > assigning to obj->fd_array at malloc() site, you won't need to do all
> > > the error-handling free()s below.
> >
> > hmm. that sounds worse.
> > why add another 8 byte to bpf_object that won't be used
> > until this last step of bpf_object__load_progs.
> > And only for the duration of this loading.
> > It's cheaper to have this alloc here with two free()s below.
> 
> So if you care about extra 8 bytes, then it's even more efficient to
> have just one obj->fd_array rather than N prog->fd_array, no?

I think it's layer breaking when bpf_program__load()->load_program()
has to reach out to prog->obj to do its work.
The layers are already a mess due to:
&prog->obj->maps[prog->obj->rodata_map_idx]
I wanted to avoid making it uglier.

> And it's
> also not very clean that prog->fd_array will have a dangling pointer
> to deallocated memory after bpf_object__load_progs().

prog->reloc_desc is free and zeroed after __relocate.
prog->insns are freed and _not_ zereod after __load_progs.
so prog->fd_array won't be the first such pointer.
I can add zeroing, of course.

> 
> But that brings the entire question of why use fd_array at all here?
> Commit description doesn't explain why libbpf has to use fd_array and
> why it should be preferred. What are the advantages justifying added
> complexity and extra memory allocation/clean up? It also reduces test
> coverage of the "old ways" that offer the same capabilities. I think
> this should be part of the commit description, if we agree that
> fd_array has to be used outside of the auto-generated loader program.

I can add a knob to it to use it during loader gen for the loader gen
and for the runner of the loader prog.
I think it will add more complexity.
The bpf CI runs on older kernels, so the test coverage of "old ways"
is not reduced regardless.
>From the kernel pov BPF_PSEUDO_MAP_FD vs BPF_PSEUDO_MAP_IDX there is
no advantage.
>From the libbpf side patch 9 looked trivial enough _not_ do it conditionally,
but whatever. I don't mind more 'if'-s.