netdev - Re: [PATCH bpf-next v4 3/6] bpf: allow for map-in-map with dynamic inner array map entries

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:   Sat, 10 Oct 2020 02:10:57 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        john fastabend <john.fastabend@...il.com>,
        Yonghong Song <yhs@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v4 3/6] bpf: allow for map-in-map with dynamic
 inner array map entries

On 10/10/20 1:01 AM, Andrii Nakryiko wrote:
> On Fri, Oct 9, 2020 at 3:40 PM Daniel Borkmann <daniel@...earbox.net> wrote:
[...]
>>          *insn++ = BPF_ALU64_IMM(BPF_ADD, map_ptr, offsetof(struct bpf_array, value));
>>          *insn++ = BPF_LDX_MEM(BPF_W, ret, index, 0);
>>          if (!map->bypass_spec_v1) {
>> @@ -496,8 +499,10 @@ static int array_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
>>   static bool array_map_meta_equal(const struct bpf_map *meta0,
>>                                   const struct bpf_map *meta1)
>>   {
>> -       return meta0->max_entries == meta1->max_entries &&
>> -               bpf_map_meta_equal(meta0, meta1);
>> +       if (!bpf_map_meta_equal(meta0, meta1))
>> +               return false;
>> +       return meta0->map_flags & BPF_F_INNER_MAP ? true :
>> +              meta0->max_entries == meta1->max_entries;
> 
> even if meta1 doesn't have BPF_F_INNER_MAP, it's ok, because all the
> accesses for map returned from outer map lookup will not inline, is
> that right? So this flag only matters for the inner map's prototype.

Not right now, we would have to open code bpf_map_meta_equal() to cut out that
bit from the meta0/1 flags comparison. I wouldn't change bpf_map_meta_equal()
itself given that bit can be reused for different purpose for other map types.

> You also mentioned that not inlining array access should still be
> fast. So I wonder, what if we just force non-inlined access for inner
> maps of ARRAY type? Would it be too bad of a hit for existing
> applications?

Fast in the sense of that we can avoid a retpoline given the direct call
to array_map_lookup_elem() as opposed to bpf_map_lookup_elem(). In the
array_map_gen_lookup() we even have insn level optimizations such as
replacing BPF_MUL with BPF_LSH with immediate elem size on power of 2
#elems as well as avoiding spectre masking (which the call one has not),
presumably for cases like XDP we might want the best implementation if
usage allows it.

> The benefit would be that everything would just work without a special
> flag. If perf hit isn't prohibitive, it might be worthwhile to
> simplify user experience?

Taking the above penalty aside for same sized-elems, simplest one would have
been to just set inner_map_meta->ops to &array_map_no_inline_ops inside the
bpf_map_meta_alloc().