[PATCH bpf 0/2] libbpf: fix inner map removal in bpf_object__create_map

[PATCH bpf 0/2] libbpf: fix inner map removal in bpf_object__create_map

[Martynas Pumputis]

This patch set fixes the removal of a BTF-defined inner map if the
creation of the outer map has failed. This was identified by Andrii
in [1].

[1]: https://lore.kernel.org/bpf/CAEf4BzYaQsD6NaEUij6ttDeKYP7oEB0=c0D9_xdAKw6FYb7h1g@mail.gmail.com/

Martynas Pumputis (2):
  libbpf: fix removal of inner map in bpf_object__create_map
  selftests/bpf: check inner map deletion

 tools/lib/bpf/libbpf.c                        |  5 +-
 .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
 tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
 3 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c

-- 
2.32.0

[PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Martynas Pumputis]

If creating an outer map of a BTF-defined map-in-map fails (via
bpf_object__create_map()), then the previously created its inner map
won't be destroyed.

Fix this by ensuring that the destroy routines are not bypassed in the
case of a failure.

Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
---
 tools/lib/bpf/libbpf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6f5e2757bb3c..1a840e81ea0a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 {
 	struct bpf_create_map_attr create_attr;
 	struct bpf_map_def *def = &map->def;
+	int ret = 0;
 
 	memset(&create_attr, 0, sizeof(create_attr));
 
@@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	}
 
 	if (map->fd < 0)
-		return -errno;
+		ret = -errno;
 
 	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
 		if (obj->gen_loader)
@@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		zfree(&map->inner_map);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
-- 
2.32.0

RE: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[John Fastabend]

Martynas Pumputis wrote:
> If creating an outer map of a BTF-defined map-in-map fails (via
> bpf_object__create_map()), then the previously created its inner map
> won't be destroyed.
> 
> Fix this by ensuring that the destroy routines are not bypassed in the
> case of a failure.
> 
> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
>  tools/lib/bpf/libbpf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6f5e2757bb3c..1a840e81ea0a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  {
>  	struct bpf_create_map_attr create_attr;
>  	struct bpf_map_def *def = &map->def;
> +	int ret = 0;
>  
>  	memset(&create_attr, 0, sizeof(create_attr));
>  
> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  	}
>  
>  	if (map->fd < 0)
> -		return -errno;
> +		ret = -errno;
>  

I'm trying to track this down, not being overly familiar with this bit of
code.

We entered bpf_object__create_map with map->inner_map potentially set and
then here we are going to zfree(&map->inner_map). I'm trying to track
down if this is problematic, I guess not? But seems like we could
also free a map here that we didn't create from this call in the above
logic.

>  	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {

        if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
                if (obj->gen_loader)
                        map->inner_map->fd = -1;
                bpf_map__destroy(map->inner_map);
                zfree(&map->inner_map);
        }


Also not sure here, sorry didn't have time to follow too thoroughly
will check again later. But, the 'map->inner_map->fd = -1' is going to
cause bpf_map__destroy to bypass the close(fd) as I understand it.
So are we leaking an fd if the inner_map->fd is coming from above
create? Or maybe never happens because obj->gen_loader is NULL?

Thanks!


>  		if (obj->gen_loader)
> @@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  		zfree(&map->inner_map);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> -- 
> 2.32.0
> 

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Martynas Pumputis]

On 7/15/21 2:30 AM, John Fastabend wrote:
> Martynas Pumputis wrote:
>> If creating an outer map of a BTF-defined map-in-map fails (via
>> bpf_object__create_map()), then the previously created its inner map
>> won't be destroyed.
>>
>> Fix this by ensuring that the destroy routines are not bypassed in the
>> case of a failure.
>>
>> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
>> Reported-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Martynas Pumputis <m@lambda.lt>
>> ---
>>   tools/lib/bpf/libbpf.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 6f5e2757bb3c..1a840e81ea0a 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>   {
>>   	struct bpf_create_map_attr create_attr;
>>   	struct bpf_map_def *def = &map->def;
>> +	int ret = 0;
>>   
>>   	memset(&create_attr, 0, sizeof(create_attr));
>>   
>> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>   	}
>>   
>>   	if (map->fd < 0)
>> -		return -errno;
>> +		ret = -errno;
>>   
> 
> I'm trying to track this down, not being overly familiar with this bit of
> code.
> 
> We entered bpf_object__create_map with map->inner_map potentially set and
> then here we are going to zfree(&map->inner_map). I'm trying to track
> down if this is problematic, I guess not? But seems like we could
> also free a map here that we didn't create from this call in the above
> logic.
> 

