netdev - Re: [PATCH v2 bpf-next 14/16] libbpf: Generate loader program out of BPF ELF file.

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: <20210428014222.7m5ndlmtbn4msj7y@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 27 Apr 2021 18:42:22 -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 14/16] libbpf: Generate loader program out of
 BPF ELF file.

On Tue, Apr 27, 2021 at 10:34:50AM -0700, Andrii Nakryiko wrote:
> > > > +void bpf_gen__load_btf(struct bpf_gen *gen, const void *btf_raw_data, __u32 btf_raw_size)
> > > > +{
> > > > +       union bpf_attr attr = {};
> > >
> > > here and below: memset(0)?
> >
> > that's unnecessary. there is no backward/forward compat issue here.
> > and bpf_attr doesn't have gaps inside.
> 
> systemd definitely had a problem with non-zero padding with such usage
> of bpf_attr recently, but I don't remember which command specifically.
> Is there any downside to making sure that this will keep working for
> later bpf_attr changes regardless of whether there are gaps or not?

I'd like to avoid cargo culting memset where it's not needed,
but thinking more about it...
I'll switch to memset(, cmd_specific_attr_size) instead.
I wanted to do this optimization forever in the rest of libbpf.
That would be a starting place.
Eventually when bpf.c will migrate into bpf.h I will convert it to
memset(, attr_size) too.
The bpf_attr is too big to do full zeroing.
Loader gen already taking advantage of that with partial bpf_attr for everything.

> > unfortunately there are various piece of the code that check <0
> > means not init.
> > I can try to fix them all, but it felt unncessary complex vs screwing up stdin.
> > It's bpftool's stdin, but you're right that it's not pretty.
> > I can probably use special very large FD number and check for it.
> > Still cleaner than fixing all checks.
> 
> Maybe after generation go over each prog/map/BTF and reset their FDs
> to -1 if gen_loader is used? Or I guess we can just sprinkle
> 
> if (!obj->gen_loader)
>     close(fd);
> 
> in the right places. Not great from code readability, but at least
> won't have spurious close()s.

Interesting ideas. I haven't thought about reseting back to -1.
Will do and address the rest of comments too.