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]

Message-ID: <baad0eb0-0d53-6bc2-6836-1b4e1b3fdd90@netronome.com>
Date:   Mon, 15 Jan 2018 12:27:01 +0000
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     alexei.starovoitov@...il.com, kafai@...com,
        oss-drivers@...ronome.com, netdev@...r.kernel.org,
        francois.theron@...ronome.com
Subject: Re: [RFC bpf-next] bpf: add new jited info fields in bpf_dev_offload
 and bpf_prog_info

> On Fri, 12 Jan 2018 12:31:15 +0100, Daniel Borkmann wrote:
>> What kind of string would sit in jited_arch_name for nfp?

The name is purely to let libfd know which bfd backend to choose for the
disassembler.

>> 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?
> Right, the inference can certainly work.  Probably by matching the PCI
> ID of the device?  Or we can just assume it's the NFP since there is
> only one upstream BPF offload :)

Checked ethtool source code and I am thinking (and had tried) we could just
call the existing "ifindex_to_name_ns" in bpftool which returns the device
name, then we could use ioctl/SIOCETHTOOL to get the "struct ethtool_drvinfo"
associated with the device, then map the driver name to bpf name.

Will send out new patches following this way shortly, please let me know if
this is not good.

>
>> 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).
> I was advocating for full separate set of fields, Jiong was trying for
> a reuse, and we sort of met in the middle :)  Depends on how one looks
> at the definition of the jited_prog_insn field, old user space is not
> guaranteed to pay attention to ifindex.  We will drop the arch and reuse
> host fields if that's the direction you're leaning in.

I also want to make sure adding new "jited_image" and "jited_len" is the
correct approach.

Was thinking to reusing exsiting fields for host jit, i.e
bpf_prog->jited_len and bpf_prog->aux->jited_data, but different offload
targets might have different structure for "jited_data" that we need new
call back to parse it, also I feel keep host fields clean for host JIT
only might help preventing code logic for host and offload interleaving
with each other, so had gone with adding new fields.

-- 
Regards,
Jiong