Keep in mind that we free the inner map anyway if the creation of the 
outer map was successful. Also, we don't try to recreate the map if any 
of the steps has failed. So I think it should not be problematic.


>>   	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
> 
>          if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
>                  if (obj->gen_loader)
>                          map->inner_map->fd = -1;
>                  bpf_map__destroy(map->inner_map);
>                  zfree(&map->inner_map);
>          }
> 
> 
> Also not sure here, sorry didn't have time to follow too thoroughly
> will check again later. But, the 'map->inner_map->fd = -1' is going to
> cause bpf_map__destroy to bypass the close(fd) as I understand it.
> So are we leaking an fd if the inner_map->fd is coming from above
> create? Or maybe never happens because obj->gen_loader is NULL?

I think in the case of obj->gen_loader, we don't need to close the FD of 
any map, as the creation of maps will happen at a later stage in the 
kernel: 
https://lore.kernel.org/bpf/20210514003623.28033-15-alexei.starovoitov@gmail.com/.

> 
> Thanks!
> 
> 
>>   		if (obj->gen_loader)
>> @@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>   		zfree(&map->inner_map);
>>   	}
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
>> -- 
>> 2.32.0
>>
> 
> 

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[John Fastabend]

Martynas Pumputis wrote:
> 
> 
> On 7/15/21 2:30 AM, John Fastabend wrote:
> > Martynas Pumputis wrote:
> >> If creating an outer map of a BTF-defined map-in-map fails (via
> >> bpf_object__create_map()), then the previously created its inner map
> >> won't be destroyed.
> >>
> >> Fix this by ensuring that the destroy routines are not bypassed in the
> >> case of a failure.
> >>
> >> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> >> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> >> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> >> ---
> >>   tools/lib/bpf/libbpf.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 6f5e2757bb3c..1a840e81ea0a 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
> >>   {
> >>   	struct bpf_create_map_attr create_attr;
> >>   	struct bpf_map_def *def = &map->def;
> >> +	int ret = 0;
> >>   
> >>   	memset(&create_attr, 0, sizeof(create_attr));
> >>   
> >> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
> >>   	}
> >>   
> >>   	if (map->fd < 0)
> >> -		return -errno;
> >> +		ret = -errno;
> >>   
> > 
> > I'm trying to track this down, not being overly familiar with this bit of
> > code.
> > 
> > We entered bpf_object__create_map with map->inner_map potentially set and
> > then here we are going to zfree(&map->inner_map). I'm trying to track
> > down if this is problematic, I guess not? But seems like we could
> > also free a map here that we didn't create from this call in the above
> > logic.
> > 
> 
> Keep in mind that we free the inner map anyway if the creation of the 
> outer map was successful. Also, we don't try to recreate the map if any 
> of the steps has failed. So I think it should not be problematic.

Maybe not problematic, but I'm still missing a bit of detail here.
We create an inner map then immediately destroy it? I'll need to
reread to understand the why part.

> 
> 
> >>   	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
> > 
> >          if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
> >                  if (obj->gen_loader)
> >                          map->inner_map->fd = -1;
> >                  bpf_map__destroy(map->inner_map);
> >                  zfree(&map->inner_map);
> >          }
> > 
> > 
> > Also not sure here, sorry didn't have time to follow too thoroughly
> > will check again later. But, the 'map->inner_map->fd = -1' is going to
> > cause bpf_map__destroy to bypass the close(fd) as I understand it.
> > So are we leaking an fd if the inner_map->fd is coming from above
> > create? Or maybe never happens because obj->gen_loader is NULL?
> 
> I think in the case of obj->gen_loader, we don't need to close the FD of 
> any map, as the creation of maps will happen at a later stage in the 
> kernel: 
> https://lore.kernel.org/bpf/20210514003623.28033-15-alexei.starovoitov@gmail.com/.

