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

From: Martynas Pumputis <m@lambda.lt>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf 1/2] libbpf: fix removal of inner map in bpf_object__create_map
Date: Fri, 16 Jul 2021 17:24:14 +0200	[thread overview]
Message-ID: <ca789cd7-b8c9-08f9-fca0-d25549967d0d@lambda.lt> (raw)
In-Reply-To: <CAEf4BzbNpqkDGfprj_hH-=3zZNxZ7SkEsCRZnb5==6vfAoXt8w@mail.gmail.com>

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
>>