[PATCH net-next v2 0/2] libbpf: support more map options

[PATCH net-next v2 0/2] libbpf: support more map options

[Craig Gallek]

From: Craig Gallek <kraig@google.com>

The functional change to this series is the ability to use flags when
creating maps from object files loaded by libbpf.  In order to do this,
the first patch updates the library to handle map definitions that
differ in size from libbpf's struct bpf_map_def.

For object files with a larger map definition, libbpf will continue to load
if the unknown fields are all zero, otherwise the map is rejected.  If the
map definition in the object file is smaller than expected, libbpf will use
zero as a default value in the missing fields.

Craig Gallek (2):
  libbpf: parse maps sections of varying size
  libbpf: use map_flags when creating maps

 tools/lib/bpf/libbpf.c | 56 ++++++++++++++++++++++++++++++++++++++++++--------
 tools/lib/bpf/libbpf.h |  1 +
 2 files changed, 48 insertions(+), 9 deletions(-)

-- 
2.14.2.822.g60be5d43e6-goog

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

[Craig Gallek]

From: Craig Gallek <kraig@google.com>

This library previously assumed a fixed-size map options structure.
Any new options were ignored.  In order to allow the options structure
to grow and to support parsing older programs, this patch updates
the maps section parsing to handle varying sizes.

Object files with maps sections smaller than expected will have the new
fields initialized to zero.  Object files which have larger than expected
maps sections will be rejected unless all of the unrecognized data is zero.

This change still assumes that each map definition in the maps section
is the same size.

Signed-off-by: Craig Gallek <kraig@google.com>
---
 tools/lib/bpf/libbpf.c | 54 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4f402dcdf372..28b300868ad7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
 }
 
 static int
-bpf_object__validate_maps(struct bpf_object *obj)
+bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz)
 {
 	int i;
 
@@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj)
 		const struct bpf_map *a = &obj->maps[i - 1];
 		const struct bpf_map *b = &obj->maps[i];
 
-		if (b->offset - a->offset < sizeof(struct bpf_map_def)) {
-			pr_warning("corrupted map section in %s: map \"%s\" too small\n",
-				   obj->path, a->name);
+		if (b->offset - a->offset < map_def_sz) {
+			pr_warning("corrupted map section in %s: map \"%s\" too small "
+				   "(%zd vs %d)\n",
+				   obj->path, a->name, b->offset - a->offset,
+				   map_def_sz);
 			return -EINVAL;
 		}
 	}
@@ -615,7 +617,7 @@ static int compare_bpf_map(const void *_a, const void *_b)
 static int
 bpf_object__init_maps(struct bpf_object *obj)
 {
-	int i, map_idx, nr_maps = 0;
+	int i, map_idx, map_def_sz, nr_maps = 0;
 	Elf_Scn *scn;
 	Elf_Data *data;
 	Elf_Data *symbols = obj->efile.symbols;
@@ -658,6 +660,15 @@ bpf_object__init_maps(struct bpf_object *obj)
 	if (!nr_maps)
 		return 0;
 
+	/* 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;
+	}
+
 	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
 	if (!obj->maps) {
 		pr_warning("alloc maps for object failed\n");
@@ -690,7 +701,7 @@ bpf_object__init_maps(struct bpf_object *obj)
 				      obj->efile.strtabidx,
 				      sym.st_name);
 		obj->maps[map_idx].offset = sym.st_value;
-		if (sym.st_value + sizeof(struct bpf_map_def) > data->d_size) {
+		if (sym.st_value + map_def_sz > data->d_size) {
 			pr_warning("corrupted maps section in %s: last map \"%s\" too small\n",
 				   obj->path, map_name);
 			return -EINVAL;
@@ -704,12 +715,39 @@ bpf_object__init_maps(struct bpf_object *obj)
 		pr_debug("map %d is \"%s\"\n", map_idx,
 			 obj->maps[map_idx].name);
 		def = (struct bpf_map_def *)(data->d_buf + sym.st_value);
-		obj->maps[map_idx].def = *def;
+		/*
+		 * If the definition of the map in the object file fits in
+		 * bpf_map_def, copy it.  Any extra fields in our version
+		 * of bpf_map_def will default to zero as a result of the
+		 * calloc above.
+		 */
+		if (map_def_sz <= sizeof(struct bpf_map_def)) {
+			memcpy(&obj->maps[map_idx].def, def, map_def_sz);
+		} else {
+			/*
+			 * Here the map structure being read is bigger than what
+			 * we expect, truncate if the excess bits are all zero.
+			 * If they are not zero, reject this map as
+			 * incompatible.
+			 */
+			char *b;
+			for (b = ((char *)def) + sizeof(struct bpf_map_def);
+			     b < ((char *)def) + map_def_sz; b++) {
+				if (*b != 0) {
+					pr_warning("maps section in %s: \"%s\" "
+						   "has unrecognized, non-zero "
+						   "options\n",
+						   obj->path, map_name);
+					return -EINVAL;
+				}
+			}
+			obj->maps[map_idx].def = *def;
+		}
 		map_idx++;
 	}
 
 	qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]), compare_bpf_map);