+1

> 
> > 
> > Thanks!
> > 
> > 

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[John Fastabend]

John Fastabend wrote:
> Martynas Pumputis wrote:
> > 
> > 
> > On 7/15/21 2:30 AM, John Fastabend wrote:
> > > Martynas Pumputis wrote:
> > >> If creating an outer map of a BTF-defined map-in-map fails (via
> > >> bpf_object__create_map()), then the previously created its inner map
> > >> won't be destroyed.
> > >>
> > >> Fix this by ensuring that the destroy routines are not bypassed in the
> > >> case of a failure.
> > >>
> > >> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> > >> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> > >> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> > >> ---
> > >>   tools/lib/bpf/libbpf.c | 5 +++--
> > >>   1 file changed, 3 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > >> index 6f5e2757bb3c..1a840e81ea0a 100644
> > >> --- a/tools/lib/bpf/libbpf.c
> > >> +++ b/tools/lib/bpf/libbpf.c
> > >> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
> > >>   {
> > >>   	struct bpf_create_map_attr create_attr;
> > >>   	struct bpf_map_def *def = &map->def;
> > >> +	int ret = 0;
> > >>   
> > >>   	memset(&create_attr, 0, sizeof(create_attr));
> > >>   
> > >> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
> > >>   	}
> > >>   
> > >>   	if (map->fd < 0)
> > >> -		return -errno;
> > >> +		ret = -errno;
> > >>   
> > > 
> > > I'm trying to track this down, not being overly familiar with this bit of
> > > code.
> > > 
> > > We entered bpf_object__create_map with map->inner_map potentially set and
> > > then here we are going to zfree(&map->inner_map). I'm trying to track
> > > down if this is problematic, I guess not? But seems like we could
> > > also free a map here that we didn't create from this call in the above
> > > logic.
> > > 
> > 
> > Keep in mind that we free the inner map anyway if the creation of the 
> > outer map was successful. Also, we don't try to recreate the map if any 
> > of the steps has failed. So I think it should not be problematic.
> 
> Maybe not problematic, but I'm still missing a bit of detail here.
> We create an inner map then immediately destroy it? I'll need to
> reread to understand the why part.

OK understand now ;)

RE: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[John Fastabend]

Martynas Pumputis wrote:
> If creating an outer map of a BTF-defined map-in-map fails (via
> bpf_object__create_map()), then the previously created its inner map
> won't be destroyed.
> 
> Fix this by ensuring that the destroy routines are not bypassed in the
> case of a failure.
> 
> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---

LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Andrii Nakryiko]

On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
>
> If creating an outer map of a BTF-defined map-in-map fails (via
> bpf_object__create_map()), then the previously created its inner map
> won't be destroyed.
>
> Fix this by ensuring that the destroy routines are not bypassed in the
> case of a failure.
>
> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
>  tools/lib/bpf/libbpf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6f5e2757bb3c..1a840e81ea0a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  {
>         struct bpf_create_map_attr create_attr;
>         struct bpf_map_def *def = &map->def;
> +       int ret = 0;
>
>         memset(&create_attr, 0, sizeof(create_attr));
>
> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>         }
>
>         if (map->fd < 0)
> -               return -errno;
> +               ret = -errno;

Oh, isn't this a complicated function, eh? I stared at the code for a
while until I understood the whole idea with map->inner_map handling
there.

I think your change is correct, I'd just love you to consolidate all
those "int err" definitions, and use just one throughout this
function. It will clean up two other if() blocks, and in this case
"err" name is more appropriate, because it always is <= 0.

>
>         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
>                 if (obj->gen_loader)
> @@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                 zfree(&map->inner_map);
>         }
>
> -       return 0;
> +       return ret;
>  }
>
>  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> --
> 2.32.0
>

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Martynas Pumputis]

