[net-next] libbpf: use map_flags when creating maps

From: Craig Gallek <kraig@google.com>

This extends struct bpf_map_def to include a flags field.  Note that
this has the potential to break the validation logic in
bpf_object__validate_maps and bpf_object__init_maps as they use
sizeof(struct bpf_map_def) as a minimal allowable size of a map section.
Any bpf program compiled with a smaller struct bpf_map_def will fail this
check.

I don't believe this will be an issue in practice as both compile-time
definitions of struct bpf_map_def (in samples/bpf/bpf_load.h and
tools/testing/selftests/bpf/bpf_helpers.h) have always been larger
than this newly updated version in libbpf.h.

Signed-off-by: Craig Gallek <kraig@google.com>
---
 tools/lib/bpf/libbpf.c | 2 +-
 tools/lib/bpf/libbpf.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

On 9/27/17 7:04 AM, Craig Gallek wrote:
> From: Craig Gallek <kraig@google.com>
>
> This extends struct bpf_map_def to include a flags field.  Note that
> this has the potential to break the validation logic in
> bpf_object__validate_maps and bpf_object__init_maps as they use
> sizeof(struct bpf_map_def) as a minimal allowable size of a map section.
> Any bpf program compiled with a smaller struct bpf_map_def will fail this
> check.
>
> I don't believe this will be an issue in practice as both compile-time
> definitions of struct bpf_map_def (in samples/bpf/bpf_load.h and
> tools/testing/selftests/bpf/bpf_helpers.h) have always been larger
> than this newly updated version in libbpf.h.
>
> Signed-off-by: Craig Gallek <kraig@google.com>
> ---
>  tools/lib/bpf/libbpf.c | 2 +-
>  tools/lib/bpf/libbpf.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 35f6dfcdc565..6bea85f260a3 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -874,7 +874,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>  				      def->key_size,
>  				      def->value_size,
>  				      def->max_entries,
> -				      0);
> +				      def->map_flags);
>  		if (*pfd < 0) {
>  			size_t j;
>  			int err = *pfd;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 7959086eb9c9..6e20003109e0 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -207,6 +207,7 @@ struct bpf_map_def {
>  	unsigned int key_size;
>  	unsigned int value_size;
>  	unsigned int max_entries;
> +	unsigned int map_flags;
>  };

yes it will break loading of pre-compiled .o
Instead of breaking, let's fix the loader to do it the way
samples/bpf/bpf_load.c does.
See commit 156450d9d964 ("samples/bpf: make bpf_load.c code compatible 
with ELF maps section changes")

On 09/27/2017 06:29 PM, Alexei Starovoitov wrote:
> On 9/27/17 7:04 AM, Craig Gallek wrote:
>> From: Craig Gallek <kraig@google.com>
>>
>> This extends struct bpf_map_def to include a flags field.  Note that
>> this has the potential to break the validation logic in
>> bpf_object__validate_maps and bpf_object__init_maps as they use
>> sizeof(struct bpf_map_def) as a minimal allowable size of a map section.
>> Any bpf program compiled with a smaller struct bpf_map_def will fail this
>> check.
>>
>> I don't believe this will be an issue in practice as both compile-time
>> definitions of struct bpf_map_def (in samples/bpf/bpf_load.h and
>> tools/testing/selftests/bpf/bpf_helpers.h) have always been larger
>> than this newly updated version in libbpf.h.
>>
>> Signed-off-by: Craig Gallek <kraig@google.com>
>> ---
>>  tools/lib/bpf/libbpf.c | 2 +-
>>  tools/lib/bpf/libbpf.h | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 35f6dfcdc565..6bea85f260a3 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -874,7 +874,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>>                        def->key_size,
>>                        def->value_size,
>>                        def->max_entries,
>> -                      0);
>> +                      def->map_flags);
>>          if (*pfd < 0) {
>>              size_t j;
>>              int err = *pfd;
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 7959086eb9c9..6e20003109e0 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -207,6 +207,7 @@ struct bpf_map_def {
>>      unsigned int key_size;
>>      unsigned int value_size;
>>      unsigned int max_entries;
>> +    unsigned int map_flags;
>>  };
>
> yes it will break loading of pre-compiled .o
> Instead of breaking, let's fix the loader to do it the way
> samples/bpf/bpf_load.c does.
> See commit 156450d9d964 ("samples/bpf: make bpf_load.c code compatible with ELF maps section changes")

+1, iproute2 loader also does map spec fixup

For libbpf it would be good also such that it reduces the diff
further between the libbpf and bpf_load so that it allows move
to libbpf for samples in future.

On Wed, Sep 27, 2017 at 6:03 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 09/27/2017 06:29 PM, Alexei Starovoitov wrote:
>>
>> On 9/27/17 7:04 AM, Craig Gallek wrote:
>>>
>>> From: Craig Gallek <kraig@google.com>
>>>
>>> This extends struct bpf_map_def to include a flags field.  Note that
>>> this has the potential to break the validation logic in
>>> bpf_object__validate_maps and bpf_object__init_maps as they use
>>> sizeof(struct bpf_map_def) as a minimal allowable size of a map section.
>>> Any bpf program compiled with a smaller struct bpf_map_def will fail this
>>> check.
>>>
>>> I don't believe this will be an issue in practice as both compile-time
>>> definitions of struct bpf_map_def (in samples/bpf/bpf_load.h and
>>> tools/testing/selftests/bpf/bpf_helpers.h) have always been larger
>>> than this newly updated version in libbpf.h.
>>>
>>> Signed-off-by: Craig Gallek <kraig@google.com>
>>> ---
>>>  tools/lib/bpf/libbpf.c | 2 +-
>>>  tools/lib/bpf/libbpf.h | 1 +
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index 35f6dfcdc565..6bea85f260a3 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -874,7 +874,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>>>                        def->key_size,
>>>                        def->value_size,
>>>                        def->max_entries,
>>> -                      0);
>>> +                      def->map_flags);
>>>          if (*pfd < 0) {
>>>              size_t j;
>>>              int err = *pfd;
>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>> index 7959086eb9c9..6e20003109e0 100644
>>> --- a/tools/lib/bpf/libbpf.h
>>> +++ b/tools/lib/bpf/libbpf.h
>>> @@ -207,6 +207,7 @@ struct bpf_map_def {
>>>      unsigned int key_size;
>>>      unsigned int value_size;
>>>      unsigned int max_entries;
>>> +    unsigned int map_flags;
>>>  };
>>
>>
>> yes it will break loading of pre-compiled .o
>> Instead of breaking, let's fix the loader to do it the way
>> samples/bpf/bpf_load.c does.
>> See commit 156450d9d964 ("samples/bpf: make bpf_load.c code compatible
>> with ELF maps section changes")
>
>
> +1, iproute2 loader also does map spec fixup
>
> For libbpf it would be good also such that it reduces the diff
> further between the libbpf and bpf_load so that it allows move
> to libbpf for samples in future.

Fair enough, I'll try to get this to work more dynamically.  I did
noticed that the fields of struct bpf_map_def in
selftests/.../bpf_helpers.h and iproute2's struct bpf_elf_map have
diverged. The flags field is the only thing missing from libbpf right
now (and they are at the same offset for both), so it won't be an
issue for this change, but it is going to make unifying all of these
things under libbpf not trivial at some point...

On 09/28/2017 07:33 PM, Craig Gallek wrote:
> On Wed, Sep 27, 2017 at 6:03 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 09/27/2017 06:29 PM, Alexei Starovoitov wrote:
>>> On 9/27/17 7:04 AM, Craig Gallek wrote:
>>>> From: Craig Gallek <kraig@google.com>
[...]
>>>
>>> yes it will break loading of pre-compiled .o
>>> Instead of breaking, let's fix the loader to do it the way
>>> samples/bpf/bpf_load.c does.
>>> See commit 156450d9d964 ("samples/bpf: make bpf_load.c code compatible
>>> with ELF maps section changes")
>>
>> +1, iproute2 loader also does map spec fixup
>>
>> For libbpf it would be good also such that it reduces the diff
>> further between the libbpf and bpf_load so that it allows move
>> to libbpf for samples in future.
>
> Fair enough, I'll try to get this to work more dynamically.  I did
> noticed that the fields of struct bpf_map_def in
> selftests/.../bpf_helpers.h and iproute2's struct bpf_elf_map have
> diverged. The flags field is the only thing missing from libbpf right
> now (and they are at the same offset for both), so it won't be an
> issue for this change, but it is going to make unifying all of these
> things under libbpf not trivial at some point...

Yes, iproute2 uses its own loader with own specifics related to
iproute2. With the above I rather meant that we can reduce the
gap between libbpf and bpf_load from the BPF samples, so that
it would allow us to migrate samples over entirely, there was
unfortunately never a follow-up on 156450d9d964 though, but given
there is need to extend libbpf (I guess mostly due to lpm map
handling ;)), we need to do a similar thing there as well to not
break existing obj files.