netdev - Re: [PATCH bpf-next 13/15] 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] [day] [month] [year] [list]

Date:   Wed, 21 Apr 2021 10:50:06 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Yonghong Song <yhs@...com>,
        "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 bpf-next 13/15] libbpf: Generate loader program out of BPF
 ELF file.

On Wed, Apr 21, 2021 at 10:46 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Apr 20, 2021 at 9:46 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Tue, Apr 20, 2021 at 06:34:11PM -0700, Yonghong Song wrote:
> > > >
> > > > kconfig, typeless ksym, struct_ops and CO-RE are not supported yet.
> > >
> > > Beyond this, currently libbpf has a lot of flexibility between prog open
> > > and load, change program type, key/value size, pin maps, max_entries, reuse
> > > map, etc. it is worthwhile to mention this in the cover letter.
> > > It is possible that these changes may defeat the purpose of signing the
> > > program though.
> >
> > Right. We'd need to decide which ones are ok to change after signature
> > verification. I think max_entries gotta be allowed, since tools
> > actively change it. The other fields selftest change too, but I'm not sure
> > it's a good thing to allow for signed progs. TBD.
> >
>
> [...]
>
> >
> > > > +static void mark_feat_supported(enum kern_feature_id last_feat)
> > > > +{
> > > > +   struct kern_feature_desc *feat;
> > > > +   int i;
> > > > +
> > > > +   for (i = 0; i <= last_feat; i++) {
> > > > +           feat = &feature_probes[i];
> > > > +           WRITE_ONCE(feat->res, FEAT_SUPPORTED);
> > > > +   }
> > >
> > > This assumes all earlier features than FD_IDX are supported. I think this is
> > > probably fine although it may not work for some weird backport.
> > > Did you see any issues if we don't explicitly set previous features
> > > supported?
> >
> > This helper is only used as mark_feat_supported(FEAT_FD_IDX)
> > to tell libbpf that it shouldn't probe anything.
> > Otherwise probing via prog_load screw up gen_trace completely.
> > May be it will be mark_all_feat_supported(void), but that seems less flexible.
>
>
> mark_feat_supported() is changing global state irreversibly, which is
> not great. I think it will be cleaner to just pass bpf_object * into
> kernel_supports() helper, and there return true if obj->gen_trace is
> set. That way it won't affect any other use cases that can happen in
> the same process (not that there are any right now, but still). I
> checked and in all current places there is obj available or it can be
> accessed through prog->obj, so this shouldn't be a problem.

sure. Will use that.