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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>,
	john fastabend <john.fastabend@gmail.com>,
	Yonghong Song <yhs@fb.com>, Networking <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 3/6] bpf: allow for map-in-map with dynamic inner array map entries
Date: Fri, 9 Oct 2020 11:42:40 -0700	[thread overview]
Message-ID: <CAEf4Bzb+p8Kum8aX-t7Zzm9VQADZZf=JU0CcqUUTT7Uut_uxzA@mail.gmail.com> (raw)
In-Reply-To: <99c67c05-700e-8f54-7fea-2daa6d19ec9e@iogearbox.net>

On Fri, Oct 9, 2020 at 11:35 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/9/20 7:42 PM, Andrii Nakryiko wrote:
> > On Fri, Oct 9, 2020 at 7:13 AM Daniel Borkmann <daniel@iogearbox.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