netdev - Re: [PATCH bpf-next v4 01/16] bpf: implement lookup-free direct value access for maps

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: <2389ce8e-55d4-226d-55f3-29190199bb8d@iogearbox.net>
Date:   Sat, 6 Apr 2019 12:58:23 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
        joe@...d.net.nz, yhs@...com, andrii.nakryiko@...il.com,
        kafai@...com
Subject: Re: [PATCH bpf-next v4 01/16] bpf: implement lookup-free direct value
 access for maps

On 04/06/2019 03:56 AM, Alexei Starovoitov wrote:
> On Fri, Apr 05, 2019 at 10:59:27PM +0200, Daniel Borkmann wrote:
>>  
>> -/* when bpf_ldimm64->src_reg == BPF_PSEUDO_MAP_FD, bpf_ldimm64->imm == fd */
>> +/* When BPF ldimm64's insn[0].src_reg != 0 then this can have
>> + * two extensions:
>> + *
>> + * insn[0].src_reg:  BPF_PSEUDO_MAP_FD   BPF_PSEUDO_MAP_VALUE
>> + * insn[0].imm:      map fd              map fd
>> + * insn[1].imm:      0                   offset into value
>> + * insn[0].off:      0                   lower 16 bit of map index
>> + * insn[1].off:      0                   higher 16 bit of map index
>> + * ldimm64 rewrite:  address of map      address of map[index]+offset
>> + * verifier type:    CONST_PTR_TO_MAP    PTR_TO_MAP_VALUE
> ...
>> +	else if (insn->src_reg == BPF_PSEUDO_MAP_VALUE)
>> +		snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
>> +			 "map[id:%u][%u]+%u", insn->imm,
>> +			 ((__u32)(__u16)insn[0].off) |
>> +			 ((__u32)(__u16)insn[1].off) << 16,
>> +			 (insn + 1)->imm);
> 
> Hopefully one last nit...
> Do we really need to allow this odd split index support?
> Later patches enforce array of 1 element and libbpf only uses that.
> This index feature feels too quirky and not really useful at this moment.
> Can we enforce that insn[0|1].off == 0 instead ?
> Later we can extend it to mean index without breaking anything.

I originally didn't have it in v2 of the series, but I ended up
implementing it after feedback from Andrii back then complaining
that it's too specific and not generic enough. I agreed with him
that the limitation of max_elems = 1 wasn't too nice, so I went
to implement that full 32 bit index can be used thus that it has
the potential of efficient map lookup replacement for array maps in
general which is quite nice since within single insn it allows to
select index and offset into value all as simple 64 bit imm load.

Thanks,
Daniel