[PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[Andrii Nakryiko]

Previously, if libbpf failed to resolve CO-RE relocation for some
instructions, it would either return error immediately, or, if
.relaxed_core_relocs option was set, would replace relocatable offset/imm part
of an instruction with a bogus value (-1). Neither approach is good, because
there are many possible scenarios where relocation is expected to fail (e.g.,
when some field knowingly can be missing on specific kernel versions). On the
other hand, replacing offset with invalid one can hide programmer errors, if
this relocation failue wasn't anticipated.

This patch deprecates .relaxed_core_relocs option and changes the approach to
always replacing instruction, for which relocation failed, with invalid BPF
helper call instruction. For cases where this is expected, BPF program should
already ensure that that instruction is unreachable, in which case this
invalid instruction is going to be silently ignored. But if instruction wasn't
guarded, BPF program will be rejected at verification step with verifier log
pointing precisely to the place in assembly where the problem is.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
 tools/lib/bpf/libbpf.h |  6 ++-
 2 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ae34b681ae82..39f1b7633a7c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -345,7 +345,6 @@ struct bpf_object {
 
 	bool loaded;
 	bool has_pseudo_calls;
-	bool relaxed_core_relocs;
 
 	/*
 	 * Information when doing elf related work. Only valid if fd
@@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
  */
 static int bpf_core_reloc_insn(struct bpf_program *prog,
 			       const struct bpf_field_reloc *relo,
+			       int relo_idx,
 			       const struct bpf_core_spec *local_spec,
 			       const struct bpf_core_spec *targ_spec)
 {
-	bool failed = false, validate = true;
 	__u32 orig_val, new_val;
 	struct bpf_insn *insn;
+	bool validate = true;
 	int insn_idx, err;
 	__u8 class;
 
 	if (relo->insn_off % sizeof(struct bpf_insn))
 		return -EINVAL;
 	insn_idx = relo->insn_off / sizeof(struct bpf_insn);
+	insn = &prog->insns[insn_idx];
+	class = BPF_CLASS(insn->code);
 
 	if (relo->kind == BPF_FIELD_EXISTS) {
 		orig_val = 1; /* can't generate EXISTS relo w/o local field */
 		new_val = targ_spec ? 1 : 0;
 	} else if (!targ_spec) {
-		failed = true;
-		new_val = (__u32)-1;
+		pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
+			 bpf_program__title(prog, false), relo_idx, insn_idx);
+		insn->code = BPF_JMP | BPF_CALL;
+		insn->dst_reg = 0;
+		insn->src_reg = 0;
+		insn->off = 0;
+		/* if this instruction is reachable (not a dead code),
+		 * verifier will complain with the following message:
+		 * invalid func unknown#195896080
+		 */
+		insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
+		return 0;
 	} else {
 		err = bpf_core_calc_field_relo(prog, relo, local_spec,
 					       &orig_val, &validate);
@@ -4268,50 +4280,47 @@ static int bpf_core_reloc_insn(struct bpf_program *prog,
 			return err;
 	}
 
-	insn = &prog->insns[insn_idx];
-	class = BPF_CLASS(insn->code);
-
 	switch (class) {
 	case BPF_ALU:
 	case BPF_ALU64:
 		if (BPF_SRC(insn->code) != BPF_K)
 			return -EINVAL;
-		if (!failed && validate && insn->imm != orig_val) {
-			pr_warn("prog '%s': unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n",
-				bpf_program__title(prog, false), insn_idx,
-				insn->imm, orig_val, new_val);
+		if (validate && insn->imm != orig_val) {
+			pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n",
+				bpf_program__title(prog, false), relo_idx,
+				insn_idx, insn->imm, orig_val, new_val);
 			return -EINVAL;
 		}
 		orig_val = insn->imm;
 		insn->imm = new_val;
-		pr_debug("prog '%s': patched insn #%d (ALU/ALU64)%s imm %u -> %u\n",
-			 bpf_program__title(prog, false), insn_idx,
-			 failed ? " w/ failed reloc" : "", orig_val, new_val);
+		pr_debug("prog '%s': relo #%d: patched insn #%d (ALU/ALU64) imm %u -> %u\n",
+			 bpf_program__title(prog, false), relo_idx, insn_idx,
+			 orig_val, new_val);
 		break;
 	case BPF_LDX:
 	case BPF_ST:
 	case BPF_STX:
-		if (!failed && validate && insn->off != orig_val) {
-			pr_warn("prog '%s': unexpected insn #%d (LD/LDX/ST/STX) value: got %u, exp %u -> %u\n",
-				bpf_program__title(prog, false), insn_idx,
-				insn->off, orig_val, new_val);
+		if (validate && insn->off != orig_val) {
+			pr_warn("prog '%s': relo #%d: unexpected insn #%d (LD/LDX/ST/STX) value: got %u, exp %u -> %u\n",
+				bpf_program__title(prog, false), relo_idx,
+				insn_idx, insn->off, orig_val, new_val);
 			return -EINVAL;
 		}
 		if (new_val > SHRT_MAX) {
-			pr_warn("prog '%s': insn #%d (LD/LDX/ST/STX) value too big: %u\n",
-				bpf_program__title(prog, false), insn_idx,
-				new_val);
+			pr_warn("prog '%s': relo #%d: insn #%d (LDX/ST/STX) value too big: %u\n",
+				bpf_program__title(prog, false), relo_idx,
+				insn_idx, new_val);
 			return -ERANGE;
 		}
 		orig_val = insn->off;
 		insn->off = new_val;
-		pr_debug("prog '%s': patched insn #%d (LD/LDX/ST/STX)%s off %u -> %u\n",
-			 bpf_program__title(prog, false), insn_idx,
-			 failed ? " w/ failed reloc" : "", orig_val, new_val);
+		pr_debug("prog '%s': relo #%d: patched insn #%d (LDX/ST/STX) off %u -> %u\n",
+			 bpf_program__title(prog, false), relo_idx, insn_idx,
+			 orig_val, new_val);
 		break;
 	default:
-		pr_warn("prog '%s': trying to relocate unrecognized insn #%d, code:%x, src:%x, dst:%x, off:%x, imm:%x\n",
-			bpf_program__title(prog, false),
+		pr_warn("prog '%s': relo #%d: trying to relocate unrecognized insn #%d, code:%x, src:%x, dst:%x, off:%x, imm:%x\n",
+			bpf_program__title(prog, false), relo_idx,
 			insn_idx, insn->code, insn->src_reg, insn->dst_reg,
 			insn->off, insn->imm);
 		return -EINVAL;
@@ -4510,24 +4519,33 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
 	}
 
 	/*
-	 * For BPF_FIELD_EXISTS relo or when relaxed CO-RE reloc mode is
-	 * requested, it's expected that we might not find any candidates.
-	 * In this case, if field wasn't found in any candidate, the list of
-	 * candidates shouldn't change at all, we'll just handle relocating
-	 * appropriately, depending on relo's kind.
+	 * For BPF_FIELD_EXISTS relo or when used BPF program has field
+	 * existence checks or kernel version/config checks, it's expected
+	 * that we might not find any candidates. In this case, if field
+	 * wasn't found in any candidate, the list of candidates shouldn't
+	 * change at all, we'll just handle relocating appropriately,
+	 * depending on relo's kind.
 	 */
 	if (j > 0)
 		cand_ids->len = j;
 
-	if (j == 0 && !prog->obj->relaxed_core_relocs &&
-	    relo->kind != BPF_FIELD_EXISTS) {
-		pr_warn("prog '%s': relo #%d: no matching targets found for [%d] %s + %s\n",
-			prog_name, relo_idx, local_id, local_name, spec_str);
-		return -ESRCH;
-	}
+	/*
+	 * If no candidates were found, it might be both a programmer error,
+	 * as well as expected case, depending whether instruction w/
+	 * relocation is guarded in some way that makes it unreachable (dead
+	 * code) if relocation can't be resolved. This is handled in
+	 * bpf_core_reloc_insn() uniformly by replacing that instruction with
+	 * BPF helper call insn (using invalid helper ID). If that instruction
+	 * is indeed unreachable, then it will be ignored and eliminated by
+	 * verifier. If it was an error, then verifier will complain and point
+	 * to a specific instruction number in its log.
+	 */
+	if (j == 0)
+		pr_debug("prog '%s': relo #%d: no matching targets found for [%d] %s + %s\n",
+			 prog_name, relo_idx, local_id, local_name, spec_str);
 
 	/* bpf_core_reloc_insn should know how to handle missing targ_spec */
-	err = bpf_core_reloc_insn(prog, relo, &local_spec,
+	err = bpf_core_reloc_insn(prog, relo, relo_idx, &local_spec,
 				  j ? &targ_spec : NULL);
 	if (err) {
 		pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
@@ -5057,7 +5075,6 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 	if (IS_ERR(obj))
 		return obj;
 
-	obj->relaxed_core_relocs = OPTS_GET(opts, relaxed_core_relocs, false);
 	kconfig = OPTS_GET(opts, kconfig, NULL);
 	if (kconfig) {
 		obj->kconfig = strdup(kconfig);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 2a5e3b087002..3fe12c9d1f92 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -77,7 +77,11 @@ struct bpf_object_open_opts {
 	const char *object_name;
 	/* parse map definitions non-strictly, allowing extra attributes/data */
 	bool relaxed_maps;
-	/* process CO-RE relocations non-strictly, allowing them to fail */
+	/* DEPRECATED: handle CO-RE relocations non-strictly, allowing failures.
+	 * Value is ignored. Relocations always are processed non-strictly.
+	 * Non-relocatable instructions are replaced with invalid ones to
+	 * prevent accidental errors.
+	 * */
 	bool relaxed_core_relocs;
 	/* maps that set the 'pinning' attribute in their definition will have
 	 * their pin_path attribute set to a file in this directory, and be
-- 
2.17.1

Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[Martin Lau]

On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
> Previously, if libbpf failed to resolve CO-RE relocation for some
> instructions, it would either return error immediately, or, if
> .relaxed_core_relocs option was set, would replace relocatable offset/imm part
> of an instruction with a bogus value (-1). Neither approach is good, because
> there are many possible scenarios where relocation is expected to fail (e.g.,
> when some field knowingly can be missing on specific kernel versions). On the
> other hand, replacing offset with invalid one can hide programmer errors, if
> this relocation failue wasn't anticipated.
> 
> This patch deprecates .relaxed_core_relocs option and changes the approach to
> always replacing instruction, for which relocation failed, with invalid BPF
> helper call instruction. For cases where this is expected, BPF program should
> already ensure that that instruction is unreachable, in which case this
> invalid instruction is going to be silently ignored. But if instruction wasn't
> guarded, BPF program will be rejected at verification step with verifier log
> pointing precisely to the place in assembly where the problem is.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
>  tools/lib/bpf/libbpf.h |  6 ++-
>  2 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ae34b681ae82..39f1b7633a7c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -345,7 +345,6 @@ struct bpf_object {
>  
>  	bool loaded;
>  	bool has_pseudo_calls;
> -	bool relaxed_core_relocs;
>  
>  	/*
>  	 * Information when doing elf related work. Only valid if fd
> @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
>   */
>  static int bpf_core_reloc_insn(struct bpf_program *prog,
>  			       const struct bpf_field_reloc *relo,
> +			       int relo_idx,
>  			       const struct bpf_core_spec *local_spec,
>  			       const struct bpf_core_spec *targ_spec)
>  {
> -	bool failed = false, validate = true;
>  	__u32 orig_val, new_val;
>  	struct bpf_insn *insn;
> +	bool validate = true;
>  	int insn_idx, err;
>  	__u8 class;
>  
>  	if (relo->insn_off % sizeof(struct bpf_insn))
>  		return -EINVAL;
>  	insn_idx = relo->insn_off / sizeof(struct bpf_insn);
> +	insn = &prog->insns[insn_idx];
> +	class = BPF_CLASS(insn->code);
>  
>  	if (relo->kind == BPF_FIELD_EXISTS) {
>  		orig_val = 1; /* can't generate EXISTS relo w/o local field */
>  		new_val = targ_spec ? 1 : 0;
>  	} else if (!targ_spec) {
> -		failed = true;
> -		new_val = (__u32)-1;
> +		pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
> +			 bpf_program__title(prog, false), relo_idx, insn_idx);
> +		insn->code = BPF_JMP | BPF_CALL;
> +		insn->dst_reg = 0;
> +		insn->src_reg = 0;
> +		insn->off = 0;
> +		/* if this instruction is reachable (not a dead code),
> +		 * verifier will complain with the following message:
> +		 * invalid func unknown#195896080
> +		 */
> +		insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
Should this value become a binded contract in uapi/bpf.h so
that the verifier can print a more meaningful name than "unknown#195896080"?

> +		return 0;
>  	} else {
>  		err = bpf_core_calc_field_relo(prog, relo, local_spec,
>  					       &orig_val, &validate);

Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[Andrii Nakryiko]

On Thu, Jan 23, 2020 at 11:21 PM Martin Lau <kafai@fb.com> wrote:
>
> On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
> > Previously, if libbpf failed to resolve CO-RE relocation for some
> > instructions, it would either return error immediately, or, if
> > .relaxed_core_relocs option was set, would replace relocatable offset/imm part
> > of an instruction with a bogus value (-1). Neither approach is good, because
> > there are many possible scenarios where relocation is expected to fail (e.g.,
> > when some field knowingly can be missing on specific kernel versions). On the
> > other hand, replacing offset with invalid one can hide programmer errors, if
> > this relocation failue wasn't anticipated.
> >
> > This patch deprecates .relaxed_core_relocs option and changes the approach to
> > always replacing instruction, for which relocation failed, with invalid BPF
> > helper call instruction. For cases where this is expected, BPF program should
> > already ensure that that instruction is unreachable, in which case this
> > invalid instruction is going to be silently ignored. But if instruction wasn't
> > guarded, BPF program will be rejected at verification step with verifier log
> > pointing precisely to the place in assembly where the problem is.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
> >  tools/lib/bpf/libbpf.h |  6 ++-
> >  2 files changed, 61 insertions(+), 40 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index ae34b681ae82..39f1b7633a7c 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -345,7 +345,6 @@ struct bpf_object {
> >
> >       bool loaded;
> >       bool has_pseudo_calls;
> > -     bool relaxed_core_relocs;
> >
> >       /*
> >        * Information when doing elf related work. Only valid if fd
> > @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
> >   */
> >  static int bpf_core_reloc_insn(struct bpf_program *prog,
> >                              const struct bpf_field_reloc *relo,
> > +                            int relo_idx,
> >                              const struct bpf_core_spec *local_spec,
> >                              const struct bpf_core_spec *targ_spec)
> >  {
> > -     bool failed = false, validate = true;
> >       __u32 orig_val, new_val;
> >       struct bpf_insn *insn;
> > +     bool validate = true;
> >       int insn_idx, err;
> >       __u8 class;
> >
> >       if (relo->insn_off % sizeof(struct bpf_insn))
> >               return -EINVAL;
> >       insn_idx = relo->insn_off / sizeof(struct bpf_insn);
> > +     insn = &prog->insns[insn_idx];
> > +     class = BPF_CLASS(insn->code);
> >
> >       if (relo->kind == BPF_FIELD_EXISTS) {
> >               orig_val = 1; /* can't generate EXISTS relo w/o local field */
> >               new_val = targ_spec ? 1 : 0;
> >       } else if (!targ_spec) {
> > -             failed = true;
> > -             new_val = (__u32)-1;
> > +             pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
> > +                      bpf_program__title(prog, false), relo_idx, insn_idx);
> > +             insn->code = BPF_JMP | BPF_CALL;
> > +             insn->dst_reg = 0;
> > +             insn->src_reg = 0;
> > +             insn->off = 0;
> > +             /* if this instruction is reachable (not a dead code),
> > +              * verifier will complain with the following message:
> > +              * invalid func unknown#195896080
> > +              */
> > +             insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
> Should this value become a binded contract in uapi/bpf.h so
> that the verifier can print a more meaningful name than "unknown#195896080"?
>

It feels a bit premature to fix this in kernel. It's one of many ways
we can do this, e.g., alternative would be using invalid opcode
altogether. It's not yet clear what's the best way to report this from
kernel. Maybe in the future verifier will have some better way to
pinpoint where and what problem there is in user's program through
some more structured approach than current free-form log.

So what I'm trying to say is that we should probably get a bit more
experience using these features first and understand what
kernel/userspace interface should be for reporting issues like this,
before setting anything in stone in verifier. For now, this
"unknown#195896080" should be a pretty unique search term :)

> > +             return 0;
> >       } else {
> >               err = bpf_core_calc_field_relo(prog, relo, local_spec,
> >                                              &orig_val, &validate);

Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[Martin Lau]

On Fri, Jan 24, 2020 at 10:30:12AM -0800, Andrii Nakryiko wrote:
> On Thu, Jan 23, 2020 at 11:21 PM Martin Lau <kafai@fb.com> wrote:
> >
> > On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
> > > Previously, if libbpf failed to resolve CO-RE relocation for some
> > > instructions, it would either return error immediately, or, if
> > > .relaxed_core_relocs option was set, would replace relocatable offset/imm part
> > > of an instruction with a bogus value (-1). Neither approach is good, because
> > > there are many possible scenarios where relocation is expected to fail (e.g.,
> > > when some field knowingly can be missing on specific kernel versions). On the
> > > other hand, replacing offset with invalid one can hide programmer errors, if
> > > this relocation failue wasn't anticipated.
> > >
> > > This patch deprecates .relaxed_core_relocs option and changes the approach to
> > > always replacing instruction, for which relocation failed, with invalid BPF
> > > helper call instruction. For cases where this is expected, BPF program should
> > > already ensure that that instruction is unreachable, in which case this
> > > invalid instruction is going to be silently ignored. But if instruction wasn't
> > > guarded, BPF program will be rejected at verification step with verifier log
> > > pointing precisely to the place in assembly where the problem is.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
> > >  tools/lib/bpf/libbpf.h |  6 ++-
> > >  2 files changed, 61 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index ae34b681ae82..39f1b7633a7c 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -345,7 +345,6 @@ struct bpf_object {
> > >
> > >       bool loaded;
> > >       bool has_pseudo_calls;
> > > -     bool relaxed_core_relocs;
> > >
> > >       /*
> > >        * Information when doing elf related work. Only valid if fd
> > > @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
> > >   */
> > >  static int bpf_core_reloc_insn(struct bpf_program *prog,
> > >                              const struct bpf_field_reloc *relo,
> > > +                            int relo_idx,
> > >                              const struct bpf_core_spec *local_spec,
> > >                              const struct bpf_core_spec *targ_spec)
> > >  {
> > > -     bool failed = false, validate = true;
> > >       __u32 orig_val, new_val;
> > >       struct bpf_insn *insn;
> > > +     bool validate = true;
> > >       int insn_idx, err;
> > >       __u8 class;
> > >
> > >       if (relo->insn_off % sizeof(struct bpf_insn))
> > >               return -EINVAL;
> > >       insn_idx = relo->insn_off / sizeof(struct bpf_insn);
> > > +     insn = &prog->insns[insn_idx];
> > > +     class = BPF_CLASS(insn->code);
> > >
> > >       if (relo->kind == BPF_FIELD_EXISTS) {
> > >               orig_val = 1; /* can't generate EXISTS relo w/o local field */
> > >               new_val = targ_spec ? 1 : 0;
> > >       } else if (!targ_spec) {
> > > -             failed = true;
> > > -             new_val = (__u32)-1;
> > > +             pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
> > > +                      bpf_program__title(prog, false), relo_idx, insn_idx);
> > > +             insn->code = BPF_JMP | BPF_CALL;
> > > +             insn->dst_reg = 0;
> > > +             insn->src_reg = 0;
> > > +             insn->off = 0;
> > > +             /* if this instruction is reachable (not a dead code),
> > > +              * verifier will complain with the following message:
> > > +              * invalid func unknown#195896080
> > > +              */
> > > +             insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
> > Should this value become a binded contract in uapi/bpf.h so
> > that the verifier can print a more meaningful name than "unknown#195896080"?
> >
> 
> It feels a bit premature to fix this in kernel. It's one of many ways
> we can do this, e.g., alternative would be using invalid opcode
> altogether. It's not yet clear what's the best way to report this from
> kernel. Maybe in the future verifier will have some better way to
> pinpoint where and what problem there is in user's program through
> some more structured approach than current free-form log.
> 
> So what I'm trying to say is that we should probably get a bit more
> experience using these features first and understand what
> kernel/userspace interface should be for reporting issues like this,
> before setting anything in stone in verifier. For now, this
> "unknown#195896080" should be a pretty unique search term :)
Sure.  I think this value will never be used for real in the life time.
I was mostly worry this message will be confusing.  May be the loader
could be improved to catch this and interpret it in a more meaningful
way.

The change lgtm,

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of failed CO-RE relocations

[Daniel Borkmann]

On 1/24/20 9:12 PM, Martin Lau wrote:
> On Fri, Jan 24, 2020 at 10:30:12AM -0800, Andrii Nakryiko wrote:
>> On Thu, Jan 23, 2020 at 11:21 PM Martin Lau <kafai@fb.com> wrote:
>>> On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
>>>> Previously, if libbpf failed to resolve CO-RE relocation for some
>>>> instructions, it would either return error immediately, or, if
>>>> .relaxed_core_relocs option was set, would replace relocatable offset/imm part
>>>> of an instruction with a bogus value (-1). Neither approach is good, because
>>>> there are many possible scenarios where relocation is expected to fail (e.g.,
>>>> when some field knowingly can be missing on specific kernel versions). On the
>>>> other hand, replacing offset with invalid one can hide programmer errors, if
>>>> this relocation failue wasn't anticipated.
>>>>
>>>> This patch deprecates .relaxed_core_relocs option and changes the approach to
>>>> always replacing instruction, for which relocation failed, with invalid BPF
>>>> helper call instruction. For cases where this is expected, BPF program should
>>>> already ensure that that instruction is unreachable, in which case this
>>>> invalid instruction is going to be silently ignored. But if instruction wasn't
>>>> guarded, BPF program will be rejected at verification step with verifier log
>>>> pointing precisely to the place in assembly where the problem is.
>>>>
>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>> ---
>>>>   tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
>>>>   tools/lib/bpf/libbpf.h |  6 ++-
>>>>   2 files changed, 61 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index ae34b681ae82..39f1b7633a7c 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -345,7 +345,6 @@ struct bpf_object {
>>>>
>>>>        bool loaded;
>>>>        bool has_pseudo_calls;
>>>> -     bool relaxed_core_relocs;
>>>>
>>>>        /*
>>>>         * Information when doing elf related work. Only valid if fd
>>>> @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
>>>>    */
>>>>   static int bpf_core_reloc_insn(struct bpf_program *prog,
>>>>                               const struct bpf_field_reloc *relo,
>>>> +                            int relo_idx,
>>>>                               const struct bpf_core_spec *local_spec,
>>>>                               const struct bpf_core_spec *targ_spec)
>>>>   {
>>>> -     bool failed = false, validate = true;
>>>>        __u32 orig_val, new_val;
>>>>        struct bpf_insn *insn;
>>>> +     bool validate = true;
>>>>        int insn_idx, err;
>>>>        __u8 class;
>>>>
>>>>        if (relo->insn_off % sizeof(struct bpf_insn))
>>>>                return -EINVAL;
>>>>        insn_idx = relo->insn_off / sizeof(struct bpf_insn);
>>>> +     insn = &prog->insns[insn_idx];
>>>> +     class = BPF_CLASS(insn->code);
>>>>
>>>>        if (relo->kind == BPF_FIELD_EXISTS) {
>>>>                orig_val = 1; /* can't generate EXISTS relo w/o local field */
>>>>                new_val = targ_spec ? 1 : 0;
>>>>        } else if (!targ_spec) {
>>>> -             failed = true;
>>>> -             new_val = (__u32)-1;
>>>> +             pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
>>>> +                      bpf_program__title(prog, false), relo_idx, insn_idx);
>>>> +             insn->code = BPF_JMP | BPF_CALL;
>>>> +             insn->dst_reg = 0;
>>>> +             insn->src_reg = 0;
>>>> +             insn->off = 0;
>>>> +             /* if this instruction is reachable (not a dead code),
>>>> +              * verifier will complain with the following message:
>>>> +              * invalid func unknown#195896080
>>>> +              */
>>>> +             insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
>>> Should this value become a binded contract in uapi/bpf.h so
>>> that the verifier can print a more meaningful name than "unknown#195896080"?
>>
>> It feels a bit premature to fix this in kernel. It's one of many ways
>> we can do this, e.g., alternative would be using invalid opcode
>> altogether. It's not yet clear what's the best way to report this from
>> kernel. Maybe in the future verifier will have some better way to
>> pinpoint where and what problem there is in user's program through
>> some more structured approach than current free-form log.
>>
>> So what I'm trying to say is that we should probably get a bit more
>> experience using these features first and understand what
>> kernel/userspace interface should be for reporting issues like this,
>> before setting anything in stone in verifier. For now, this
>> "unknown#195896080" should be a pretty unique search term :)
> Sure.  I think this value will never be used for real in the life time.
> I was mostly worry this message will be confusing.  May be the loader
> could be improved to catch this and interpret it in a more meaningful
> way.

Agree with both of you that we might want to find a better error reporting
mechanism here in future, but can be done on top of this. Applied, thanks!