Hi Jakub,

On 05/18/2018 12:21 AM, Jakub Kicinski wrote:
> On Thu, 17 May 2018 12:05:47 +0530, Sandipan Das wrote:
>> Currently, we resolve the callee's address for a JITed function
>> call by using the imm field of the call instruction as an offset
>> from __bpf_call_base. If bpf_jit_kallsyms is enabled, we further
>> use this address to get the callee's kernel symbol's name.
>>
>> For some architectures, such as powerpc64, the imm field is not
>> large enough to hold this offset. So, instead of assigning this
>> offset to the imm field, the verifier now assigns the subprog
>> id. Also, a list of kernel symbol addresses for all the JITed
>> functions is provided in the program info. We now use the imm
>> field as an index for this list to lookup a callee's symbol's
>> address and resolve its name.
>>
>> Suggested-by: Daniel Borkmann <dan...@iogearbox.net>
>> Signed-off-by: Sandipan Das <sandi...@linux.vnet.ibm.com>
> 
> A few nit-picks below, thank you for the patch!
> 
>>  tools/bpf/bpftool/prog.c          | 31 +++++++++++++++++++++++++++++++
>>  tools/bpf/bpftool/xlated_dumper.c | 24 +++++++++++++++++-------
>>  tools/bpf/bpftool/xlated_dumper.h |  2 ++
>>  3 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> index 9bdfdf2d3fbe..ac2f62a97e84 100644
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
>> @@ -430,6 +430,10 @@ static int do_dump(int argc, char **argv)
>>      unsigned char *buf;
>>      __u32 *member_len;
>>      __u64 *member_ptr;
>> +    unsigned int nr_addrs;
>> +    unsigned long *addrs = NULL;
>> +    __u32 *ksyms_len;
>> +    __u64 *ksyms_ptr;
> 
> nit: please try to keep the variables ordered longest to shortest like
> we do in networking code (please do it in all functions).
> 
>>      ssize_t n;
>>      int err;
>>      int fd;
>> @@ -437,6 +441,8 @@ static int do_dump(int argc, char **argv)
>>      if (is_prefix(*argv, "jited")) {
>>              member_len = &info.jited_prog_len;
>>              member_ptr = &info.jited_prog_insns;
>> +            ksyms_len = &info.nr_jited_ksyms;
>> +            ksyms_ptr = &info.jited_ksyms;
>>      } else if (is_prefix(*argv, "xlated")) {
>>              member_len = &info.xlated_prog_len;
>>              member_ptr = &info.xlated_prog_insns;
>> @@ -496,10 +502,23 @@ static int do_dump(int argc, char **argv)
>>              return -1;
>>      }
>>  
>> +    nr_addrs = *ksyms_len;
> 
> Here and ...
> 
>> +    if (nr_addrs) {
>> +            addrs = malloc(nr_addrs * sizeof(__u64));
>> +            if (!addrs) {
>> +                    p_err("mem alloc failed");
>> +                    free(buf);
>> +                    close(fd);
>> +                    return -1;
> 
> You can just jump to err_free here.
> 
>> +            }
>> +    }
>> +
>>      memset(&info, 0, sizeof(info));
>>  
>>      *member_ptr = ptr_to_u64(buf);
>>      *member_len = buf_size;
>> +    *ksyms_ptr = ptr_to_u64(addrs);
>> +    *ksyms_len = nr_addrs;
> 
> ... here - this function is getting long, so maybe I'm not seeing
> something, but are ksyms_ptr and ksyms_len guaranteed to be initialized?
> 
>>      err = bpf_obj_get_info_by_fd(fd, &info, &len);
>>      close(fd);
>> @@ -513,6 +532,11 @@ static int do_dump(int argc, char **argv)
>>              goto err_free;
>>      }
>>  
>> +    if (*ksyms_len > nr_addrs) {
>> +            p_err("too many addresses returned");
>> +            goto err_free;
>> +    }
>> +
>>      if ((member_len == &info.jited_prog_len &&
>>           info.jited_prog_insns == 0) ||
>>          (member_len == &info.xlated_prog_len &&
>> @@ -558,6 +582,9 @@ static int do_dump(int argc, char **argv)
>>                      dump_xlated_cfg(buf, *member_len);
>>      } else {
>>              kernel_syms_load(&dd);
>> +            dd.jited_ksyms = ksyms_ptr;
>> +            dd.nr_jited_ksyms = *ksyms_len;
>> +
>>              if (json_output)
>>                      dump_xlated_json(&dd, buf, *member_len, opcodes);
>>              else
>> @@ -566,10 +593,14 @@ static int do_dump(int argc, char **argv)
>>      }
>>  
>>      free(buf);
>> +    if (addrs)
>> +            free(addrs);
> 
> Free can deal with NULL pointers, no need for an if.
> 
>>      return 0;
>>  
>>  err_free:
>>      free(buf);
>> +    if (addrs)
>> +            free(addrs);
>>      return -1;
>>  }
>>  
>> diff --git a/tools/bpf/bpftool/xlated_dumper.c 
>> b/tools/bpf/bpftool/xlated_dumper.c
>> index 7a3173b76c16..dc8e4eca0387 100644
>> --- a/tools/bpf/bpftool/xlated_dumper.c
>> +++ b/tools/bpf/bpftool/xlated_dumper.c
>> @@ -178,8 +178,12 @@ static const char *print_call_pcrel(struct dump_data 
>> *dd,
>>              snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
>>                       "%+d#%s", insn->off, sym->name);
>>      else
> 
> else if (address)
> 
> saves us the indentation.
> 
>> -            snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
>> -                     "%+d#0x%lx", insn->off, address);
>> +            if (address)
>> +                    snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
>> +                             "%+d#0x%lx", insn->off, address);
>> +            else
>> +                    snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
>> +                             "%+d", insn->off);
>>      return dd->scratch_buff;
>>  }
>>  
>> @@ -200,14 +204,20 @@ static const char *print_call(void *private_data,
>>                            const struct bpf_insn *insn)
>>  {
>>      struct dump_data *dd = private_data;
>> -    unsigned long address = dd->address_call_base + insn->imm;
>> -    struct kernel_sym *sym;
>> +    unsigned long address = 0;
>> +    struct kernel_sym *sym = NULL;
>>  
> 
> Hm.  Quite a bit of churn.  Why not just add these three lines here:
> 
> if (insn->src_reg == BPF_PSEUDO_CALL && 
>     insn->imm < dd->nr_jited_ksyms)
>       address = dd->jited_ksyms[insn->imm];
> 
>> -    sym = kernel_syms_search(dd, address);
>> -    if (insn->src_reg == BPF_PSEUDO_CALL)
>> +    if (insn->src_reg == BPF_PSEUDO_CALL) {
>> +            if (dd->nr_jited_ksyms) {
>> +                    address = dd->jited_ksyms[insn->imm];
> 
> Perhaps it's paranoid, but it'd please do to bound check insn->imm
> against dd->nr_jited_ksyms.
> 
>> +                    sym = kernel_syms_search(dd, address);
>> +            }
>>              return print_call_pcrel(dd, sym, address, insn);
>> -    else
>> +    } else {
>> +            address = dd->address_call_base + insn->imm;
>> +            sym = kernel_syms_search(dd, address);
>>              return print_call_helper(dd, sym, address);
>> +    }
>>  }
>>  
>>  static const char *print_imm(void *private_data,
> 
> 

Thank you for the suggestions. Will post out v2 with these changes.

- Sandipan

Reply via email to