-	return bpf_object__validate_maps(obj);
+	return bpf_object__validate_maps(obj, map_def_sz);
 }
 
 static int bpf_object__elf_collect(struct bpf_object *obj)
-- 
2.14.2.822.g60be5d43e6-goog

[PATCH net-next v2 2/2] libbpf: use map_flags when creating maps

[Craig Gallek]

From: Craig Gallek <kraig@google.com>

This is required to use BPF_MAP_TYPE_LPM_TRIE or any other map type
which requires flags.

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 28b300868ad7..5996e7565cc8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -968,7 +968,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;
 };
 
 /*
-- 
2.14.2.822.g60be5d43e6-goog

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

[Alexei Starovoitov]

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.
I'd like to hear what Daniel and Jesper say,
since we really want to move to libbpf.a in samples/bpf/
and loader has to get to parity with the one in samples.

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

[Jesper Dangaard Brouer]

First of all, thank you Craig for working on this.  As Alexei says, we
need to improve tools/lib/bpf/libbpf and move towards converting users
of bpf_load.c to this lib instead.

Comments inlined below.

On Mon,  2 Oct 2017 12:41:28 -0400 Craig Gallek <kraigatgoog@gmail.com> wrote:

> From: Craig Gallek <kraig@google.com>
> 
> This library previously assumed a fixed-size map options structure.
> Any new options were ignored.  In order to allow the options structure
> to grow and to support parsing older programs, this patch updates
> the maps section parsing to handle varying sizes.
> 
> Object files with maps sections smaller than expected will have the new
> fields initialized to zero.  Object files which have larger than expected
> maps sections will be rejected unless all of the unrecognized data is zero.
> 
> This change still assumes that each map definition in the maps section
> is the same size.
> 
> Signed-off-by: Craig Gallek <kraig@google.com>
> ---
>  tools/lib/bpf/libbpf.c | 54 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 4f402dcdf372..28b300868ad7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
>  }
>  
>  static int
> -bpf_object__validate_maps(struct bpf_object *obj)
> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz)
>  {
>  	int i;
>  
> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj)
>  		const struct bpf_map *a = &obj->maps[i - 1];
>  		const struct bpf_map *b = &obj->maps[i];
>  
> -		if (b->offset - a->offset < sizeof(struct bpf_map_def)) {
> -			pr_warning("corrupted map section in %s: map \"%s\" too small\n",
> -				   obj->path, a->name);
> +		if (b->offset - a->offset < map_def_sz) {
> +			pr_warning("corrupted map section in %s: map \"%s\" too small "
> +				   "(%zd vs %d)\n",
> +				   obj->path, a->name, b->offset - a->offset,
> +				   map_def_sz);
>  			return -EINVAL;
>  		}
>  	}
> @@ -615,7 +617,7 @@ static int compare_bpf_map(const void *_a, const void *_b)
>  static int
>  bpf_object__init_maps(struct bpf_object *obj)
>  {
> -	int i, map_idx, nr_maps = 0;
> +	int i, map_idx, map_def_sz, nr_maps = 0;
>  	Elf_Scn *scn;
>  	Elf_Data *data;
>  	Elf_Data *symbols = obj->efile.symbols;
> @@ -658,6 +660,15 @@ bpf_object__init_maps(struct bpf_object *obj)
>  	if (!nr_maps)
>  		return 0;
>  
> +	/* 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;
> +	}
> +
>  	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
>  	if (!obj->maps) {
>  		pr_warning("alloc maps for object failed\n");
> @@ -690,7 +701,7 @@ bpf_object__init_maps(struct bpf_object *obj)
>  				      obj->efile.strtabidx,
>  				      sym.st_name);
>  		obj->maps[map_idx].offset = sym.st_value;
> -		if (sym.st_value + sizeof(struct bpf_map_def) > data->d_size) {
> +		if (sym.st_value + map_def_sz > data->d_size) {
>  			pr_warning("corrupted maps section in %s: last map \"%s\" too small\n",
>  				   obj->path, map_name);
>  			return -EINVAL;
> @@ -704,12 +715,39 @@ bpf_object__init_maps(struct bpf_object *obj)
>  		pr_debug("map %d is \"%s\"\n", map_idx,
>  			 obj->maps[map_idx].name);
>  		def = (struct bpf_map_def *)(data->d_buf + sym.st_value);
> -		obj->maps[map_idx].def = *def;
> +		/*
> +		 * If the definition of the map in the object file fits in
> +		 * bpf_map_def, copy it.  Any extra fields in our version
> +		 * of bpf_map_def will default to zero as a result of the
> +		 * calloc above.
> +		 */
> +		if (map_def_sz <= sizeof(struct bpf_map_def)) {
> +			memcpy(&obj->maps[map_idx].def, def, map_def_sz);
> +		} else {
> +			/*
> +			 * Here the map structure being read is bigger than what
> +			 * we expect, truncate if the excess bits are all zero.
> +			 * If they are not zero, reject this map as
> +			 * incompatible.
> +			 */
> +			char *b;
> +			for (b = ((char *)def) + sizeof(struct bpf_map_def);
> +			     b < ((char *)def) + map_def_sz; b++) {
> +				if (*b != 0) {
> +					pr_warning("maps section in %s: \"%s\" "
> +						   "has unrecognized, non-zero "
> +						   "options\n",
> +						   obj->path, map_name);
> +					return -EINVAL;
> +				}
> +			}
> +			obj->maps[map_idx].def = *def;

I'm not too happy/comfortable with this way of copying the memory of
"def" (the type-cased struct bpf_map_def).  I guess it works, and is
part of the C-standard(?).


> +		}
>  		map_idx++;
>  	}
>  
>  	qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]), compare_bpf_map);
> -	return bpf_object__validate_maps(obj);
> +	return bpf_object__validate_maps(obj, map_def_sz);
>  }
>  
>  static int bpf_object__elf_collect(struct bpf_object *obj)

