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

Date:   Fri, 9 Oct 2020 18:05:31 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 Fri, Oct 9, 2020 at 5:10 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> 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

Ah, ok, then probably an extra flag is necessary.

> 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().