On 7/16/21 7:27 AM, Andrii Nakryiko wrote:
> On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
>>
>> If creating an outer map of a BTF-defined map-in-map fails (via
>> bpf_object__create_map()), then the previously created its inner map
>> won't be destroyed.
>>
>> Fix this by ensuring that the destroy routines are not bypassed in the
>> case of a failure.
>>
>> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
>> Reported-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Martynas Pumputis <m@lambda.lt>
>> ---
>>   tools/lib/bpf/libbpf.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 6f5e2757bb3c..1a840e81ea0a 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>   {
>>          struct bpf_create_map_attr create_attr;
>>          struct bpf_map_def *def = &map->def;
>> +       int ret = 0;
>>
>>          memset(&create_attr, 0, sizeof(create_attr));
>>
>> @@ -4561,7 +4562,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>          }
>>
>>          if (map->fd < 0)
>> -               return -errno;
>> +               ret = -errno;
> 
> Oh, isn't this a complicated function, eh? I stared at the code for a
> while until I understood the whole idea with map->inner_map handling
> there.
> 
> I think your change is correct, I'd just love you to consolidate all
> those "int err" definitions, and use just one throughout this
> function. It will clean up two other if() blocks, and in this case
> "err" name is more appropriate, because it always is <= 0.

Good idea. I will send v2 once we have agreed on the selftesting issue.

> 
>>
>>          if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
>>                  if (obj->gen_loader)
>> @@ -4570,7 +4571,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>>                  zfree(&map->inner_map);
>>          }
>>
>> -       return 0;
>> +       return ret;
>>   }
>>
>>   static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
>> --
>> 2.32.0
>>

[PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis]

Add a test case to check whether an unsuccessful creation of an outer
map of a BTF-defined map-in-map destroys the inner map.

As bpf_object__create_map() is a static function, we cannot just call it
from the test case and then check whether a map accessible via
map->inner_map_fd has been removed. Instead, we iterate over all maps
and check whether the map "$MAP_NAME.inner" does not exist.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
---
 .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
 tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c

diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
new file mode 100644
index 000000000000..03601779e4ed
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Isovalent, Inc. */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct inner {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, __u32);
+	__type(value, int);
+	__uint(max_entries, 4);
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, 0); /* This will make map creation to fail */
+	__uint(key_size, sizeof(__u32));
+	__array(values, struct inner);
+} mim SEC(".maps");
+
+SEC("xdp_noop")
+int xdp_noop0(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 30cbf5d98f7d..48f6c6dfd188 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
 }
 
 #define MAPINMAP_PROG "./test_map_in_map.o"
+#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
 static void test_map_in_map(void)
 {
 	struct bpf_object *obj;
 	struct bpf_map *map;
 	int mim_fd, fd, err;
 	int pos = 0;
+	struct bpf_map_info info = {};
+	__u32 len = sizeof(info);
+	__u32 id = 0;
 
 	obj = bpf_object__open(MAPINMAP_PROG);
 
@@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
 
 	close(fd);
 	bpf_object__close(obj);
+
+
+	/* Test that failing bpf_object__create_map() destroys the inner map */
+
+	obj = bpf_object__open(MAPINMAP_INVALID_PROG);
+
+	map = bpf_object__find_map_by_name(obj, "mim");
+	if (!map) {
+		printf("Failed to load array of maps from test prog\n");
+		goto out_map_in_map;
+	}
+
+	err = bpf_object__load(obj);
+	if (!err) {
+		printf("Loading obj supposed to fail\n");
+		goto out_map_in_map;
+	}
+
+	/* Iterate over all maps to check whether the internal map
+	 * ("mim.internal") has been destroyed.
+	 */
+	while (true) {
+		err = bpf_map_get_next_id(id, &id);
+		if (err) {
+			if (errno == ENOENT)
+				break;
+			printf("Failed to get next map: %d", errno);
+			goto out_map_in_map;
+		}
+
+		fd = bpf_map_get_fd_by_id(id);
+		if (fd < 0) {
+			if (errno == ENOENT)
+				continue;
+			printf("Failed to get map by id %u: %d", id, errno);
+			goto out_map_in_map;
+		}
+
+		err = bpf_obj_get_info_by_fd(fd, &info, &len);
+		if (err) {
+			printf("Failed to get map info by fd %d: %d", fd,
+			       errno);
+			goto out_map_in_map;
+		}
+
+		if (!strcmp(info.name, "mim.inner")) {
+			printf("Inner map mim.inner was not destroyed\n");
+			goto out_map_in_map;
+		}
+	}
+
 	return;
 
 out_map_in_map:
-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	exit(1);
 }
 
