Re: [PATCH v5 bpf-next 05/17] bpf: Pass a set of bpf_core_relo-s to prog_load command.

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jakub Sitnicki <jakub@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>, bpf <bpf@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>,
	kernel-team <kernel-team@cloudflare.com>
Subject: Re: [PATCH v5 bpf-next 05/17] bpf: Pass a set of bpf_core_relo-s to prog_load command.
Date: Wed, 20 Apr 2022 09:50:48 -0700	[thread overview]
Message-ID: <CAEf4BzbT4vQBnZzdD00SuPCDkeb4Cm=F6PLUoO_3X93UQD5hbQ@mail.gmail.com> (raw)
In-Reply-To: <87sfq8f0ex.fsf@cloudflare.com>

On Wed, Apr 20, 2022 at 4:40 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Hi Alexei,
>
> On Wed, Dec 01, 2021 at 10:10 AM -08, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > struct bpf_core_relo is generated by llvm and processed by libbpf.
> > It's a de-facto uapi.
> > With CO-RE in the kernel the struct bpf_core_relo becomes uapi de-jure.
> > Add an ability to pass a set of 'struct bpf_core_relo' to prog_load command
> > and let the kernel perform CO-RE relocations.
> >
> > Note the struct bpf_line_info and struct bpf_func_info have the same
> > layout when passed from LLVM to libbpf and from libbpf to the kernel
> > except "insn_off" fields means "byte offset" when LLVM generates it.
> > Then libbpf converts it to "insn index" to pass to the kernel.
> > The struct bpf_core_relo's "insn_off" field is always "byte offset".
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  include/linux/bpf.h            |  8 ++++
> >  include/uapi/linux/bpf.h       | 59 +++++++++++++++++++++++++-
> >  kernel/bpf/btf.c               |  6 +++
> >  kernel/bpf/syscall.c           |  2 +-
> >  kernel/bpf/verifier.c          | 76 ++++++++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h | 59 +++++++++++++++++++++++++-
> >  tools/lib/bpf/relo_core.h      | 53 ------------------------
> >  7 files changed, 207 insertions(+), 56 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index cad0829710be..8bbf08fbab66 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1732,6 +1732,14 @@ bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog);
> >  const struct btf_func_model *
> >  bpf_jit_find_kfunc_model(const struct bpf_prog *prog,
> >                        const struct bpf_insn *insn);
> > +struct bpf_core_ctx {
> > +     struct bpf_verifier_log *log;
> > +     const struct btf *btf;
> > +};
> > +
> > +int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> > +                int relo_idx, void *insn);
> > +
> >  #else /* !CONFIG_BPF_SYSCALL */
> >  static inline struct bpf_prog *bpf_prog_get(u32 ufd)
> >  {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 9e66b1880020..c26871263f1f 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1342,8 +1342,10 @@ union bpf_attr {
> >                       /* or valid module BTF object fd or 0 to attach to vmlinux */
> >                       __u32           attach_btf_obj_fd;
> >               };
> > -             __u32           :32;            /* pad */
> > +             __u32           core_relo_cnt;  /* number of bpf_core_relo */
> >               __aligned_u64   fd_array;       /* array of FDs */
> > +             __aligned_u64   core_relos;
> > +             __u32           core_relo_rec_size; /* sizeof(struct bpf_core_relo) */
> >       };
> >
> >       struct { /* anonymous struct used by BPF_OBJ_* commands */
>
> I think I've spotted a breakage.
>
> Plugging the 4 byte hole with core_relo_cnt means that programs built
> against < v5.17 headers pass garbage as core_relo_cnt value.
>
> That in turn makes check_core_relo() fail with -EINVAL, which fails
> PROG_LOAD.

bpf_attr is supposed to be zeroed out with memset(), so that hole
should have zero even before core_relo_cnt was added

>
> [...]