netdev - Re: [PATCH v2 bpf-next 1/3] bpf: Consolidate inner-map-compatible properties into bpf

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]

Message-ID: <CAEf4BzbpwfFPUruwdbcarTeT_7pcxNw5PPO0RE81QLvJxkXOBw@mail.gmail.com>
Date:   Fri, 29 May 2020 13:40:46 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Daniel Borkmann <daniel@...earbox.net>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>, Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: Consolidate inner-map-compatible
 properties into bpf_types.h

On Thu, May 28, 2020 at 11:31 PM Martin KaFai Lau <kafai@...com> wrote:
>
> On Tue, May 26, 2020 at 10:54:26AM -0700, Andrii Nakryiko wrote:
> > On Fri, May 22, 2020 at 6:01 PM Martin KaFai Lau <kafai@...com> wrote:
> > >
> > > On Sat, May 23, 2020 at 12:22:48AM +0200, Daniel Borkmann wrote:
> > > > On 5/22/20 4:23 AM, Martin KaFai Lau wrote:
> > > > [...]
> > > > >   };

[...]


> > >
> > > >
> > > > Like explicit annotating via struct bpf_map_ops where everything is visible in one
> > > > single place where the map is defined:
> > > >
> > > > const struct bpf_map_ops array_map_ops = {
> > > >         .map_alloc_check = array_map_alloc_check,
> > > >         [...]
> > > >         .map_flags_fixed = BPF_MAP_IN_MAP_OK,
> > > > };
> > > I am not sure about adding it to bpf_map_ops instead of bpf_types.h.
> > > It will be easier to figure out what map types do not support MAP_IN_MAP (and
> > > other future map's fixed properties) in one place "bpf_types.h" instead of
> > > having to dig into each map src file.
> >
> > I'm 100% with Daniel here. If we are consolidating such things, I'd
> > rather have them in one place where differences between maps are
> > defined, which is ops. Despite an "ops" name, this seems like a
> > perfect place for specifying all those per-map-type properties and
> > behaviors. Adding flags into bpf_types.h just splits everything into
> > two places: bpf_types.h specifies some differences, while ops specify
> > all the other ones.
> >
> > Figuring out map-in-map support is just one of many questions one
> > might ask about differences between map types, I don't think that
> > justifies adding them to bpf_types.h. Grepping for struct bpf_map_ops
> > with search context (i.e., -A15 or something like that) should be
> > enough to get a quick glance at all possible maps and what they
> > define/override.
> >
> > It also feels like adding this as bool field for each aspect instead
> > of a collection of bits is cleaner and a bit more scalable. If we need
> > to add another property with some parameter/constant, or just enum,
> > defining one of few possible behaviors, it would be easier to just add
> > another field, instead of trying to cram that into u32. It also solves
> > your problem of "at the glance" view of map-in-map support features.
> > Just name that field unique enough to grep by it :)
> How about another way.  What patch 2 want is each map could have its own
> bpf_map_meta_equal().  Instead of adding 2 flags, add the bpf_map_meta_equal()
> as a ops to bpf_map_ops.  Each map supports to be used as an inner_map
> needs to set this ops.  Then it will be an opt-in.
> A default implementation can be provided for most maps' use.
> The maps (e.g. arraymap and other future maps) that has different requirement
> can implement its own.  For the existing maps, when we address those
> limitations (e.g. arraymap's gen_lookup) later,  we can then change its
> bpf_map_meta_equal.
>
> Thoughts?
>

I think that would work as well, I don't mind.

> >
> > >
> > > If the objective is to have the future map "consciously" opt-in, how about
> > > keeping the "BPF_MAP_TYPE" name as is but add a fixed_flags param as the
> > > earlier v1 and flip it from NO to OK flag.  It will be clear that,
> > > it is a decision that the new map needs to make instead of a quiet 0
> > > in "struct bpf_map_ops".
> > >
> > > For example,
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops, BPF_MAP_IN_MAP_OK)
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_PROG_ARRAY, prog_array_map_ops, 0)
> > > BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops, BPF_MAP_IN_MAP_OK | BPF_MAP_IN_MAP_DYNAMIC_SIZE_OK)
> > >
> > > >
> > > > That way, if someone forgets to add .map_flags_fixed to a new map type, it's okay since
> > > > it's _safe_ to forget to add these flags (and okay to add in future uapi-wise) as opposed
> > > > to the other way round where one can easily miss the opt-out case and potentially crash
> > > > the machine worst case.