Re: [PATCH bpf-next 4/6] bpf: Support access to bpf map fields

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Jun 18, 2020 at 05:27:43PM -0700, Andrey Ignatov wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2020-06-18 17:08 -0700]:
> > On Thu, Jun 18, 2020 at 4:52 PM Andrey Ignatov <rdna@fb.com> wrote:
> > >
> > > Andrey Ignatov <rdna@fb.com> [Thu, 2020-06-18 12:42 -0700]:
> > > > Martin KaFai Lau <kafai@fb.com> [Wed, 2020-06-17 23:18 -0700]:
> > > > > On Wed, Jun 17, 2020 at 01:43:45PM -0700, Andrey Ignatov wrote:
> > > > > [ ... ]
> > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > > index e5c5305e859c..fa21b1e766ae 100644
> > > > > > --- a/kernel/bpf/btf.c
> > > > > > +++ b/kernel/bpf/btf.c
> > > > > > @@ -3577,6 +3577,67 @@ btf_get_prog_ctx_type(struct bpf_verifier_log *log, struct btf *btf,
> > > > > >   return ctx_type;
> > > > > >  }
> > > > > >
> > > > > > +#define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type)
> > > > > > +#define BPF_LINK_TYPE(_id, _name)
> > > > > > +static const struct bpf_map_ops * const btf_vmlinux_map_ops[] = {
> > > > > > +#define BPF_MAP_TYPE(_id, _ops) \
> > > > > > + [_id] = &_ops,
> > > > > > +#include <linux/bpf_types.h>
> > > > > > +#undef BPF_MAP_TYPE
> > > > > > +};
> > > > > > +static u32 btf_vmlinux_map_ids[] = {
> > > > > > +#define BPF_MAP_TYPE(_id, _ops) \
> > > > > > + [_id] = (u32)-1,
> > > > > > +#include <linux/bpf_types.h>
> > > > > > +#undef BPF_MAP_TYPE
> > > > > > +};
> > > > > > +#undef BPF_PROG_TYPE
> > > > > > +#undef BPF_LINK_TYPE
> > > > > > +
> > > > > > +static int btf_vmlinux_map_ids_init(const struct btf *btf,
> > > > > > +                             struct bpf_verifier_log *log)
> > > > > > +{
> > > > > > + int base_btf_id, btf_id, i;
> > > > > > + const char *btf_name;
> > > > > > +
> > > > > > + base_btf_id = btf_find_by_name_kind(btf, "bpf_map", BTF_KIND_STRUCT);
> > > > > > + if (base_btf_id < 0)
> > > > > > +         return base_btf_id;
> > > > > > +
> > > > > > + BUILD_BUG_ON(ARRAY_SIZE(btf_vmlinux_map_ops) !=
> > > > > > +              ARRAY_SIZE(btf_vmlinux_map_ids));
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(btf_vmlinux_map_ops); ++i) {
> > > > > > +         if (!btf_vmlinux_map_ops[i])
> > > > > > +                 continue;
> > > > > > +         btf_name = btf_vmlinux_map_ops[i]->map_btf_name;
> > > > > > +         if (!btf_name) {
> > > > > > +                 btf_vmlinux_map_ids[i] = base_btf_id;
> > > > > > +                 continue;
> > > > > > +         }
> > > > > > +         btf_id = btf_find_by_name_kind(btf, btf_name, BTF_KIND_STRUCT);
> > > > > > +         if (btf_id < 0)
> > > > > > +                 return btf_id;
> > > > > > +         btf_vmlinux_map_ids[i] = btf_id;
> > > > > Since map_btf_name is already in map_ops, how about storing map_btf_id in
> > > > > map_ops also?
> > > > > btf_id 0 is "void" which is as good as -1, so there is no need
> > > > > to modify all map_ops to init map_btf_id to -1.
> > > >
> > > > Yeah, btf_id == 0 being a valid id was the reason I used -1.
> > > >
> > > > I realized that having a map type specific struct with btf_id == 0
> > > > should be practically impossible, but is it guaranteed to always be
> > > > "void" or it just happened so and can change in the future?
> > > >
> > > > If both this and having one more field in bpf_map_ops is not a problem,
> > > > I'll move it to bpf_map_ops.
> > >
> > > Nope, I can't do it. All `struct bpf_map_ops` are global `const`, i.e.
> > > rodata and a try cast `const` away and change them causes a panic.
> > >
> > > Simple user space repro:
> > >
> > >         % cat 1.c
> > >         #include <stdio.h>
> > >
> > >         struct map_ops {
> > >                 int a;
> > >         };
> > >
> > >         const struct map_ops ops = {
> > >                 .a = 1,
> > >         };
> > >
> > >         int main(void)
> > >         {
> > >                 struct map_ops *ops_rw = (struct map_ops *)&ops;
> > >
> > >                 printf("before a=%d\n", ops_rw->a);
> > >                 ops_rw->a = 3;
> > >                 printf(" afrer a=%d\n", ops_rw->a);
> > >         }
> > >         % clang -O2 -Wall -Wextra -pedantic -pedantic-errors -g 1.c && ./a.out
> > >         before a=1
> > >         Segmentation fault (core dumped)
> > >         % objdump -t a.out  | grep -w ops
> > >         0000000000400600 g     O .rodata        0000000000000004              ops
> > >
> > > --
> > > Andrey Ignatov
> > 
> > See the trick that helper prototypes do for BTF ids. Fictional example:
> > 
> > static int hash_map_btf_id;
> > 
> > const struct bpf_map_ops hash_map_opss = {
> >  ...
> >  .btf_id = &hash_map_btf_id,
> > };
> > 
> > 
> > then *hash_map_ops.btf_id = <final_btf_id>;
> 
> Yeah, it would work, but IMO it wouldn't make the implementation better
> since for every map type I would need to write two additional lines of
> code. And whoever adds new map type will need to repeat this trick.
I think bpf_func_proto has already been doing this.  Yonghong's
tcp/udp iter is also doing something similar.

> 
> Yeah, it can be automated with a macro, but IMO it's better to avoid
> the work than to automate it.
> 
> Martin, Andrii is there any strong reason to convert to map_btf_id
> field? Or it's "nice to have" kind of thing? If the latter, I'd prefer
> to stick with my approach.
I just think it will be more natural to be able to directly
access it through a map's related pointer, considering
the map_btf_name is already there.

I don't feel strongly here for now.  I think things will have to change
anyway after quickly peeking at Jiri's patch.  Also,
I take back on the collision check.  I think this check
should belong to Jiri's patch instead (if it is indeed needed).
Renaming the sock_hash is good enough for now.