netdev - Re: [PATCH net-next v2 1/2] libbpf: parse maps sections of varying size

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <59D53611.5020907@iogearbox.net>
Date:   Wed, 04 Oct 2017 21:27:13 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Craig Gallek <kraigatgoog@...il.com>
CC:     Alexei Starovoitov <ast@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Chonggang Li <chonggangli@...gle.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/2] libbpf: parse maps sections of varying
 size

On 10/04/2017 03:59 PM, Craig Gallek wrote:
> On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 10/03/2017 01:07 AM, Alexei Starovoitov wrote:
>>> On 10/2/17 9:41 AM, Craig Gallek wrote:
>>>>
>>>> +    /* Assume equally sized map definitions */
>>>> +    map_def_sz = data->d_size / nr_maps;
>>>> +    if (!data->d_size || (data->d_size % nr_maps) != 0) {
>>>> +        pr_warning("unable to determine map definition size "
>>>> +               "section %s, %d maps in %zd bytes\n",
>>>> +               obj->path, nr_maps, data->d_size);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> this approach is not as flexible as done by samples/bpf/bpf_load.c
>>> where it looks at every map independently by walking symtab,
>>> but I guess it's ok.
>>
>> Regarding different map spec structs in a single prog: unless
>> we have a good use case why we would need it (and I'm not aware
>> of anything in particular), I would just go with a fixed size.
>> I did kind of similar sanity checks in bpf_fetch_maps_end() in
>> iproute2 loader as well.
>
> Split vote?  I'm happy to try to make this work with varying sizes,
> but I agree the usefulness seems low.  It would imply building map
> sections with different definition structures and we would then need a
> way to differentiate them.  If the goal is to allow for different map
> definition structures, I don't think size alone is a sufficient
> differentiator.

They would still all need to go to maps section, but you would end
up going with different structs for map specs, where they all would
need to overlap for the members in order for this to work. E.g. you
would have maps of both structs sitting in map section of a single
prog ...

struct bpf_map_spec_v1 {
	__u32 type;
	__u32 size_key;
	__u32 size_value;
	__u32 max_elem;
};

struct bpf_map_spec_v2 {
	__u32 type;
	__u32 size_key;
	__u32 size_value;
	__u32 max_elem;
	__u32 flags;
};

... rather than just using single spec everywhere consistently, and
have the members zeroed that are unused or such, so if there's no
real use-case for different structs (cannot think of any right now),
lets not add complexity for it until it's really required.