bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Yonghong Song <yhs@fb.com>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 02/10] libbpf: implement BPF CO-RE offset relocation algorithm
Date: Fri, 26 Jul 2019 23:25:36 -0700	[thread overview]
Message-ID: <CAEf4BzZxPgAh4PGSWyD0tPOd1wh=DGZuSe1fzxc-Sgyk4D5vDg@mail.gmail.com> (raw)
In-Reply-To: <20190725231831.7v7mswluomcymy2l@ast-mbp>

On Thu, Jul 25, 2019 at 4:18 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jul 24, 2019 at 12:27:34PM -0700, Andrii Nakryiko wrote:
> > This patch implements the core logic for BPF CO-RE offsets relocations.
> > All the details are described in code comments.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 866 ++++++++++++++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.h |   1 +
> >  2 files changed, 861 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8741c39adb1c..86d87bf10d46 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -38,6 +38,7 @@
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> >  #include <sys/vfs.h>
> > +#include <sys/utsname.h>
> >  #include <tools/libc_compat.h>
> >  #include <libelf.h>
> >  #include <gelf.h>
> > @@ -47,6 +48,7 @@
> >  #include "btf.h"
> >  #include "str_error.h"
> >  #include "libbpf_internal.h"
> > +#include "hashmap.h"
> >
> >  #ifndef EM_BPF
> >  #define EM_BPF 247
> > @@ -1013,16 +1015,22 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
> >  }
> >
> >  static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> > -                                                  __u32 id)
> > +                                                  __u32 id,
> > +                                                  __u32 *res_id)
>
> I think it would be more readable to format it like:
> static const struct btf_type *
> skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id)

Ok.

