Re: [PATCH bpf-next 3/9] libbpf: improve relocation ambiguity detection

Re: [PATCH bpf-next 3/9] libbpf: improve relocation ambiguity detection

[Menglong Dong]

On 2020/8/19 06:39 Andrii Nakryiko <andriin@fb.com> write:
> Split the instruction patching logic into relocation value calculation and
> application of relocation to instruction. Using this, evaluate relocation
> against each matching candidate and validate that all candidates agree on
> relocated value. If not, report ambiguity and fail load.
> 
> This logic is necessary to avoid dangerous (however unlikely) accidental match
> against two incompatible candidate types. Without this change, libbpf will
> pick a random type as *the* candidate and apply potentially invalid
> relocation.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 170 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 124 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2047e4ed0076..1ba458140f50 100644
> --- a/tools/lib/bpf/libbpf.c
[......]
> @@ -5005,16 +5063,31 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
>  		if (err == 0)
>  			continue;
>  
> +		err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, &cand_spec, &cand_res);
> +		if (err)
> +			return err;
> +
>  		if (j == 0) {
> +			targ_res = cand_res;
>  			targ_spec = cand_spec;
>  		} else if (cand_spec.bit_offset != targ_spec.bit_offset) {
> -			/* if there are many candidates, they should all
> -			 * resolve to the same bit offset
> +			/* if there are many field relo candidates, they
> +			 * should all resolve to the same bit offset
>  			 */
> -			pr_warn("prog '%s': relo #%d: offset ambiguity: %u != %u\n",
> +			pr_warn("prog '%s': relo #%d: field offset ambiguity: %u != %u\n",
>  				prog_name, relo_idx, cand_spec.bit_offset,
>  				targ_spec.bit_offset);
>  			return -EINVAL;
> +		} else if (cand_res.poison != targ_res.poison || cand_res.new_val != targ_res.new_val) {
> +			/* all candidates should result in the same relocation
> +			 * decision and value, otherwise it's dangerous to
> +			 * proceed due to ambiguity
> +			 */
> +			pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %u != %s %u\n",
> +				prog_name, relo_idx,
> +				cand_res.poison ? "failure" : "success", cand_res.new_val,
> +				targ_res.poison ? "failure" : "success", targ_res.new_val);
> +			return -EINVAL;
>  		}

Hi, Andrii. This approach is not friend to bpf_core_cast() if the struct
is not used in the vmlinux, but the kernel modules.

Take "struct nft_chain" for example. Following code will fail:
    struct nft_chain *chain = bpf_core_cast(ptr, struct nft_chain).

The bpf_core_cast() will record a BPF_CORE_TYPE_ID_TARGET relocation
for "struct nft_chain". The libbpf will find multi btf type of nft_chain
in the modules nf_tables, nft_reject, etc, and it will fail the verification
due to the "new_val", which is btf type id, not the same, even if all
the "struct nft_chain" are exactly the same in different kernel modules.

I think this is a common case. So how about we check the consistence of
struct nft_chain in all the candidate list, and use the first one if all of
them have exactly the same definition?

We can check all the members in the struct iteratively, and make
sure they are all the same.

Thanks!
Menglong Dong

>  
>  		cand_ids->data[j++] = cand_spec.spec[0].type_id;
> @@ -5042,13 +5115,18 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
>  	 * verifier. If it was an error, then verifier will complain and point
>  	 * to a specific instruction number in its log.
>  	 */
> -	if (j == 0)
> +	if (j == 0) {
>  		pr_debug("prog '%s': relo #%d: no matching targets found\n",
>  			 prog_name, relo_idx);
>  
> -	/* bpf_core_reloc_insn should know how to handle missing targ_spec */
> -	err = bpf_core_reloc_insn(prog, relo, relo_idx, &local_spec,
> -				  j ? &targ_spec : NULL);
> +		/* calculate single target relo result explicitly */
> +		err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, NULL, &targ_res);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* bpf_core_patch_insn() should know how to handle missing targ_spec */
> +	err = bpf_core_patch_insn(prog, relo, relo_idx, &targ_res);
>  	if (err) {
>  		pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
>  			prog_name, relo_idx, relo->insn_off, err);
> -- 
> 2.24.1
> 
> 

Re: [PATCH bpf-next 3/9] libbpf: improve relocation ambiguity detection

[Andrii Nakryiko]

