On Tue, Feb 23, 2021 at 11:47 AM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 2/23/21 11:19 AM, Andrii Nakryiko wrote:
> > On Tue, Feb 23, 2021 at 10:56 AM Yonghong Song <yhs@xxxxxx> wrote:
> >>
> >>
> >>
> >> On 2/23/21 12:03 AM, Andrii Nakryiko wrote:
> >>> On Wed, Feb 17, 2021 at 12:56 PM Yonghong Song <yhs@xxxxxx> wrote:
> >>>>
> >>>> A new relocation RELO_SUBPROG_ADDR is added to capture
> >>>> local (static) function pointers loaded with ld_imm64
> >>>> insns. Such ld_imm64 insns are marked with
> >>>> BPF_PSEUDO_FUNC and will be passed to kernel so
> >>>> kernel can replace them with proper actual jited
> >>>> func addresses.
> >>>>
> >>>> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> >>>> ---
> >>>> tools/lib/bpf/libbpf.c | 40 +++++++++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 37 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>>> index 21a3eedf070d..772c7455f1a2 100644
> >>>> --- a/tools/lib/bpf/libbpf.c
> >>>> +++ b/tools/lib/bpf/libbpf.c
> >>>> @@ -188,6 +188,7 @@ enum reloc_type {
> >>>> RELO_CALL,
> >>>> RELO_DATA,
> >>>> RELO_EXTERN,
> >>>> + RELO_SUBPROG_ADDR,
> >>>> };
> >>>>
> >>>> struct reloc_desc {
> >>>> @@ -579,6 +580,11 @@ static bool is_ldimm64(struct bpf_insn *insn)
> >>>> return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> >>>> }
> >>>>
> >>>> +static bool insn_is_pseudo_func(struct bpf_insn *insn)
> >>>> +{
> >>>> + return is_ldimm64(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
> >>>> +}
> >>>> +
> >>>> static int
> >>>> bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
> >>>> const char *name, size_t sec_idx, const char *sec_name,
> >>>> @@ -3406,6 +3412,16 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
> >>>> return -LIBBPF_ERRNO__RELOC;
> >>>> }
> >>>>
> >>>> + if (GELF_ST_BIND(sym->st_info) == STB_LOCAL &&
> >>>> + GELF_ST_TYPE(sym->st_info) == STT_SECTION &&
> >>>
> >>> STB_LOCAL + STT_SECTION is a section symbol. But STT_FUNC symbol could
> >>> be referenced as well, no? So this is too strict.
> >>
> >> Yes, STT_FUNC symbol could be referenced but we do not have use
> >> case yet. If we encode STT_FUNC (global), the kernel will reject
> >> it. We can extend libbpf to support STT_FUNC once we got a use
> >> case.
> >
> > I don't really like tailoring libbpf generic SUBPROG_ADDR relocation
> > to one current specific use case, though. Taking the address of
> > SUBPROG_ADDR is not, conceptually, tied with passing it to for_each as
> > a callback. E.g., what if you just want to compare two registers
> > pointing to subprogs, without actually passing them to for_each(). I
> > don't know if it's possible right now, but I don't see why that
> > shouldn't be supported. In the latter case, adding arbitrary
> > restrictions about static vs global functions doesn't make much sense.
> >
> > So let's teach libbpf the right logic without assuming any specific
> > use case. It pays off in the long run.
>
> Okay, Will support global function as well. It won't hurt.
>
> >
> >>
> >>>
> >>>> + (!shdr_idx || shdr_idx == obj->efile.text_shndx) &&
> >>>
> >>> this doesn't look right, shdr_idx == 0 is a bad condition and should
> >>> be rejected, not accepted.
> >>
> >> it is my fault. Will fix in the next revision.
> >>
> >>>
> >>>> + !(sym->st_value % BPF_INSN_SZ)) {
> >>>> + reloc_desc->type = RELO_SUBPROG_ADDR;
> >>>> + reloc_desc->insn_idx = insn_idx;
> >>>> + reloc_desc->sym_off = sym->st_value;
> >>>> + return 0;
> >>>> + }
> >>>> +
> >>>
> >>> So see code right after sym_is_extern(sym) check. It checks for valid
> >>> shrd_idx, which is good and would be good to use that. After that we
> >>> can assume shdr_idx is valid and we can make a simple
> >>> obj->efile.text_shndx check then and use that as a signal that this is
> >>> SUBPROG_ADDR relocation (instead of deducing that from STT_SECTION).
> >>>
> >>> And !(sym->st_value % BPF_INSN_SZ) should be reported as an error, not
> >>> silently skipped. Again, just how BPF_JMP | BPF_CALL does it. That way
> >>> it's more user-friendly, if something goes wrong. So it will look like
> >>> this:
> >>>
> >>> if (shdr_idx == obj->efile.text_shndx) {
> >>> /* check sym->st_value, pr_warn(), return error */
> >>>
> >>> reloc_desc->type = RELO_SUBPROG_ADDR;
> >>> ...
> >>> return 0;
> >>> }
> >>
> >> Okay. Will do similar checking to insn->code == (BPF_JMP | BPF_CALL)
> >> in the next revision.
> >>
> >>>
> >>>> if (sym_is_extern(sym)) {
> >>>> int sym_idx = GELF_R_SYM(rel->r_info);
> >>>> int i, n = obj->nr_extern;
> >>>> @@ -6172,6 +6188,10 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
> >>>> }
> >>>> relo->processed = true;
> >>>> break;
> >>>> + case RELO_SUBPROG_ADDR:
> >>>> + insn[0].src_reg = BPF_PSEUDO_FUNC;
> >>>
> >>> BTW, doesn't Clang emit instruction with BPF_PSEUDO_FUNC set properly
> >>> already? If not, why not?
> >>
> >> This is really a contract between libbpf and kernel, similar to
> >> BPF_PSEUDO_MAP_FD/BPF_PSEUDO_MAP_VALUE/BPF_PSEUDO_BTF_ID.
> >> Adding encoding in clang is not needed as this is simply a load
> >> of function address as far as clang concerned.
> >
> > Yeah, not a big deal, I was under the impression we do that for other
> > BPF_PSEUDO cases, but checking other parts of libbpf, doesn't seem
> > like that's the case.
> >
> >>
> >>>
> >>>> + /* will be handled as a follow up pass */
> >>>> + break;
> >>>> case RELO_CALL:
> >>>> /* will be handled as a follow up pass */
> >>>> break;
> >>>> @@ -6358,11 +6378,11 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
> >>>>
> >>>> for (insn_idx = 0; insn_idx < prog->sec_insn_cnt; insn_idx++) {
> >>>> insn = &main_prog->insns[prog->sub_insn_off + insn_idx];
> >>>> - if (!insn_is_subprog_call(insn))
> >>>> + if (!insn_is_subprog_call(insn) && !insn_is_pseudo_func(insn))
> >>>> continue;
> >>>>
> >>>> relo = find_prog_insn_relo(prog, insn_idx);
> >>>> - if (relo && relo->type != RELO_CALL) {
> >>>> + if (relo && relo->type != RELO_CALL && relo->type != RELO_SUBPROG_ADDR) {
> >>>> pr_warn("prog '%s': unexpected relo for insn #%zu, type %d\n",
> >>>> prog->name, insn_idx, relo->type);
> >>>> return -LIBBPF_ERRNO__RELOC;
> >>>> @@ -6374,8 +6394,22 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
> >>>> * call always has imm = -1, but for static functions
> >>>> * relocation is against STT_SECTION and insn->imm
> >>>> * points to a start of a static function
> >>>> + *
> >>>> + * for local func relocation, the imm field encodes
> >>>> + * the byte offset in the corresponding section.
> >>>> + */
> >>>> + if (relo->type == RELO_CALL)
> >>>> + sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
> >>>> + else
> >>>> + sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm / BPF_INSN_SZ + 1;
> >>>> + } else if (insn_is_pseudo_func(insn)) {
> >>>> + /*
> >>>> + * RELO_SUBPROG_ADDR relo is always emitted even if both
> >>>> + * functions are in the same section, so it shouldn't reach here.
> >>>> */
> >>>> - sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
> >>>> + pr_warn("prog '%s': missing relo for insn #%zu, type %d\n",
> >>>
> >>> nit: "missing subprog addr relo" to make it clearer?
> >>
> >> sure. will do.
> >
> > given the "generic support" comment above, I think we should still
> > support this case as well. WDYT?
>
> Even for global function with ldimm64, relocations should have been
> generated previously. I will double verify to ensure this is true
> for global function as well for different cases, in the same section or
> different sections, etc.
Thanks! Subprogram call within another subprogram definitely doesn't
generate relocation, but this might be the only exception. Sorry for
nagging about this :) If relo is always there, then this error is good
as is.
>
> >
> >>
> >>>
> >>>> + prog->name, insn_idx, relo->type);
> >>>> + return -LIBBPF_ERRNO__RELOC;
> >>>> } else {
> >>>> /* if subprogram call is to a static function within
> >>>> * the same ELF section, there won't be any relocation
> >>>> --
> >>>> 2.24.1
> >>>>
