| Message ID | 20211028063501.2239335-4-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | Typeless/weak ksym for gen_loader + misc fixups | expand |
On Thu, Oct 28, 2021 at 12:04:56PM +0530, Kumar Kartikeya Dwivedi wrote: > This extends existing ksym relocation code to also support relocating > weak ksyms. Care needs to be taken to zero out the src_reg (currently > BPF_PSEUOD_BTF_ID, always set for gen_loader by bpf_object__relocate_data) > when the BTF ID lookup fails at runtime. This is not a problem for > libbpf as it only sets ext->is_set when BTF ID lookup succeeds (and only > proceeds in case of failure if ext->is_weak, leading to src_reg > remaining as 0 for weak unresolved ksym). > > A pattern similar to emit_relo_kfunc_btf is followed of first storing > the default values and then jumping over actual stores in case of an > error. For src_reg adjustment, we also need to perform it when copying > the populated instruction, so depending on if copied insn[0].imm is 0 or > not, we decide to jump over the adjustment. > > We cannot reach that point unless the ksym was weak and resolved and > zeroed out, as the emit_check_err will cause us to jump to cleanup > label, so we do not need to recheck whether the ksym is weak before > doing the adjustment after copying BTF ID and BTF FD. > > This is consistent with how libbpf relocates weak ksym. Logging > statements are added to show the relocation result and aid debugging. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/lib/bpf/gen_loader.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c > index 11172a868180..1c404752e565 100644 > --- a/tools/lib/bpf/gen_loader.c > +++ b/tools/lib/bpf/gen_loader.c > @@ -13,6 +13,7 @@ > #include "hashmap.h" > #include "bpf_gen_internal.h" > #include "skel_internal.h" > +#include <asm/byteorder.h> > > #define MAX_USED_MAPS 64 > #define MAX_USED_PROGS 32 > @@ -776,12 +777,24 @@ static void emit_relo_ksym_typeless(struct bpf_gen *gen, > emit_ksym_relo_log(gen, relo, kdesc->ref); > } > > +static __u32 src_reg_mask(void) > +{ > +#if defined(__LITTLE_ENDIAN_BITFIELD) > + return 0x0f; /* src_reg,dst_reg,... */ > +#elif defined(__BIG_ENDIAN_BITFIELD) > + return 0xf0; /* dst_reg,src_reg,... */ > +#else > +#error "Unsupported bit endianness, cannot proceed" > +#endif > +} > + > /* Expects: > * BPF_REG_8 - pointer to instruction > */ > static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn) > { > struct ksym_desc *kdesc; > + __u32 reg_mask; > > kdesc = get_ksym_desc(gen, relo); > if (!kdesc) > @@ -792,19 +805,35 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, > kdesc->insn + offsetof(struct bpf_insn, imm)); > move_blob2blob(gen, insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 4, > kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)); > - goto log; > + emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_8, offsetof(struct bpf_insn, imm))); Thanks a lot for working on this. I've applied the set. The above load is redundant, right? BPF_REG_0 already has that value and could have been used in the JNE below, right? > + /* jump over src_reg adjustment if imm is not 0 */ > + emit(gen, BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0, 3)); > + goto clear_src_reg; Is there a test for this part of the code? It's only for weak && unresolved && multi referenced ksym, right? Or bpf_link_fops2 test_ksyms_weak.c fits this category? > } > /* remember insn offset, so we can copy BTF ID and FD later */ > kdesc->insn = insn; > emit_bpf_find_by_name_kind(gen, relo); > - emit_check_err(gen); > + if (!relo->is_weak) > + emit_check_err(gen); > + /* set default values as 0 */ > + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, offsetof(struct bpf_insn, imm), 0)); > + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 0)); > + /* skip success case stores if ret < 0 */ > + emit(gen, BPF_JMP_IMM(BPF_JSLT, BPF_REG_7, 0, 4)); > /* store btf_id into insn[insn_idx].imm */ > emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, offsetof(struct bpf_insn, imm))); > /* store btf_obj_fd into insn[insn_idx + 1].imm */ > emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32)); > emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, > sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm))); The double store (first with zeros and then with real values) doesn't look pretty. I think an extra jump over two stores would have been cleaner.
On Fri, Oct 29, 2021 at 05:52:44AM IST, Alexei Starovoitov wrote: > On Thu, Oct 28, 2021 at 12:04:56PM +0530, Kumar Kartikeya Dwivedi wrote: > > This extends existing ksym relocation code to also support relocating > > weak ksyms. Care needs to be taken to zero out the src_reg (currently > > BPF_PSEUOD_BTF_ID, always set for gen_loader by bpf_object__relocate_data) > > when the BTF ID lookup fails at runtime. This is not a problem for > > libbpf as it only sets ext->is_set when BTF ID lookup succeeds (and only > > proceeds in case of failure if ext->is_weak, leading to src_reg > > remaining as 0 for weak unresolved ksym). > > > > A pattern similar to emit_relo_kfunc_btf is followed of first storing > > the default values and then jumping over actual stores in case of an > > error. For src_reg adjustment, we also need to perform it when copying > > the populated instruction, so depending on if copied insn[0].imm is 0 or > > not, we decide to jump over the adjustment. > > > > We cannot reach that point unless the ksym was weak and resolved and > > zeroed out, as the emit_check_err will cause us to jump to cleanup > > label, so we do not need to recheck whether the ksym is weak before > > doing the adjustment after copying BTF ID and BTF FD. > > > > This is consistent with how libbpf relocates weak ksym. Logging > > statements are added to show the relocation result and aid debugging. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > tools/lib/bpf/gen_loader.c | 35 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 32 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c > > index 11172a868180..1c404752e565 100644 > > --- a/tools/lib/bpf/gen_loader.c > > +++ b/tools/lib/bpf/gen_loader.c > > @@ -13,6 +13,7 @@ > > #include "hashmap.h" > > #include "bpf_gen_internal.h" > > #include "skel_internal.h" > > +#include <asm/byteorder.h> > > > > #define MAX_USED_MAPS 64 > > #define MAX_USED_PROGS 32 > > @@ -776,12 +777,24 @@ static void emit_relo_ksym_typeless(struct bpf_gen *gen, > > emit_ksym_relo_log(gen, relo, kdesc->ref); > > } > > > > +static __u32 src_reg_mask(void) > > +{ > > +#if defined(__LITTLE_ENDIAN_BITFIELD) > > + return 0x0f; /* src_reg,dst_reg,... */ > > +#elif defined(__BIG_ENDIAN_BITFIELD) > > + return 0xf0; /* dst_reg,src_reg,... */ > > +#else > > +#error "Unsupported bit endianness, cannot proceed" > > +#endif > > +} > > + > > /* Expects: > > * BPF_REG_8 - pointer to instruction > > */ > > static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn) > > { > > struct ksym_desc *kdesc; > > + __u32 reg_mask; > > > > kdesc = get_ksym_desc(gen, relo); > > if (!kdesc) > > @@ -792,19 +805,35 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, > > kdesc->insn + offsetof(struct bpf_insn, imm)); > > move_blob2blob(gen, insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 4, > > kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)); > > - goto log; > > + emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_8, offsetof(struct bpf_insn, imm))); > > Thanks a lot for working on this. I've applied the set. > > The above load is redundant, right? BPF_REG_0 already has that value > and could have been used in the JNE below, right? > Hm, true, we could certainly avoid another load here. > > + /* jump over src_reg adjustment if imm is not 0 */ > > + emit(gen, BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0, 3)); > > + goto clear_src_reg; > > Is there a test for this part of the code? > It's only for weak && unresolved && multi referenced ksym, right? Correct. > Or bpf_link_fops2 test_ksyms_weak.c fits this category? > Yes, the result of relocation is as follows (t=0 means typed, w=1 means weak): find_by_name_kind(bpf_link_fops2,14) r=-2 var t=0 w=1 (bpf_link_fops2:count=1): imm[0]: 0, imm[1]: 0 var t=0 w=1 (bpf_link_fops2:count=1): insn.reg r=1 // goto clear_src_reg happens for this one var t=0 w=1 (bpf_link_fops2:count=2): imm[0]: 0, imm[1]: 0 var t=0 w=1 (bpf_link_fops2:count=2): insn.reg r=1 > > } > > /* remember insn offset, so we can copy BTF ID and FD later */ > > kdesc->insn = insn; > > emit_bpf_find_by_name_kind(gen, relo); > > - emit_check_err(gen); > > + if (!relo->is_weak) > > + emit_check_err(gen); > > + /* set default values as 0 */ > > + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, offsetof(struct bpf_insn, imm), 0)); > > + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 0)); > > + /* skip success case stores if ret < 0 */ > > + emit(gen, BPF_JMP_IMM(BPF_JSLT, BPF_REG_7, 0, 4)); > > /* store btf_id into insn[insn_idx].imm */ > > emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, offsetof(struct bpf_insn, imm))); > > /* store btf_obj_fd into insn[insn_idx + 1].imm */ > > emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32)); > > emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, > > sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm))); > > The double store (first with zeros and then with real values) doesn't look pretty. > I think an extra jump over two stores would have been cleaner. I will address all your (and Andrii's) points in another patch. -- Kartikeya
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c index 11172a868180..1c404752e565 100644 --- a/tools/lib/bpf/gen_loader.c +++ b/tools/lib/bpf/gen_loader.c @@ -13,6 +13,7 @@ #include "hashmap.h" #include "bpf_gen_internal.h" #include "skel_internal.h" +#include <asm/byteorder.h> #define MAX_USED_MAPS 64 #define MAX_USED_PROGS 32 @@ -776,12 +777,24 @@ static void emit_relo_ksym_typeless(struct bpf_gen *gen, emit_ksym_relo_log(gen, relo, kdesc->ref); } +static __u32 src_reg_mask(void) +{ +#if defined(__LITTLE_ENDIAN_BITFIELD) + return 0x0f; /* src_reg,dst_reg,... */ +#elif defined(__BIG_ENDIAN_BITFIELD) + return 0xf0; /* dst_reg,src_reg,... */ +#else +#error "Unsupported bit endianness, cannot proceed" +#endif +} + /* Expects: * BPF_REG_8 - pointer to instruction */ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn) { struct ksym_desc *kdesc; + __u32 reg_mask; kdesc = get_ksym_desc(gen, relo); if (!kdesc) @@ -792,19 +805,35 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, kdesc->insn + offsetof(struct bpf_insn, imm)); move_blob2blob(gen, insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 4, kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)); - goto log; + emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_8, offsetof(struct bpf_insn, imm))); + /* jump over src_reg adjustment if imm is not 0 */ + emit(gen, BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0, 3)); + goto clear_src_reg; } /* remember insn offset, so we can copy BTF ID and FD later */ kdesc->insn = insn; emit_bpf_find_by_name_kind(gen, relo); - emit_check_err(gen); + if (!relo->is_weak) + emit_check_err(gen); + /* set default values as 0 */ + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, offsetof(struct bpf_insn, imm), 0)); + emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 0)); + /* skip success case stores if ret < 0 */ + emit(gen, BPF_JMP_IMM(BPF_JSLT, BPF_REG_7, 0, 4)); /* store btf_id into insn[insn_idx].imm */ emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, offsetof(struct bpf_insn, imm))); /* store btf_obj_fd into insn[insn_idx + 1].imm */ emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32)); emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm))); -log: + emit(gen, BPF_JMP_IMM(BPF_JSGE, BPF_REG_7, 0, 3)); +clear_src_reg: + /* clear bpf_object__relocate_data's src_reg assignment, otherwise we get a verifier failure */ + reg_mask = src_reg_mask(); + emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code))); + emit(gen, BPF_ALU32_IMM(BPF_AND, BPF_REG_9, reg_mask)); + emit(gen, BPF_STX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, offsetofend(struct bpf_insn, code))); + emit_ksym_relo_log(gen, relo, kdesc->ref); }
This extends existing ksym relocation code to also support relocating weak ksyms. Care needs to be taken to zero out the src_reg (currently BPF_PSEUOD_BTF_ID, always set for gen_loader by bpf_object__relocate_data) when the BTF ID lookup fails at runtime. This is not a problem for libbpf as it only sets ext->is_set when BTF ID lookup succeeds (and only proceeds in case of failure if ext->is_weak, leading to src_reg remaining as 0 for weak unresolved ksym). A pattern similar to emit_relo_kfunc_btf is followed of first storing the default values and then jumping over actual stores in case of an error. For src_reg adjustment, we also need to perform it when copying the populated instruction, so depending on if copied insn[0].imm is 0 or not, we decide to jump over the adjustment. We cannot reach that point unless the ksym was weak and resolved and zeroed out, as the emit_check_err will cause us to jump to cleanup label, so we do not need to recheck whether the ksym is weak before doing the adjustment after copying BTF ID and BTF FD. This is consistent with how libbpf relocates weak ksym. Logging statements are added to show the relocation result and aid debugging. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/lib/bpf/gen_loader.c | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-)