>
> > +     } else if (class == BPF_ST && BPF_MODE(insn->code) == BPF_MEM) {
> > +             if (insn->imm != orig_off)
> > +                     return -EINVAL;
> > +             insn->imm = new_off;
> > +             pr_debug("prog '%s': patched insn #%d (ST | MEM) imm %d -> %d\n",
> > +                      bpf_program__title(prog, false),
> > +                      insn_idx, orig_off, new_off);
>
> I'm pretty sure llvm was not capable of emitting BPF_ST insn.
> When did that change?

I just looked at possible instructions that could have 32-bit
immediate value. This is `*(rX) = offsetof(struct s, field)`, which I
though is conceivable. Do you think I should drop it?

>
> > +/*
> > + * CO-RE relocate single instruction.
> > + *
> > + * The outline and important points of the algorithm:
> > + * 1. For given local type, find corresponding candidate target types.
> > + *    Candidate type is a type with the same "essential" name, ignoring
> > + *    everything after last triple underscore (___). E.g., `sample`,
> > + *    `sample___flavor_one`, `sample___flavor_another_one`, are all candidates
> > + *    for each other. Names with triple underscore are referred to as
> > + *    "flavors" and are useful, among other things, to allow to
> > + *    specify/support incompatible variations of the same kernel struct, which
> > + *    might differ between different kernel versions and/or build
> > + *    configurations.
>
> "flavors" is a convention of bpftool btf2c converter, right?
> May be mention it here with pointer to the code?

Yes, btf2c converter generates "flavors" on type name conflict (adding
___2, ___3), but it's not the only use case. It's a general way to
have independent incompatible definitions for the same target type.
E.g., locally in your BPF program you can define two thread_structs to
accommodate field rename between kernel version changes:

struct thread_struct___before_47 {
    long fs;
};

struct thread_struct___after_47 {
    long fsbase;
};

Then with conditional relocations you'll use one of them to "extract"
it from real kernel's thread_struct:

void *fsbase;

if (LINUX_VERSION < 407)
    BPF_CORE_READ(&fsbase, sizeof(fsbase),
                  &((struct thread_struct___before_47 *)&thread)->fs);
else
    BPF_CORE_READ(&fsbase, sizeof(fsbase),
                  &((struct thread_struct___after_47 *)&thread)->fsbase);

So it works both ways (for local and target types) by design. I can
mention that btf2c converter uses this convention for types with
conflicting names, but btf2c is not a definition of what flavor is.

>
> > +     pr_debug("prog '%s': relo #%d: insn_off=%d, [%d] (%s) + %s\n",
> > +              prog_name, relo_idx, relo->insn_off,
> > +              local_id, local_name, spec_str);
> > +
> > +     err = bpf_core_spec_parse(local_btf, local_id, spec_str, &local_spec);
> > +     if (err) {
> > +             pr_warning("prog '%s': relo #%d: parsing [%d] (%s) + %s failed: %d\n",
> > +                        prog_name, relo_idx, local_id, local_name, spec_str,
> > +                        err);
> > +             return -EINVAL;
> > +     }
> > +     pr_debug("prog '%s': relo #%d: [%d] (%s) + %s is off %u, len %d, raw_len %d\n",
> > +              prog_name, relo_idx, local_id, local_name, spec_str,
> > +              local_spec.offset, local_spec.len, local_spec.raw_len);
>
> one warn and two debug that print more or less the same info seems like overkill.

Only one of them will ever be emitted, though. And this information is
and will be invaluable to debug issues/explain behavior in the future
once adoption starts. So I'm inclined to keep them, at least for now.
But I think I'll extract spec formatting into a separate reusable
function, which will make this significantly less verbose.

>
> > +     for (i = 0, j = 0; i < cand_ids->len; i++) {
> > +             cand_id = cand_ids->data[j];
> > +             cand_type = btf__type_by_id(targ_btf, cand_id);
> > +             cand_name = btf__name_by_offset(targ_btf, cand_type->name_off);
> > +
> > +             err = bpf_core_spec_match(&local_spec, targ_btf,
> > +                                       cand_id, &cand_spec);
> > +             if (err < 0) {
> > +                     pr_warning("prog '%s': relo #%d: failed to match spec [%d] (%s) + %s to candidate #%d [%d] (%s): %d\n",
> > +                                prog_name, relo_idx, local_id, local_name,
> > +                                spec_str, i, cand_id, cand_name, err);
> > +                     return err;
> > +             }
> > +             if (err == 0) {
> > +                     pr_debug("prog '%s': relo #%d: candidate #%d [%d] (%s) doesn't match spec\n",
> > +                              prog_name, relo_idx, i, cand_id, cand_name);
> > +                     continue;
> > +             }
> > +
> > +             pr_debug("prog '%s': relo #%d: candidate #%d ([%d] %s) is off %u, len %d, raw_len %d\n",
> > +                      prog_name, relo_idx, i, cand_id, cand_name,
> > +                      cand_spec.offset, cand_spec.len, cand_spec.raw_len);
>
> have the same feeling about 3 printfs above.
>

Same as above.

  reply	other threads:[~2019-07-27  6:25 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-24 19:27 [PATCH bpf-next 00/10] CO-RE offset relocations Andrii Nakryiko
2019-07-24 19:27 ` [PATCH bpf-next 01/10] libbpf: add .BTF.ext offset relocation section loading Andrii Nakryiko
2019-07-24 21:42   ` Andrii Nakryiko
2019-07-25  0:00   ` Song Liu
2019-07-25  0:37     ` Andrii Nakryiko
2019-07-25  5:20       ` Song Liu
2019-07-27  5:11         ` Andrii Nakryiko
2019-07-29 20:00           ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 02/10] libbpf: implement BPF CO-RE offset relocation algorithm Andrii Nakryiko
2019-07-25 19:32   ` Song Liu
2019-07-27  6:11     ` Andrii Nakryiko
2019-07-27 18:59       ` Song Liu
2019-07-27 19:09         ` Andrii Nakryiko
2019-07-28  0:24           ` Song Liu
2019-07-25 23:18   ` Alexei Starovoitov
2019-07-27  6:25     ` Andrii Nakryiko [this message]
2019-07-27 17:00       ` Alexei Starovoitov
2019-07-27 18:24         ` Andrii Nakryiko
2019-07-27 21:29           ` Yonghong Song
2019-07-27 21:36             ` Andrii Nakryiko
2019-07-29 19:56   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 03/10] selftests/bpf: add CO-RE relocs testing setup Andrii Nakryiko
2019-07-29 20:22   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 04/10] selftests/bpf: add CO-RE relocs struct flavors tests Andrii Nakryiko
2019-07-29 20:37   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 05/10] selftests/bpf: add CO-RE relocs nesting tests Andrii Nakryiko
2019-07-29 21:06   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 06/10] selftests/bpf: add CO-RE relocs array tests Andrii Nakryiko
2019-07-25 23:26   ` Alexei Starovoitov
2019-07-26 23:29     ` Andrii Nakryiko
2019-07-24 19:27 ` [PATCH bpf-next 07/10] selftests/bpf: add CO-RE relocs enum/ptr/func_proto tests Andrii Nakryiko
2019-07-29 21:09   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 08/10] selftests/bpf: add CO-RE relocs modifiers/typedef tests Andrii Nakryiko
2019-07-29 21:11   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 09/10] selftest/bpf: add CO-RE relocs ptr-as-array tests Andrii Nakryiko
2019-07-29 21:14   ` Song Liu
2019-07-24 19:27 ` [PATCH bpf-next 10/10] selftests/bpf: add CO-RE relocs ints tests Andrii Nakryiko
2019-07-29 21:21   ` Song Liu
2019-07-29 20:20 ` [PATCH bpf-next 00/10] CO-RE offset relocations Song Liu
2019-07-29 20:36   ` Song Liu
2019-07-29 23:09     ` Andrii Nakryiko
2019-07-30  5:27       ` Song Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzZxPgAh4PGSWyD0tPOd1wh=DGZuSe1fzxc-Sgyk4D5vDg@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).