Re: [PATCH bpf-next 13/15] libbpf: Generate loader program out of BPF ELF file.

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Yonghong Song <yhs@fb.com>,
	"David S. Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 13/15] libbpf: Generate loader program out of BPF ELF file.
Date: Wed, 21 Apr 2021 10:46:46 -0700	[thread overview]
Message-ID: <CAEf4BzY6AQznGOyfp1j54pp-9pJ_SpWZZo6GWMENnDng8aizig@mail.gmail.com> (raw)
In-Reply-To: <20210421044643.mqb4lnbqtgxmkcl4@ast-mbp.dhcp.thefacebook.com>

On Tue, Apr 20, 2021 at 9:46 PM Alexei Starovoitov
<alexei.starovoitov@gmail.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.

[...]