[PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[Nikita V. Shirokov]

we could end up in situation when we have object file w/ all btf
info but kernel does not support btf yet. in this situation currently
libbpf just set obj->btf to NULL w/o freeing it first.
this patch if fixing it by making sure to run btf__free first

v2->v3:
 - adding "Fixes" tag

v1->v2:
 - adding netdev to cc

Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f5eb60379c8d..d5b830d60601 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -838,6 +838,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 			if (IS_ERR(obj->btf) || btf__load(obj->btf)) {
 				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
 					   BTF_ELF_SEC, PTR_ERR(obj->btf));
+				if (!IS_ERR(obj->btf))
+					btf__free(obj->btf);
 				obj->btf = NULL;
 			}
 		} else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
-- 
2.17.1

Re: [PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[Yonghong Song]

On 3/7/19 9:18 PM, Nikita V. Shirokov wrote:
> we could end up in situation when we have object file w/ all btf
> info but kernel does not support btf yet. in this situation currently
> libbpf just set obj->btf to NULL w/o freeing it first.
> this patch if fixing it by making sure to run btf__free first
> 
> v2->v3:
>   - adding "Fixes" tag
> 
> v1->v2:
>   - adding netdev to cc
> 
> Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
> Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/lib/bpf/libbpf.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f5eb60379c8d..d5b830d60601 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -838,6 +838,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>   			if (IS_ERR(obj->btf) || btf__load(obj->btf)) {
>   				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
>   					   BTF_ELF_SEC, PTR_ERR(obj->btf));
> +				if (!IS_ERR(obj->btf))
> +					btf__free(obj->btf);
>   				obj->btf = NULL;
>   			}
>   		} else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
> 

Re: [PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[Daniel Borkmann]

On 03/08/2019 06:18 AM, Nikita V. Shirokov wrote:
> we could end up in situation when we have object file w/ all btf
> info but kernel does not support btf yet. in this situation currently
> libbpf just set obj->btf to NULL w/o freeing it first.
> this patch if fixing it by making sure to run btf__free first
> 
> v2->v3:
>  - adding "Fixes" tag
> 
> v1->v2:
>  - adding netdev to cc
> 
> Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
> Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Applied, thanks!

Re: [PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[Martin Lau]

On Fri, Mar 08, 2019 at 05:18:14AM +0000, Nikita V. Shirokov wrote:
> we could end up in situation when we have object file w/ all btf
> info but kernel does not support btf yet. in this situation currently
> libbpf just set obj->btf to NULL w/o freeing it first.
> this patch if fixing it by making sure to run btf__free first
> 
> v2->v3:
>  - adding "Fixes" tag
> 
> v1->v2:
>  - adding netdev to cc
> 
> Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
I think the Fixes tag should belong to a later patch, just in
case there will be an issue when pulling into stable:

Fixes: d29d87f7e612 ("btf: separate btf creation and loading")

> Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>
> ---
>  tools/lib/bpf/libbpf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f5eb60379c8d..d5b830d60601 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -838,6 +838,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>  			if (IS_ERR(obj->btf) || btf__load(obj->btf)) {
>  				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
>  					   BTF_ELF_SEC, PTR_ERR(obj->btf));
Printing this PTR_ERR(obj->btf) seems not very correct also if btf__load()
was the one failing.

> +				if (!IS_ERR(obj->btf))
> +					btf__free(obj->btf);
>  				obj->btf = NULL;
>  			}
>  		} else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
> -- 
> 2.17.1
> 

Re: [PATCH v3 bpf]: libbpf: fixing leak when kernel does not support btf

[Daniel Borkmann]

On 03/08/2019 05:45 PM, Martin Lau wrote:
> On Fri, Mar 08, 2019 at 05:18:14AM +0000, Nikita V. Shirokov wrote:
>> we could end up in situation when we have object file w/ all btf
>> info but kernel does not support btf yet. in this situation currently
>> libbpf just set obj->btf to NULL w/o freeing it first.
>> this patch if fixing it by making sure to run btf__free first
>>
>> v2->v3:
>>  - adding "Fixes" tag
>>
>> v1->v2:
>>  - adding netdev to cc
>>
>> Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
> I think the Fixes tag should belong to a later patch, just in
> case there will be an issue when pulling into stable:
> 
> Fixes: d29d87f7e612 ("btf: separate btf creation and loading")

Agree, I just fixed up the tags, thanks!

>> Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>
>> ---
>>  tools/lib/bpf/libbpf.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index f5eb60379c8d..d5b830d60601 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -838,6 +838,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>>  			if (IS_ERR(obj->btf) || btf__load(obj->btf)) {
>>  				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
>>  					   BTF_ELF_SEC, PTR_ERR(obj->btf));
> Printing this PTR_ERR(obj->btf) seems not very correct also if btf__load()
> was the one failing.

Makes sense as well. Given this is logically independent from addressing
this leak, please send a follow-up fix for the pr_warning().

>> +				if (!IS_ERR(obj->btf))
>> +					btf__free(obj->btf);
>>  				obj->btf = NULL;
>>  			}
>>  		} else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
>> -- 
>> 2.17.1
>>