| Message ID | 20220501190012.2577087-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add 64bit enum value support | expand |
On 5/1/22 3:00 PM, Yonghong Song wrote: > Currently, the libbpf limits the relocation value to be 32bit > since all current relocations have such a limit. But with > BTF_KIND_ENUM64 support, the enum value could be 64bit. > So let us permit 64bit relocation value in libbpf. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ > tools/lib/bpf/relo_core.h | 4 ++-- > 2 files changed, 14 insertions(+), 14 deletions(-) [...] > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c > index ba4453dfd1ed..2ed94daabbe5 100644 > --- a/tools/lib/bpf/relo_core.c > +++ b/tools/lib/bpf/relo_core.c [...] > @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > > insn[0].imm = new_val; > insn[1].imm = 0; /* currently only 32-bit values are supported */ > - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", > + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", > prog_name, relo_idx, insn_idx, > (unsigned long long)imm, new_val); > break; Since new_val is 64bit now, should the insn[1].imm be set here, and the comment about 32-bit be removed?
On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: > > Currently, the libbpf limits the relocation value to be 32bit > since all current relocations have such a limit. But with > BTF_KIND_ENUM64 support, the enum value could be 64bit. > So let us permit 64bit relocation value in libbpf. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ > tools/lib/bpf/relo_core.h | 4 ++-- > 2 files changed, 14 insertions(+), 14 deletions(-) > [...] > @@ -929,7 +929,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > int insn_idx, const struct bpf_core_relo *relo, > int relo_idx, const struct bpf_core_relo_res *res) > { > - __u32 orig_val, new_val; > + __u64 orig_val, new_val; > __u8 class; > > class = BPF_CLASS(insn->code); > @@ -954,14 +954,14 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > if (BPF_SRC(insn->code) != BPF_K) > return -EINVAL; > if (res->validate && insn->imm != orig_val) { > - pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n", > + pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %llu -> %llu\n", > prog_name, relo_idx, > insn_idx, insn->imm, orig_val, new_val); %llu is not valid formatter for __u64 on all architectures, please add explicit (unsigned long long) cast but also in general for non-ldimm64 instructions we need to check that new value fits in 32 bits [...] > @@ -1026,7 +1026,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > > imm = insn[0].imm + ((__u64)insn[1].imm << 32); > if (res->validate && imm != orig_val) { > - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", > + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %llu -> %llu\n", > prog_name, relo_idx, > insn_idx, (unsigned long long)imm, > orig_val, new_val); > @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > > insn[0].imm = new_val; > insn[1].imm = 0; /* currently only 32-bit values are supported */ as Dave mentioned, not anymore, so this should take higher 32-bit of new_val > - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", > + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", > prog_name, relo_idx, insn_idx, > (unsigned long long)imm, new_val); > break; > @@ -1261,7 +1261,7 @@ int bpf_core_calc_relo_insn(const char *prog_name, > * 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", > + pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %llu != %s %llu\n", > prog_name, relo_idx, > cand_res.poison ? "failure" : "success", cand_res.new_val, > targ_res->poison ? "failure" : "success", targ_res->new_val); > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h > index 073039d8ca4f..7df0da082f2c 100644 > --- a/tools/lib/bpf/relo_core.h > +++ b/tools/lib/bpf/relo_core.h > @@ -46,9 +46,9 @@ struct bpf_core_spec { > > struct bpf_core_relo_res { > /* expected value in the instruction, unless validate == false */ > - __u32 orig_val; > + __u64 orig_val; > /* new value that needs to be patched up to */ > - __u32 new_val; > + __u64 new_val; > /* relocation unsuccessful, poison instruction, but don't fail load */ > bool poison; > /* some relocations can't be validated against orig_val */ > -- > 2.30.2 >
On 5/8/22 6:06 PM, Dave Marchevsky wrote: > On 5/1/22 3:00 PM, Yonghong Song wrote: >> Currently, the libbpf limits the relocation value to be 32bit >> since all current relocations have such a limit. But with >> BTF_KIND_ENUM64 support, the enum value could be 64bit. >> So let us permit 64bit relocation value in libbpf. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ >> tools/lib/bpf/relo_core.h | 4 ++-- >> 2 files changed, 14 insertions(+), 14 deletions(-) > > [...] > >> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c >> index ba4453dfd1ed..2ed94daabbe5 100644 >> --- a/tools/lib/bpf/relo_core.c >> +++ b/tools/lib/bpf/relo_core.c > > [...] > > >> @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, >> >> insn[0].imm = new_val; >> insn[1].imm = 0; /* currently only 32-bit values are supported */ >> - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", >> + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", >> prog_name, relo_idx, insn_idx, >> (unsigned long long)imm, new_val); >> break; > > Since new_val is 64bit now, should the insn[1].imm be set here, and the comment > about 32-bit be removed? The comment and setting of insn[1].imm are changed in patch #4. But yes, I can move the change here as well.
On 5/9/22 3:37 PM, Andrii Nakryiko wrote: > On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: >> >> Currently, the libbpf limits the relocation value to be 32bit >> since all current relocations have such a limit. But with >> BTF_KIND_ENUM64 support, the enum value could be 64bit. >> So let us permit 64bit relocation value in libbpf. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ >> tools/lib/bpf/relo_core.h | 4 ++-- >> 2 files changed, 14 insertions(+), 14 deletions(-) >> > > [...] > >> @@ -929,7 +929,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, >> int insn_idx, const struct bpf_core_relo *relo, >> int relo_idx, const struct bpf_core_relo_res *res) >> { >> - __u32 orig_val, new_val; >> + __u64 orig_val, new_val; >> __u8 class; >> >> class = BPF_CLASS(insn->code); >> @@ -954,14 +954,14 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, >> if (BPF_SRC(insn->code) != BPF_K) >> return -EINVAL; >> if (res->validate && insn->imm != orig_val) { >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n", >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %llu -> %llu\n", >> prog_name, relo_idx, >> insn_idx, insn->imm, orig_val, new_val); > > %llu is not valid formatter for __u64 on all architectures, please add > explicit (unsigned long long) cast Okay, will do. > > but also in general for non-ldimm64 instructions we need to check that > new value fits in 32 bits The real 64-bit value can only be retrieved for ldimm64 insn, so I suppose it should be fine here. But let me double check. > > [...] > >> @@ -1026,7 +1026,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, >> >> imm = insn[0].imm + ((__u64)insn[1].imm << 32); >> if (res->validate && imm != orig_val) { >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %llu -> %llu\n", >> prog_name, relo_idx, >> insn_idx, (unsigned long long)imm, >> orig_val, new_val); >> @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, >> >> insn[0].imm = new_val; >> insn[1].imm = 0; /* currently only 32-bit values are supported */ > > as Dave mentioned, not anymore, so this should take higher 32-bit of new_val Will do. > > >> - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", >> + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", >> prog_name, relo_idx, insn_idx, >> (unsigned long long)imm, new_val); >> break; >> @@ -1261,7 +1261,7 @@ int bpf_core_calc_relo_insn(const char *prog_name, >> * 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", >> + pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %llu != %s %llu\n", >> prog_name, relo_idx, >> cand_res.poison ? "failure" : "success", cand_res.new_val, >> targ_res->poison ? "failure" : "success", targ_res->new_val); [...]
On Tue, May 10, 2022 at 3:14 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 5/9/22 3:37 PM, Andrii Nakryiko wrote: > > On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> Currently, the libbpf limits the relocation value to be 32bit > >> since all current relocations have such a limit. But with > >> BTF_KIND_ENUM64 support, the enum value could be 64bit. > >> So let us permit 64bit relocation value in libbpf. > >> > >> Signed-off-by: Yonghong Song <yhs@fb.com> > >> --- > >> tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ > >> tools/lib/bpf/relo_core.h | 4 ++-- > >> 2 files changed, 14 insertions(+), 14 deletions(-) > >> > > > > [...] > > > >> @@ -929,7 +929,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> int insn_idx, const struct bpf_core_relo *relo, > >> int relo_idx, const struct bpf_core_relo_res *res) > >> { > >> - __u32 orig_val, new_val; > >> + __u64 orig_val, new_val; > >> __u8 class; > >> > >> class = BPF_CLASS(insn->code); > >> @@ -954,14 +954,14 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> if (BPF_SRC(insn->code) != BPF_K) > >> return -EINVAL; > >> if (res->validate && insn->imm != orig_val) { > >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n", > >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %llu -> %llu\n", > >> prog_name, relo_idx, > >> insn_idx, insn->imm, orig_val, new_val); > > > > %llu is not valid formatter for __u64 on all architectures, please add > > explicit (unsigned long long) cast > > Okay, will do. > > > > > but also in general for non-ldimm64 instructions we need to check that > > new value fits in 32 bits > > The real 64-bit value can only be retrieved for ldimm64 insn, so I > suppose it should be fine here. But let me double check. So, technically (I don't think that happens in practice, though), you can have ALU operation with a local 32-bit enum with some reasonable value, which in the kernel is actually ENUM64 with huge value. > > > > > [...] > > > >> @@ -1026,7 +1026,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> > >> imm = insn[0].imm + ((__u64)insn[1].imm << 32); > >> if (res->validate && imm != orig_val) { > >> - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", > >> + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %llu -> %llu\n", > >> prog_name, relo_idx, > >> insn_idx, (unsigned long long)imm, > >> orig_val, new_val); > >> @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, > >> > >> insn[0].imm = new_val; > >> insn[1].imm = 0; /* currently only 32-bit values are supported */ > > > > as Dave mentioned, not anymore, so this should take higher 32-bit of new_val > > Will do. > > > > > > >> - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", > >> + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", > >> prog_name, relo_idx, insn_idx, > >> (unsigned long long)imm, new_val); > >> break; > >> @@ -1261,7 +1261,7 @@ int bpf_core_calc_relo_insn(const char *prog_name, > >> * 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", > >> + pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %llu != %s %llu\n", > >> prog_name, relo_idx, > >> cand_res.poison ? "failure" : "success", cand_res.new_val, > >> targ_res->poison ? "failure" : "success", targ_res->new_val); > [...]
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c index ba4453dfd1ed..2ed94daabbe5 100644 --- a/tools/lib/bpf/relo_core.c +++ b/tools/lib/bpf/relo_core.c @@ -583,7 +583,7 @@ static int bpf_core_spec_match(struct bpf_core_spec *local_spec, static int bpf_core_calc_field_relo(const char *prog_name, const struct bpf_core_relo *relo, const struct bpf_core_spec *spec, - __u32 *val, __u32 *field_sz, __u32 *type_id, + __u64 *val, __u32 *field_sz, __u32 *type_id, bool *validate) { const struct bpf_core_accessor *acc; @@ -708,7 +708,7 @@ static int bpf_core_calc_field_relo(const char *prog_name, static int bpf_core_calc_type_relo(const struct bpf_core_relo *relo, const struct bpf_core_spec *spec, - __u32 *val, bool *validate) + __u64 *val, bool *validate) { __s64 sz; @@ -751,7 +751,7 @@ static int bpf_core_calc_type_relo(const struct bpf_core_relo *relo, static int bpf_core_calc_enumval_relo(const struct bpf_core_relo *relo, const struct bpf_core_spec *spec, - __u32 *val) + __u64 *val) { const struct btf_type *t; const struct btf_enum *e; @@ -929,7 +929,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, int insn_idx, const struct bpf_core_relo *relo, int relo_idx, const struct bpf_core_relo_res *res) { - __u32 orig_val, new_val; + __u64 orig_val, new_val; __u8 class; class = BPF_CLASS(insn->code); @@ -954,14 +954,14 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, if (BPF_SRC(insn->code) != BPF_K) return -EINVAL; if (res->validate && insn->imm != orig_val) { - pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %u -> %u\n", + pr_warn("prog '%s': relo #%d: unexpected insn #%d (ALU/ALU64) value: got %u, exp %llu -> %llu\n", prog_name, relo_idx, insn_idx, insn->imm, orig_val, new_val); return -EINVAL; } orig_val = insn->imm; insn->imm = new_val; - pr_debug("prog '%s': relo #%d: patched insn #%d (ALU/ALU64) imm %u -> %u\n", + pr_debug("prog '%s': relo #%d: patched insn #%d (ALU/ALU64) imm %llu -> %llu\n", prog_name, relo_idx, insn_idx, orig_val, new_val); break; @@ -969,12 +969,12 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, case BPF_ST: case BPF_STX: if (res->validate && insn->off != orig_val) { - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDX/ST/STX) value: got %u, exp %u -> %u\n", + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDX/ST/STX) value: got %u, exp %llu -> %llu\n", prog_name, relo_idx, insn_idx, insn->off, orig_val, new_val); return -EINVAL; } if (new_val > SHRT_MAX) { - pr_warn("prog '%s': relo #%d: insn #%d (LDX/ST/STX) value too big: %u\n", + pr_warn("prog '%s': relo #%d: insn #%d (LDX/ST/STX) value too big: %llu\n", prog_name, relo_idx, insn_idx, new_val); return -ERANGE; } @@ -987,7 +987,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, orig_val = insn->off; insn->off = new_val; - pr_debug("prog '%s': relo #%d: patched insn #%d (LDX/ST/STX) off %u -> %u\n", + pr_debug("prog '%s': relo #%d: patched insn #%d (LDX/ST/STX) off %llu -> %llu\n", prog_name, relo_idx, insn_idx, orig_val, new_val); if (res->new_sz != res->orig_sz) { @@ -1026,7 +1026,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, imm = insn[0].imm + ((__u64)insn[1].imm << 32); if (res->validate && imm != orig_val) { - pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", + pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %llu -> %llu\n", prog_name, relo_idx, insn_idx, (unsigned long long)imm, orig_val, new_val); @@ -1035,7 +1035,7 @@ int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn, insn[0].imm = new_val; insn[1].imm = 0; /* currently only 32-bit values are supported */ - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", prog_name, relo_idx, insn_idx, (unsigned long long)imm, new_val); break; @@ -1261,7 +1261,7 @@ int bpf_core_calc_relo_insn(const char *prog_name, * 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", + pr_warn("prog '%s': relo #%d: relocation decision ambiguity: %s %llu != %s %llu\n", prog_name, relo_idx, cand_res.poison ? "failure" : "success", cand_res.new_val, targ_res->poison ? "failure" : "success", targ_res->new_val); diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h index 073039d8ca4f..7df0da082f2c 100644 --- a/tools/lib/bpf/relo_core.h +++ b/tools/lib/bpf/relo_core.h @@ -46,9 +46,9 @@ struct bpf_core_spec { struct bpf_core_relo_res { /* expected value in the instruction, unless validate == false */ - __u32 orig_val; + __u64 orig_val; /* new value that needs to be patched up to */ - __u32 new_val; + __u64 new_val; /* relocation unsuccessful, poison instruction, but don't fail load */ bool poison; /* some relocations can't be validated against orig_val */
Currently, the libbpf limits the relocation value to be 32bit since all current relocations have such a limit. But with BTF_KIND_ENUM64 support, the enum value could be 64bit. So let us permit 64bit relocation value in libbpf. Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/relo_core.c | 24 ++++++++++++------------ tools/lib/bpf/relo_core.h | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-)