-- 
2.32.0

RE: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[John Fastabend]

Martynas Pumputis wrote:
> Add a test case to check whether an unsuccessful creation of an outer
> map of a BTF-defined map-in-map destroys the inner map.
> 
> As bpf_object__create_map() is a static function, we cannot just call it
> from the test case and then check whether a map accessible via
> map->inner_map_fd has been removed. Instead, we iterate over all maps
> and check whether the map "$MAP_NAME.inner" does not exist.
> 
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
>
> Add a test case to check whether an unsuccessful creation of an outer
> map of a BTF-defined map-in-map destroys the inner map.
>
> As bpf_object__create_map() is a static function, we cannot just call it
> from the test case and then check whether a map accessible via
> map->inner_map_fd has been removed. Instead, we iterate over all maps
> and check whether the map "$MAP_NAME.inner" does not exist.
>
> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
>  .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
>  tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
>  2 files changed, 84 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>
> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> new file mode 100644
> index 000000000000..03601779e4ed
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Isovalent, Inc. */
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct inner {
> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> +       __type(key, __u32);
> +       __type(value, int);
> +       __uint(max_entries, 4);
> +};
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> +       __uint(max_entries, 0); /* This will make map creation to fail */
> +       __uint(key_size, sizeof(__u32));
> +       __array(values, struct inner);
> +} mim SEC(".maps");
> +
> +SEC("xdp_noop")
> +int xdp_noop0(struct xdp_md *ctx)
> +{
> +       return XDP_PASS;
> +}
> +
> +int _version SEC("version") = 1;

please don't add new uses of version, it's completely unnecessary on
modern kernels

> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> index 30cbf5d98f7d..48f6c6dfd188 100644
> --- a/tools/testing/selftests/bpf/test_maps.c
> +++ b/tools/testing/selftests/bpf/test_maps.c
> @@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
>  }
>
>  #define MAPINMAP_PROG "./test_map_in_map.o"
> +#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
>  static void test_map_in_map(void)
>  {
>         struct bpf_object *obj;
>         struct bpf_map *map;
>         int mim_fd, fd, err;
>         int pos = 0;
> +       struct bpf_map_info info = {};
> +       __u32 len = sizeof(info);
> +       __u32 id = 0;
>
>         obj = bpf_object__open(MAPINMAP_PROG);
>
> @@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
>
>         close(fd);
>         bpf_object__close(obj);
> +
> +
> +       /* Test that failing bpf_object__create_map() destroys the inner map */
> +
> +       obj = bpf_object__open(MAPINMAP_INVALID_PROG);

you didn't check bpf_object__open() succeeded here...

> +
> +       map = bpf_object__find_map_by_name(obj, "mim");

... and crash will happen here on error

> +       if (!map) {
> +               printf("Failed to load array of maps from test prog\n");
> +               goto out_map_in_map;
> +       }
> +

[...]

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Martynas Pumputis]

