| Message ID | 20190308051814.19218-1-tehnerd@tehnerd.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v3,bpf] : libbpf: fixing leak when kernel does not support btf | expand |
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) { >
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!
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 >
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 >>
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) {
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(+)