Besides above comment, I think the patch is correct, based on what I
did in commit 156450d9d964 ("samples/bpf: make bpf_load.c code
compatible with ELF maps section changes").

  https://git.kernel.org/torvalds/c/156450d9d964

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

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

[Jesper Dangaard Brouer]

On Mon,  2 Oct 2017 12:41:28 -0400
Craig Gallek <kraigatgoog@gmail.com> wrote:

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 4f402dcdf372..28b300868ad7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
>  }
>  
>  static int
> -bpf_object__validate_maps(struct bpf_object *obj)
> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz)
>  {
>  	int i;
>  
> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj)
>  		const struct bpf_map *a = &obj->maps[i - 1];
>  		const struct bpf_map *b = &obj->maps[i];
>  
> -		if (b->offset - a->offset < sizeof(struct bpf_map_def)) {
> -			pr_warning("corrupted map section in %s: map \"%s\" too small\n",
> -				   obj->path, a->name);
> +		if (b->offset - a->offset < map_def_sz) {
> +			pr_warning("corrupted map section in %s: map \"%s\" too small "
> +				   "(%zd vs %d)\n",
> +				   obj->path, a->name, b->offset - a->offset,
> +				   map_def_sz);
>  			return -EINVAL;

Hmm... one more comment.  You have just coded handling of ELF
map_def_sz which are smaller in a safe manor, but here this case will
get rejected (in bpf_object__validate_maps).  That cannot be the right
intend?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

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

[Daniel Borkmann]

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.

Re: [PATCH net-next v2 0/2] libbpf: support more map options

[David Miller]

From: Craig Gallek <kraigatgoog@gmail.com>
Date: Mon,  2 Oct 2017 12:41:27 -0400

> From: Craig Gallek <kraig@google.com>
> 
> The functional change to this series is the ability to use flags when
> creating maps from object files loaded by libbpf.  In order to do this,
> the first patch updates the library to handle map definitions that
> differ in size from libbpf's struct bpf_map_def.
> 
> For object files with a larger map definition, libbpf will continue to load
> if the unknown fields are all zero, otherwise the map is rejected.  If the
> map definition in the object file is smaller than expected, libbpf will use
> zero as a default value in the missing fields.

Judging by the feedback I anticipate another spin of this series.

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

[Craig Gallek]

On Tue, Oct 3, 2017 at 10:03 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
>
> First of all, thank you Craig for working on this.  As Alexei says, we
> need to improve tools/lib/bpf/libbpf and move towards converting users
> of bpf_load.c to this lib instead.
>
> Comments inlined below.
>
>> +                     obj->maps[map_idx].def = *def;
>
> I'm not too happy/comfortable with this way of copying the memory of
> "def" (the type-cased struct bpf_map_def).  I guess it works, and is
> part of the C-standard(?).

I believe this is a C++-ism.  I'm not sure if it was pulled into the
C99 standard or if it's just a gcc 'feature' now.  I kept it because
it was in the initial code, but I'm happy to do an explicit copy for
v3 if that looks better.

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

[Craig Gallek]

On Tue, Oct 3, 2017 at 10:11 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Mon,  2 Oct 2017 12:41:28 -0400
> Craig Gallek <kraigatgoog@gmail.com> wrote:
>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 4f402dcdf372..28b300868ad7 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
>>  }
>>
>>  static int
>> -bpf_object__validate_maps(struct bpf_object *obj)
>> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz)
>>  {
>>       int i;
>>
>> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj)
>>               const struct bpf_map *a = &obj->maps[i - 1];
>>               const struct bpf_map *b = &obj->maps[i];
>>
>> -             if (b->offset - a->offset < sizeof(struct bpf_map_def)) {
>> -                     pr_warning("corrupted map section in %s: map \"%s\" too small\n",
>> -                                obj->path, a->name);
>> +             if (b->offset - a->offset < map_def_sz) {
>> +                     pr_warning("corrupted map section in %s: map \"%s\" too small "
>> +                                "(%zd vs %d)\n",
>> +                                obj->path, a->name, b->offset - a->offset,
>> +                                map_def_sz);
>>                       return -EINVAL;
>
> Hmm... one more comment.  You have just coded handling of ELF
> map_def_sz which are smaller in a safe manor, but here this case will
> get rejected (in bpf_object__validate_maps).  That cannot be the right
> intend?

I think you are right, but my test program didn't catch this...
Perhaps an off-by-one (I only used slightly smaller map_def sizes).  I
suppose this entire function is unnecessary now.  Even the old version
wouldn't catch the case where the last offset was out of bounds?

> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

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

[Craig Gallek]

On Tue, Oct 3, 2017 at 10:11 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Mon,  2 Oct 2017 12:41:28 -0400
> Craig Gallek <kraigatgoog@gmail.com> wrote:
>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 4f402dcdf372..28b300868ad7 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -580,7 +580,7 @@ bpf_object__init_kversion(struct bpf_object *obj,
>>  }
>>
>>  static int
>> -bpf_object__validate_maps(struct bpf_object *obj)
>> +bpf_object__validate_maps(struct bpf_object *obj, int map_def_sz)
>>  {
>>       int i;
>>
>> @@ -595,9 +595,11 @@ bpf_object__validate_maps(struct bpf_object *obj)
>>               const struct bpf_map *a = &obj->maps[i - 1];
>>               const struct bpf_map *b = &obj->maps[i];
>>
>> -             if (b->offset - a->offset < sizeof(struct bpf_map_def)) {
>> -                     pr_warning("corrupted map section in %s: map \"%s\" too small\n",
>> -                                obj->path, a->name);
>> +             if (b->offset - a->offset < map_def_sz) {
>> +                     pr_warning("corrupted map section in %s: map \"%s\" too small "
>> +                                "(%zd vs %d)\n",
>> +                                obj->path, a->name, b->offset - a->offset,
>> +                                map_def_sz);
>>                       return -EINVAL;
>
> Hmm... one more comment.  You have just coded handling of ELF
> map_def_sz which are smaller in a safe manor, but here this case will
> get rejected (in bpf_object__validate_maps).  That cannot be the right
> intend?

I think you are right, but my test program didn't catch this...
Perhaps an off-by-one (I only used slightly smaller map_def sizes).  I
suppose this entire function is unnecessary now.  Even the old version
wouldn't catch the case where the last offset was out of bounds?

> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

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

[Craig Gallek]

On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann <daniel@iogearbox.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.

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

[David Laight]

From: Craig Gallek
> Sent: 04 October 2017 14:58
> >> +                     obj->maps[map_idx].def = *def;
> >
> > I'm not too happy/comfortable with this way of copying the memory of
> > "def" (the type-cased struct bpf_map_def).  I guess it works, and is
> > part of the C-standard(?).
> 
> I believe this is a C++-ism.  I'm not sure if it was pulled into the
> C99 standard or if it's just a gcc 'feature' now.  I kept it because
> it was in the initial code, but I'm happy to do an explicit copy for
> v3 if that looks better.

Structure copies have been valid C for ages.

Annoyingly gcc will call memcpy() directly (not a 'private' function).

	David

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

[Daniel Borkmann]

On 10/04/2017 03:59 PM, Craig Gallek wrote:
> On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann <daniel@iogearbox.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.

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

[Alexei Starovoitov]

On 10/4/17 12:27 PM, Daniel Borkmann wrote:
> On 10/04/2017 03:59 PM, Craig Gallek wrote:
>> On Tue, Oct 3, 2017 at 10:39 AM, Daniel Borkmann
>> <daniel@iogearbox.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.

makes sense to me :)