Re: [PATCH RFC bpf-next 00/10] bpf: CO-RE support in the kernel.

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Lorenz Bauer <lmb@cloudflare.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	mcroce@microsoft.com, bpf <bpf@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH RFC bpf-next 00/10] bpf: CO-RE support in the kernel.
Date: Thu, 23 Sep 2021 11:52:06 -0700	[thread overview]
Message-ID: <CAEf4Bzbi_zZZndM93pWXn=Yu=bXQi6TWP8=pherCCtP5ZseqMA@mail.gmail.com> (raw)
In-Reply-To: <CACAyw98puHhO7f=OmEACNaje0DvVdpS7FosLY9aM8z46hy=7ww@mail.gmail.com>

On Thu, Sep 23, 2021 at 4:34 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Fri, 17 Sept 2021 at 22:57, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Hi All,
> >
> > This is very early RFC that introduces CO-RE support in the kernel.
> > There are several reasons to add such support:
> > 1. It's a step toward signed BPF programs.
> > 2. It allows golang like languages that struggle to adopt libbpf
> >    to take advantage of CO-RE powers.
> > 3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
> >    by the verifier purely based on +10 offset. If R1 points to a union
> >    the verifier picks one of the fields at this offset.
> >    With CO-RE the kernel can disambiguate the field access.
> >
> > This set wires all relevant pieces and passes two selftests with CO-RE
> > in the kernel.
> >
> > The main goal of RFC is to get feedback on patch 3.
> > It's passing CO-RE relocation into the kernel via bpf_core_apply_relo()
> > helper that is called by the loader program.
> > It works, but there is no clean way to add error reporting here.
> > So I'm thinking that the better approach would be to pass an array
> > of 'struct bpf_core_relo_desc' into PROG_LOAD command similar to
> > how func_info and line_info are passed.
> > Such approach would allow for the use case 3 above (which
> > current approach in patch 3 doesn't support).
>
> +1 to having good error reporting, it's hard to debug CO-RE failures
> as they are. PROG_LOAD seems nice, provided that relocation happens
> before verification.
>
> Some questions:
> * How can this handle kernels that don't have built-in BTF? Not a
> problem for myself, but some people have to deal with BTF-less distro
> kernels by using pahole to generate external BTF from debug symbols.
> Can we accommodate that?
> * Does in-kernel CO-RE need to account for packed structs w/ bitfields
> in them? If relocation happens after verification this could be a
> problem: [1].

The way that CO-RE relocs for bitfields are implemented with libbpf is
through the use of 5 different relocation kinds. See
BPF_CORE_READ_BITFIELD() macro in bpf_core_read.h.

> * Tangent: libbpf CO-RE has this res->validate flag, which turns off
> some checks for bitfields. I've never fully understood why that is,
> maybe Andrii can explain it?

Because there is no single canonical set of those 5 relocated values
(that I mentioned above) that the compiler has to general. There are
many ways to extract a bitfield and compiler can use different ones
due to optimization and code generation choices. So in general it's
ambiguous and impossible to validate that we agree with the compiler.
Generally we won't agree, but will still do correct bitfield
relocation. Again, I'll refer you to BPF_CORE_READ_BITFIELD() macro
for details.

>
> Lorenz
>
> 1: https://lore.kernel.org/bpf/CACAyw9_R4_ib0KvcuQC4nSOy5+Hn8-Xq-G8geDdLsNztX=0Fsw@mail.gmail.com/
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com