netdev - Re: [PATCH bpf-next v2 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 11:42:40 -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 v2 3/6] bpf: allow for map-in-map with dynamic
 inner array map entries

On Fri, Oct 9, 2020 at 11:35 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 10/9/20 7:42 PM, Andrii Nakryiko wrote:
> > On Fri, Oct 9, 2020 at 7:13 AM Daniel Borkmann <daniel@...earbox.net> wrote:
> [...]
> >>   static int percpu_array_map_btf_id;
> >>   const struct bpf_map_ops percpu_array_map_ops = {
> >>          .map_meta_equal = bpf_map_meta_equal,
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index 1110ecd7d1f3..519bf867f065 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -111,7 +111,8 @@ static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
> >>          ops = bpf_map_types[type];
> >>          if (!ops)
> >>                  return ERR_PTR(-EINVAL);
> >> -
> >> +       if (ops->map_swap_ops)
> >> +               ops = ops->map_swap_ops(attr);
> >
> > I'm afraid that this can cause quite a lot of confusion down the road.
> >
> > Wouldn't designating -EOPNOTSUPP return code from map_gen_lookup() and
> > not inlining in that case as if map_gen_lookup() wasn't even defined
> > be a much smaller and more local (semantically) change that achieves
> > exactly the same thing? Doesn't seem like switching from u32 to int
> > for return value would be a big inconvenience for existing
> > implementations of inlining callbacks, right?
>
> I was originally thinking about it, but then decided not to take this path,
> for example the ops->map_gen_lookup() patching code has sanity checks for
> the u32 return code on whether we patched 0 or too many instructions, so

Right, we won't ever need to patch >2 billion instructions, so making
the return value int shouldn't be a problem. As for not catching
accidental patched insn == -EOPNOTSUPP, I don't think that's a real
concern, is it? All the other negative value would trigger loud error.

> if there is anything funky going on in one of the map_gen_lookup() that
> we'd get a negative code, for example, I don't want to just skip and not
> have the verifier bark loudly with "bpf verifier is misconfigured", also
> didn't want to make the logic inside fixup_bpf_calls() even more complex,
> so the patch here felt simpler & more straight forward to me.

It's not straightforward in the same way as class inheritance and
overriding methods is not straightforward to follow in general.
Swapping out entire sets of operations is super confusing, IMO.

>
> Thanks,
> Daniel