On 7/16/21 7:35 AM, Andrii Nakryiko wrote:
> On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
>>
>> Add a test case to check whether an unsuccessful creation of an outer
>> map of a BTF-defined map-in-map destroys the inner map.
>>
>> As bpf_object__create_map() is a static function, we cannot just call it
>> from the test case and then check whether a map accessible via
>> map->inner_map_fd has been removed. Instead, we iterate over all maps
>> and check whether the map "$MAP_NAME.inner" does not exist.
>>
>> Signed-off-by: Martynas Pumputis <m@lambda.lt>
>> ---
>>   .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
>>   tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
>>   2 files changed, 84 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>>
>> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>> new file mode 100644
>> index 000000000000..03601779e4ed
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Isovalent, Inc. */
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +
>> +struct inner {
>> +       __uint(type, BPF_MAP_TYPE_ARRAY);
>> +       __type(key, __u32);
>> +       __type(value, int);
>> +       __uint(max_entries, 4);
>> +};
>> +
>> +struct {
>> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
>> +       __uint(max_entries, 0); /* This will make map creation to fail */
>> +       __uint(key_size, sizeof(__u32));
>> +       __array(values, struct inner);
>> +} mim SEC(".maps");
>> +
>> +SEC("xdp_noop")
>> +int xdp_noop0(struct xdp_md *ctx)
>> +{
>> +       return XDP_PASS;
>> +}
>> +
>> +int _version SEC("version") = 1;
> 
> please don't add new uses of version, it's completely unnecessary on
> modern kernels

Sure.

