Re: [PATCHv2 bpf] libbpf: Prevent double close of btf objects

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 15, 2026 at 12:35:59PM +0100, Alan Maguire wrote:
> On 15/04/2026 09:24, Jiri Olsa wrote:
> > Sashiko found possible double close of btf object fd [1],
> > which happens when strdup in load_module_btfs fails at which
> > point the obj->btf_module_cnt is already incremented.
> >
> 
> nit: maybe the subject should be something like
> 
> libbpf: Prevent double close, leak of btf objects
> 
> ...to reflect the leak fix too?

so the btf object leak is introduced by moving the obj->btf_module_cnt++
below, because before that it was released in bpf_object_post_load_cleanup
even when the strdup failed

now if strdup fails, obj->btf_module_cnt is not incremented yet and
bpf_object_post_load_cleanup won't release it

>  
> > The error path close btf fd and so does later cleanup code in
> > bpf_object_post_load_cleanup function.
> > 
> > Incrementing obj->btf_module_cnt only if there's no failure
> > and releasing btf object in error path.
> > 
> > Fixes: 91abb4a6d79d ("libbpf: Support attachment of BPF tracing programs to kernel modules")
> > [1] https://sashiko.dev/#/patchset/20260324081846.2334094-1-jolsa%40kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> > ---
> > v2 changes:
> > - squashed patches together, because they depend on each other [ci]
> > - added Fixes tag [ci]
> > 
> >  tools/lib/bpf/libbpf.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8b0c3246097f..79d9607d26e2 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5806,7 +5806,6 @@ static int load_module_btfs(struct bpf_object *obj)
> >  {
> >  	struct bpf_btf_info info;
> >  	struct module_btf *mod_btf;
> > -	struct btf *btf;
> >  	char name[64];
> >  	__u32 id = 0, len;
> >  	int err, fd;
> > @@ -5825,6 +5824,8 @@ static int load_module_btfs(struct bpf_object *obj)
> >  		return 0;
> >  
> >  	while (true) {
> > +		struct btf *btf = NULL;
> > +
> >  		err = bpf_btf_get_next_id(id, &id);
> >  		if (err && errno == ENOENT)
> >  			return 0;
> > @@ -5878,7 +5879,7 @@ static int load_module_btfs(struct bpf_object *obj)
> >  		if (err)
> >  			goto err_out;
> >  
> > -		mod_btf = &obj->btf_modules[obj->btf_module_cnt++];
> > +		mod_btf = &obj->btf_modules[obj->btf_module_cnt];
> >  
> >  		mod_btf->btf = btf;
> >  		mod_btf->id = id;
> > @@ -5888,9 +5889,11 @@ static int load_module_btfs(struct bpf_object *obj)
> >  			err = -ENOMEM;
> >  			goto err_out;
> >  		}
> > +		obj->btf_module_cnt++;
> >  		continue;
> >  
> >  err_out:
> > +		btf__free(btf);
> >  		close(fd);
> >  		return err;
> >  	}
> 
> the err_out label within the loop is a bit confusing, needed more now I guess
> since btf is in loop scope. only way round it would be to haul btf back up
> to global scope and explicitly NULL it at the start of each iteration. That way 
> we could break out of the loop to do error handling. Not a big deal though.

I guess you mean something like below (untested).. looks better, will check

thanks,
jirka


---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8b0c3246097f..3a80a018fc7d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5852,11 +5852,12 @@ static int load_module_btfs(struct bpf_object *obj)
 		info.name = ptr_to_u64(name);
 		info.name_len = sizeof(name);
 
+		btf = NULL;
 		err = bpf_btf_get_info_by_fd(fd, &info, &len);
 		if (err) {
 			err = -errno;
 			pr_warn("failed to get BTF object #%d info: %s\n", id, errstr(err));
-			goto err_out;
+			break;
 		}
 
 		/* ignore non-module BTFs */
@@ -5870,15 +5871,15 @@ static int load_module_btfs(struct bpf_object *obj)
 		if (err) {
 			pr_warn("failed to load module [%s]'s BTF object #%d: %s\n",
 				name, id, errstr(err));
-			goto err_out;
+			break;
 		}
 
 		err = libbpf_ensure_mem((void **)&obj->btf_modules, &obj->btf_module_cap,
 					sizeof(*obj->btf_modules), obj->btf_module_cnt + 1);
 		if (err)
-			goto err_out;
+			break;
 
-		mod_btf = &obj->btf_modules[obj->btf_module_cnt++];
+		mod_btf = &obj->btf_modules[obj->btf_module_cnt];
 
 		mod_btf->btf = btf;
 		mod_btf->id = id;
@@ -5886,16 +5887,16 @@ static int load_module_btfs(struct bpf_object *obj)
 		mod_btf->name = strdup(name);
 		if (!mod_btf->name) {
 			err = -ENOMEM;
-			goto err_out;
+			break;
 		}
-		continue;
+		obj->btf_module_cnt++;
+	}
 
-err_out:
+	if (err) {
+		btf__free(btf);
 		close(fd);
-		return err;
 	}
-
-	return 0;
+	return err;
 }
 
 static struct bpf_core_cand_list *