netdev - Re: [RFC bpf-next] bpf: add new jited info fields in bpf_dev_offload and bpf_prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Jan 2018 12:31:15 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        alexei.starovoitov@...il.com, kafai@...com
Cc:     oss-drivers@...ronome.com, netdev@...r.kernel.org,
        francois.theron@...ronome.com,
        Jiong Wang <jiong.wang@...ronome.com>
Subject: Re: [RFC bpf-next] bpf: add new jited info fields in bpf_dev_offload
 and bpf_prog_info

On 01/12/2018 03:17 AM, Jakub Kicinski wrote:
> On Thu, 11 Jan 2018 16:47:47 -0800, Jakub Kicinski wrote:
>> Hi!
>>
>> Jiong is working on dumping JITed NFP image via bpftool, Francois will be
>> submitting support for NFP in binutils soon (whoop! :)).
>>
>> We would appreciate if you could weigh in on the uAPI.  Is it OK to reuse
>> the existing jited_prog_len/jited_prog_insns or should we add separate
>> 2 new fields (plus the arch name) to avoid confusing old user space?
> 
> Ah, I skipped one chunk of Jiong's patch here, this would also be
> necessary if we reuse fields:
What kind of string would sit in jited_arch_name for nfp? Given you know
the {ifindex, netns_dev, netns_ino} 3‑tuple, isn't it possible to infer
the driver info from ethtool (e.g. nfp_net_get_drvinfo()) already and do
the mapping for binutils? Given we know when ifindex is 0, then its
always host JITed and the current approach with bfd_openr() works fine,
and if ifindex is non-0 it is /always/ device offloaded to the one bound
in the ifindex so JIT dump will be specific to that device only and
never host one. Not at all opposed to extending uapi, just a question
on my side to get a better understanding first wrt arch string (maybe
rough but complete patch with nfp + bpftool bits might help too).

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 2bac0dc..c7831cd 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1673,19 +1673,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>  		goto done;
>  	}
>  
> -	ulen = info.jited_prog_len;
> -	info.jited_prog_len = prog->jited_len;
> -	if (info.jited_prog_len && ulen) {
> -		if (bpf_dump_raw_ok()) {
> -			uinsns = u64_to_user_ptr(info.jited_prog_insns);
> -			ulen = min_t(u32, info.jited_prog_len, ulen);
> -			if (copy_to_user(uinsns, prog->bpf_func, ulen))
> -				return -EFAULT;
> -		} else {
> -			info.jited_prog_insns = 0;
> -		}
> -	}
> -
>  	ulen = info.xlated_prog_len;
>  	info.xlated_prog_len = bpf_prog_insn_size(prog);
>  	if (info.xlated_prog_len && ulen) {
> @@ -1711,6 +1698,21 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>  		err = bpf_prog_offload_info_fill(&info, prog);
>  		if (err)
>  			return err;
> +		else
> +			goto done;
> +	}
> +
> +	ulen = info.jited_prog_len;
> +	info.jited_prog_len = prog->jited_len;
> +	if (info.jited_prog_len && ulen) {
> +		if (bpf_dump_raw_ok()) {
> +			uinsns = u64_to_user_ptr(info.jited_prog_insns);
> +			ulen = min_t(u32, info.jited_prog_len, ulen);
> +			if (copy_to_user(uinsns, prog->bpf_func, ulen))
> +				return -EFAULT;
> +		} else {
> +			info.jited_prog_insns = 0;
> +		}
>  	}
>  
>  done:
> 
> info.jited_prog_len is an in/out parameter, so we can't write it twice
> if we share fields..  Sorry for messing up.