> 
>> +char _license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
>> index 30cbf5d98f7d..48f6c6dfd188 100644
>> --- a/tools/testing/selftests/bpf/test_maps.c
>> +++ b/tools/testing/selftests/bpf/test_maps.c
>> @@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
>>   }
>>
>>   #define MAPINMAP_PROG "./test_map_in_map.o"
>> +#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
>>   static void test_map_in_map(void)
>>   {
>>          struct bpf_object *obj;
>>          struct bpf_map *map;
>>          int mim_fd, fd, err;
>>          int pos = 0;
>> +       struct bpf_map_info info = {};
>> +       __u32 len = sizeof(info);
>> +       __u32 id = 0;
>>
>>          obj = bpf_object__open(MAPINMAP_PROG);
>>
>> @@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
>>
>>          close(fd);
>>          bpf_object__close(obj);
>> +
>> +
>> +       /* Test that failing bpf_object__create_map() destroys the inner map */
>> +
>> +       obj = bpf_object__open(MAPINMAP_INVALID_PROG);
> 
> you didn't check bpf_object__open() succeeded here...

For the sake of brevity, I didn't add the check. If the opening fails, 
then we will catch it anyway with the bpf_object__find_map_by_name() 
invocation below: it will log "libbpf: elf: failed to open $PROG_NAME: 
No such file or directory" and then segfault.

> 
>> +
>> +       map = bpf_object__find_map_by_name(obj, "mim");
> 
> ... and crash will happen here on error
> 
>> +       if (!map) {
>> +               printf("Failed to load array of maps from test prog\n");
>> +               goto out_map_in_map;
>> +       }
>> +
> 
> [...]
> 

Re: [PATCH bpf 2/2] selftests/bpf: check inner map deletion

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 8:07 AM Martynas Pumputis <m@lambda.lt> wrote:
>
>
>
> On 7/16/21 7:35 AM, Andrii Nakryiko wrote:
> > On Wed, Jul 14, 2021 at 9:52 AM Martynas Pumputis <m@lambda.lt> wrote:
> >>
> >> Add a test case to check whether an unsuccessful creation of an outer
> >> map of a BTF-defined map-in-map destroys the inner map.
> >>
> >> As bpf_object__create_map() is a static function, we cannot just call it
> >> from the test case and then check whether a map accessible via
> >> map->inner_map_fd has been removed. Instead, we iterate over all maps
> >> and check whether the map "$MAP_NAME.inner" does not exist.
> >>
> >> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> >> ---
> >>   .../bpf/progs/test_map_in_map_invalid.c       | 27 +++++++++
> >>   tools/testing/selftests/bpf/test_maps.c       | 58 ++++++++++++++++++-
> >>   2 files changed, 84 insertions(+), 1 deletion(-)
> >>   create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >> new file mode 100644
> >> index 000000000000..03601779e4ed
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c
> >> @@ -0,0 +1,27 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (c) 2021 Isovalent, Inc. */
> >> +#include <linux/bpf.h>
> >> +#include <bpf/bpf_helpers.h>
> >> +
> >> +struct inner {
> >> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> >> +       __type(key, __u32);
> >> +       __type(value, int);
> >> +       __uint(max_entries, 4);
> >> +};
> >> +
> >> +struct {
> >> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> >> +       __uint(max_entries, 0); /* This will make map creation to fail */
> >> +       __uint(key_size, sizeof(__u32));
> >> +       __array(values, struct inner);
> >> +} mim SEC(".maps");
> >> +
> >> +SEC("xdp_noop")
> >> +int xdp_noop0(struct xdp_md *ctx)
> >> +{
> >> +       return XDP_PASS;
> >> +}
> >> +
> >> +int _version SEC("version") = 1;
> >
> > please don't add new uses of version, it's completely unnecessary on
> > modern kernels
>
> Sure.
>
> >
> >> +char _license[] SEC("license") = "GPL";
> >> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> >> index 30cbf5d98f7d..48f6c6dfd188 100644
> >> --- a/tools/testing/selftests/bpf/test_maps.c
> >> +++ b/tools/testing/selftests/bpf/test_maps.c
> >> @@ -1153,12 +1153,16 @@ static void test_sockmap(unsigned int tasks, void *data)
> >>   }
> >>
> >>   #define MAPINMAP_PROG "./test_map_in_map.o"
> >> +#define MAPINMAP_INVALID_PROG "./test_map_in_map_invalid.o"
> >>   static void test_map_in_map(void)
> >>   {
> >>          struct bpf_object *obj;
> >>          struct bpf_map *map;
> >>          int mim_fd, fd, err;
> >>          int pos = 0;
> >> +       struct bpf_map_info info = {};
> >> +       __u32 len = sizeof(info);
> >> +       __u32 id = 0;
> >>
> >>          obj = bpf_object__open(MAPINMAP_PROG);
> >>
> >> @@ -1229,10 +1233,62 @@ static void test_map_in_map(void)
> >>
> >>          close(fd);
> >>          bpf_object__close(obj);
> >> +
> >> +
> >> +       /* Test that failing bpf_object__create_map() destroys the inner map */
> >> +
> >> +       obj = bpf_object__open(MAPINMAP_INVALID_PROG);
> >
> > you didn't check bpf_object__open() succeeded here...
>
> For the sake of brevity, I didn't add the check. If the opening fails,
> then we will catch it anyway with the bpf_object__find_map_by_name()
> invocation below: it will log "libbpf: elf: failed to open $PROG_NAME:
> No such file or directory" and then segfault.

Yeah, and then, due to the brevity you mentioned, someone like me will
go and waste time understanding what and where is crashing in our CIs.
Please add a proper check. Crashing test runners due to some
semi-expected failure is not an option. It happens in existing tests
very rarely, unfortunately, but it's a bug, not a feature.

>
> >
> >> +
> >> +       map = bpf_object__find_map_by_name(obj, "mim");
> >
> > ... and crash will happen here on error
> >
> >> +       if (!map) {
> >> +               printf("Failed to load array of maps from test prog\n");
> >> +               goto out_map_in_map;
> >> +       }
> >> +
> >
> > [...]
> >

[PATCH v2 bpf 0/2] libbpf: fix inner map removal in bpf_object__create_map

[Martynas Pumputis]

This patch set fixes the removal of a BTF-defined inner map if the
creation of the outer map has failed. This was identified by Andrii
in [1].

[1]: https://lore.kernel.org/bpf/CAEf4BzYaQsD6NaEUij6ttDeKYP7oEB0=c0D9_xdAKw6FYb7h1g@mail.gmail.com/



Martynas Pumputis (2):
  libbpf: fix removal of inner map in bpf_object__create_map
  selftests/bpf: check inner map deletion

 tools/lib/bpf/libbpf.c                        | 11 ++--
 .../bpf/progs/test_map_in_map_invalid.c       | 26 ++++++++
 tools/testing/selftests/bpf/test_maps.c       | 64 ++++++++++++++++++-
 3 files changed, 95 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_map_in_map_invalid.c

-- 
2.32.0

[PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Martynas Pumputis]

If creating an outer map of a BTF-defined map-in-map fails (via
bpf_object__create_map()), then the previously created its inner map
won't be destroyed.

Fix this by ensuring that the destroy routines are not bypassed in the
case of a failure.

Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
---
 tools/lib/bpf/libbpf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6f5e2757bb3c..dde521366579 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 {
 	struct bpf_create_map_attr create_attr;
 	struct bpf_map_def *def = &map->def;
+	int err = 0;
 
 	memset(&create_attr, 0, sizeof(create_attr));
 
@@ -4521,8 +4522,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 
 	if (bpf_map_type__is_map_in_map(def->type)) {
 		if (map->inner_map) {
-			int err;
-
 			err = bpf_object__create_map(obj, map->inner_map, true);
 			if (err) {
 				pr_warn("map '%s': failed to create inner map: %d\n",
@@ -4547,7 +4546,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	if (map->fd < 0 && (create_attr.btf_key_type_id ||
 			    create_attr.btf_value_type_id)) {
 		char *cp, errmsg[STRERR_BUFSIZE];
-		int err = -errno;
+		err = -errno;
 
 		cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
 		pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
@@ -4560,8 +4559,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		map->fd = bpf_create_map_xattr(&create_attr);
 	}
 
-	if (map->fd < 0)
-		return -errno;
+	err = map->fd < 0 ? -errno : 0;
 
 	if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
 		if (obj->gen_loader)
@@ -4570,7 +4568,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 		zfree(&map->inner_map);
 	}
 
-	return 0;
+	return err;
 }
 
 static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
-- 
2.32.0

Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map

[Andrii Nakryiko]

On Mon, Jul 19, 2021 at 10:36 AM Martynas Pumputis <m@lambda.lt> wrote:
>
> If creating an outer map of a BTF-defined map-in-map fails (via
> bpf_object__create_map()), then the previously created its inner map
> won't be destroyed.
>
> Fix this by ensuring that the destroy routines are not bypassed in the
> case of a failure.
>
> Fixes: 646f02ffdd49c ("libbpf: Add BTF-defined map-in-map support")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>

Please preserve received acks between versions (unless some
significant changes happen between versions invalidating the original
ack):

Acked-by: John Fastabend <john.fastabend@gmail.com>

Thanks, applied (with few adjustments) to bpf-next, given we lived
with this bug for a long while and no one ever noticed/reported it
(and BPF map will be cleaned up when the process exits anyway).
Keeping it in bpf-next will make it easier for follow-up patches
changing/enhancing bpf_map__create_map() and will avoid merge
conflicts down the line.

> Signed-off-by: Martynas Pumputis <m@lambda.lt>
> ---
>  tools/lib/bpf/libbpf.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6f5e2757bb3c..dde521366579 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4479,6 +4479,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>  {
>         struct bpf_create_map_attr create_attr;
>         struct bpf_map_def *def = &map->def;
> +       int err = 0;
>
>         memset(&create_attr, 0, sizeof(create_attr));
>
> @@ -4521,8 +4522,6 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>
>         if (bpf_map_type__is_map_in_map(def->type)) {
>                 if (map->inner_map) {
> -                       int err;
> -
>                         err = bpf_object__create_map(obj, map->inner_map, true);
>                         if (err) {
>                                 pr_warn("map '%s': failed to create inner map: %d\n",
> @@ -4547,7 +4546,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>         if (map->fd < 0 && (create_attr.btf_key_type_id ||
>                             create_attr.btf_value_type_id)) {
>                 char *cp, errmsg[STRERR_BUFSIZE];
> -               int err = -errno;

needed empty line here


> +               err = -errno;
>
>                 cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>                 pr_warn("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> @@ -4560,8 +4559,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                 map->fd = bpf_create_map_xattr(&create_attr);
>         }
>
> -       if (map->fd < 0)
> -               return -errno;
> +       err = map->fd < 0 ? -errno : 0;
>
>         if (bpf_map_type__is_map_in_map(def->type) && map->inner_map) {
>                 if (obj->gen_loader)
> @@ -4570,7 +4568,7 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
>                 zfree(&map->inner_map);
>         }
>
> -       return 0;
> +       return err;
>  }
>
>  static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> --
> 2.32.0
>