[PATCH bpf 5/6] tools: bpftool: resolve calls without using imm field

Sandipan Das sandipan at linux.vnet.ibm.com
Fri May 18 14:28:02 AEST 2018


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 <daniel at iogearbox.net>
>> Signed-off-by: Sandipan Das <sandipan at 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



More information about the Linuxppc-dev mailing list