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] [day] [month] [year] [list]

Message-ID: <20180115171327.5doba2tlnj23q65g@ast-mbp>
Date:   Mon, 15 Jan 2018 09:13:29 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jiong Wang <jiong.wang@...ronome.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.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 Mon, Jan 15, 2018 at 03:07:25PM +0100, Daniel Borkmann wrote:
> On 01/15/2018 01:27 PM, Jiong Wang wrote:
> >> On Fri, 12 Jan 2018 12:31:15 +0100, Daniel Borkmann wrote:
> [...]
> >>> 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.
> 
> I think it's fine.
> 
> > 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.
> 
> Agree, one thing less to worry since this also ties into kallsyms, etc.

why would bytes in jited_prog_insns conflict with kallsyms ?

I also agree that adding two extra fields for 'offloaded_prog + len'
is probably cleanest.

> Ok, lets stick with the current RFC, they seem to be the cleanest approach
> overall with having offload_arch_name[] in the uapi and only filled by
> driver JITs (and not host JITs).

I don't like string based interfaces especially when strings are used
by tools to compare with other strings.
imo strings are only good for humans to read.
I think returning ifindex is already enough. From it the tool
can get pci id which is the most accurate identification of the hw.