On Mon, Jan 12, 2026 at 11:36 PM Menglong Dong <menglong.dong@linux.dev> wrote:
>
> On 2020/8/19 06:39 Andrii Nakryiko <andriin@fb.com> write:
> > Split the instruction patching logic into relocation value calculation and
> > application of relocation to instruction. Using this, evaluate relocation
> > against each matching candidate and validate that all candidates agree on
> > relocated value. If not, report ambiguity and fail load.
> >
> > This logic is necessary to avoid dangerous (however unlikely) accidental match
> > against two incompatible candidate types. Without this change, libbpf will
> > pick a random type as *the* candidate and apply potentially invalid
> > relocation.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 170 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 124 insertions(+), 46 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 2047e4ed0076..1ba458140f50 100644
> > --- a/tools/lib/bpf/libbpf.c
> [......]
> > @@ -5005,16 +5063,31 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
> >               if (err == 0)
> >                       continue;
> >
> > +             err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, &cand_spec, &cand_res);
> > +             if (err)
> > +                     return err;
> > +
> >               if (j == 0) {
> > +                     targ_res = cand_res;
> >                       targ_spec = cand_spec;
> >               } else if (cand_spec.bit_offset != targ_spec.bit_offset) {
> > -                     /* if there are many candidates, they should all
> > -                      * resolve to the same bit offset
> > +                     /* if there are many field relo candidates, they
> > +                      * should all resolve to the same bit offset
> >                        */
> > -                     pr_warn("prog '%s': relo #%d: offset ambiguity: %u != %u\n",
> > +                     pr_warn("prog '%s': relo #%d: field offset ambiguity: %u != %u\n",
> >                               prog_name, relo_idx, cand_spec.bit_offset,
> >                               targ_spec.bit_offset);
> >                       return -EINVAL;
> > +             } else if (cand_res.poison != targ_res.poison || cand_res.new_val != targ_res.new_val) {
> > +                     /* all candidates should result in the same relocation
> > +                      * decision and value, otherwise it's dangerous to
> > +                      * proceed due to ambiguity
> > +                      */
> > +                     pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %u != %s %u\n",
> > +                             prog_name, relo_idx,
> > +                             cand_res.poison ? "failure" : "success", cand_res.new_val,
> > +                             targ_res.poison ? "failure" : "success", targ_res.new_val);
> > +                     return -EINVAL;
> >               }
>
> Hi, Andrii. This approach is not friend to bpf_core_cast() if the struct
> is not used in the vmlinux, but the kernel modules.
>
> Take "struct nft_chain" for example. Following code will fail:
>     struct nft_chain *chain = bpf_core_cast(ptr, struct nft_chain).
>
> The bpf_core_cast() will record a BPF_CORE_TYPE_ID_TARGET relocation
> for "struct nft_chain". The libbpf will find multi btf type of nft_chain
> in the modules nf_tables, nft_reject, etc, and it will fail the verification
> due to the "new_val", which is btf type id, not the same, even if all
> the "struct nft_chain" are exactly the same in different kernel modules.
>
> I think this is a common case. So how about we check the consistence of
> struct nft_chain in all the candidate list, and use the first one if all of
> them have exactly the same definition?

BTF type ID for some type in some kernel is not meaningful without
also capturing module's BTF ID or FD, so we'd be just capturing some
relatively random and meaningless type ID.

I'm actually not sure bpf_core_cast() can work with BTF types defined
in module's BTF. Can you please check what we do if we have
non-ambiguous BTF type defined only in module's BTF?

>
> We can check all the members in the struct iteratively, and make
> sure they are all the same.
>

It's not even clear what "same" would mean here, btw. None of the
issues you bring up are easy to solve :)

> Thanks!
> Menglong Dong
>
> >
> >               cand_ids->data[j++] = cand_spec.spec[0].type_id;
> > @@ -5042,13 +5115,18 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
> >        * verifier. If it was an error, then verifier will complain and point
> >        * to a specific instruction number in its log.
> >        */
> > -     if (j == 0)
> > +     if (j == 0) {
> >               pr_debug("prog '%s': relo #%d: no matching targets found\n",
> >                        prog_name, relo_idx);
> >
> > -     /* bpf_core_reloc_insn should know how to handle missing targ_spec */
> > -     err = bpf_core_reloc_insn(prog, relo, relo_idx, &local_spec,
> > -                               j ? &targ_spec : NULL);
> > +             /* calculate single target relo result explicitly */
> > +             err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, NULL, &targ_res);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     /* bpf_core_patch_insn() should know how to handle missing targ_spec */
> > +     err = bpf_core_patch_insn(prog, relo, relo_idx, &targ_res);
> >       if (err) {
> >               pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
> >                       prog_name, relo_idx, relo->insn_off, err);
> > --
> > 2.24.1
> >
> >
>
>
>
>

Re: [PATCH bpf-next 3/9] libbpf: improve relocation ambiguity detection

[Menglong Dong]

On 2026/1/14 07:26 Andrii Nakryiko <andrii.nakryiko@gmail.com> write:
> On Mon, Jan 12, 2026 at 11:36 PM Menglong Dong <menglong.dong@linux.dev> wrote:
> >
> > On 2020/8/19 06:39 Andrii Nakryiko <andriin@fb.com> write:
> > > Split the instruction patching logic into relocation value calculation and
> > > application of relocation to instruction. Using this, evaluate relocation
> > > against each matching candidate and validate that all candidates agree on
> > > relocated value. If not, report ambiguity and fail load.
> > >
> > > This logic is necessary to avoid dangerous (however unlikely) accidental match
> > > against two incompatible candidate types. Without this change, libbpf will
> > > pick a random type as *the* candidate and apply potentially invalid
> > > relocation.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 170 ++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 124 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 2047e4ed0076..1ba458140f50 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > [......]
> > > @@ -5005,16 +5063,31 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
> > >               if (err == 0)
> > >                       continue;
> > >
> > > +             err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, &cand_spec, &cand_res);
> > > +             if (err)
> > > +                     return err;
> > > +
> > >               if (j == 0) {
> > > +                     targ_res = cand_res;
> > >                       targ_spec = cand_spec;
> > >               } else if (cand_spec.bit_offset != targ_spec.bit_offset) {
> > > -                     /* if there are many candidates, they should all
> > > -                      * resolve to the same bit offset
> > > +                     /* if there are many field relo candidates, they
> > > +                      * should all resolve to the same bit offset
> > >                        */
> > > -                     pr_warn("prog '%s': relo #%d: offset ambiguity: %u != %u\n",
> > > +                     pr_warn("prog '%s': relo #%d: field offset ambiguity: %u != %u\n",
> > >                               prog_name, relo_idx, cand_spec.bit_offset,
> > >                               targ_spec.bit_offset);
> > >                       return -EINVAL;
> > > +             } else if (cand_res.poison != targ_res.poison || cand_res.new_val != targ_res.new_val) {
> > > +                     /* all candidates should result in the same relocation
> > > +                      * decision and value, otherwise it's dangerous to
> > > +                      * proceed due to ambiguity
> > > +                      */
> > > +                     pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %u != %s %u\n",
> > > +                             prog_name, relo_idx,
> > > +                             cand_res.poison ? "failure" : "success", cand_res.new_val,
> > > +                             targ_res.poison ? "failure" : "success", targ_res.new_val);
> > > +                     return -EINVAL;
> > >               }
> >
> > Hi, Andrii. This approach is not friend to bpf_core_cast() if the struct
> > is not used in the vmlinux, but the kernel modules.
> >
> > Take "struct nft_chain" for example. Following code will fail:
> >     struct nft_chain *chain = bpf_core_cast(ptr, struct nft_chain).
> >
> > The bpf_core_cast() will record a BPF_CORE_TYPE_ID_TARGET relocation
> > for "struct nft_chain". The libbpf will find multi btf type of nft_chain
> > in the modules nf_tables, nft_reject, etc, and it will fail the verification
> > due to the "new_val", which is btf type id, not the same, even if all
> > the "struct nft_chain" are exactly the same in different kernel modules.
> >
> > I think this is a common case. So how about we check the consistence of
> > struct nft_chain in all the candidate list, and use the first one if all of
> > them have exactly the same definition?
> 
> BTF type ID for some type in some kernel is not meaningful without
> also capturing module's BTF ID or FD, so we'd be just capturing some
> relatively random and meaningless type ID.
> 
> I'm actually not sure bpf_core_cast() can work with BTF types defined
> in module's BTF. Can you please check what we do if we have
> non-ambiguous BTF type defined only in module's BTF?

You are right. I got the following error when I use bpf_core_cast()
for the struct that in kernel module:
    Unknown type ID 142301 passed to kfunc bpf_rdonly_cast

It seems that I didn't realize the kernel side. The module's BTF ID
or FD for the struct is unknown for the kernel, and it will only
lookup it in the btf that the kfunc belongs to.

> 
> >
> > We can check all the members in the struct iteratively, and make
> > sure they are all the same.
> >
> 
> It's not even clear what "same" would mean here, btw. None of the
> issues you bring up are easy to solve :)

Yeah, thing is much more complex than I thought. I think I'd better
use bpf_probe_kernel_read() for such case ;)

Thanks!
Menglong Dong

> 
> > Thanks!
> > Menglong Dong
> >
> > >
> > >               cand_ids->data[j++] = cand_spec.spec[0].type_id;
> > > @@ -5042,13 +5115,18 @@ static int bpf_core_reloc_field(struct bpf_program *prog,
> > >        * verifier. If it was an error, then verifier will complain and point
> > >        * to a specific instruction number in its log.
> > >        */
> > > -     if (j == 0)
> > > +     if (j == 0) {
> > >               pr_debug("prog '%s': relo #%d: no matching targets found\n",
> > >                        prog_name, relo_idx);
> > >
> > > -     /* bpf_core_reloc_insn should know how to handle missing targ_spec */
> > > -     err = bpf_core_reloc_insn(prog, relo, relo_idx, &local_spec,
> > > -                               j ? &targ_spec : NULL);
> > > +             /* calculate single target relo result explicitly */
> > > +             err = bpf_core_calc_relo(prog, relo, relo_idx, &local_spec, NULL, &targ_res);
> > > +             if (err)
> > > +                     return err;
> > > +     }
> > > +
> > > +     /* bpf_core_patch_insn() should know how to handle missing targ_spec */
> > > +     err = bpf_core_patch_insn(prog, relo, relo_idx, &targ_res);
> > >       if (err) {
> > >               pr_warn("prog '%s': relo #%d: failed to patch insn at offset %d: %d\n",
> > >                       prog_name, relo_idx, relo->insn_off, err);
> > > --
> > > 2.24.1
> > >
> > >
> >
> >
> >
> >