[PATCH bpf] libbpf: Perform map fd cleanup for gen_loader in case of error

[PATCH bpf] libbpf: Perform map fd cleanup for gen_loader in case of error

[Kumar Kartikeya Dwivedi]

Alexei reported a fd leak issue in gen loader (when invoked from
bpftool) [0]. When adding ksym support, map fd allocation was moved from
stack to loader map, however I missed closing these fds (relevant when
cleanup label is jumped to on error). For the success case, the
allocated fd is returned in loader ctx, hence this problem is not
noticed.

Make two fixes, first MAX_USED_MAPS in MAX_FD_ARRAY_SZ instead of
MAX_USED_PROGS, the braino was not a problem until now for this case as
we didn't try to close map fds (otherwise use of it would have tried
closing 32 additional fds in ksym btf fd range). Then, do a cleanup for
all MAX_USED_MAPS fds in cleanup label code, so that in case of error
all temporary map fds from bpf_gen__map_create are closed.

Also, move allocation of gen->fd_array offset to bpf_gen__init. Since
offset can now be 0, and we already continue even if add_data returns 0
in case of failure, we do not need to distinguish between 0 offset and
failure case 0, as we rely on bpf_gen__finish to check errors. We can
also skip check for gen->fd_array in add_*_fd functions, since
bpf_gen__init will take care of it.

  [0]: https://lore.kernel.org/bpf/CAADnVQJ6jSitKSNKyxOrUzwY2qDRX0sPkJ=VLGHuCLVJ=qOt9g@mail.gmail.com

Fixes: 18f4fccbf314 ("libbpf: Update gen_loader to emit BTF_KIND_FUNC relocations")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/gen_loader.c | 67 ++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 7b73f97b1fa1..558479c13c77 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -18,7 +18,7 @@
 #define MAX_USED_MAPS	64
 #define MAX_USED_PROGS	32
 #define MAX_KFUNC_DESCS 256
-#define MAX_FD_ARRAY_SZ (MAX_USED_PROGS + MAX_KFUNC_DESCS)
+#define MAX_FD_ARRAY_SZ (MAX_USED_MAPS + MAX_KFUNC_DESCS)
 
 /* The following structure describes the stack layout of the loader program.
  * In addition R6 contains the pointer to context.
@@ -42,6 +42,11 @@ struct loader_stack {
 
 #define attr_field(attr, field) (attr + offsetof(union bpf_attr, field))
 
+static int blob_fd_array_off(struct bpf_gen *gen, int index)
+{
+	return gen->fd_array + index * sizeof(int);
+}
+
 static int realloc_insn_buf(struct bpf_gen *gen, __u32 size)
 {
 	size_t off = gen->insn_cur - gen->insn_start;
@@ -102,6 +107,27 @@ static void emit2(struct bpf_gen *gen, struct bpf_insn insn1, struct bpf_insn in
 	emit(gen, insn2);
 }
 
+static int add_data(struct bpf_gen *gen, const void *data, __u32 size)
+{
+	__u32 size8 = roundup(size, 8);
+	__u64 zero = 0;
+	void *prev;
+
+	if (realloc_data_buf(gen, size8))
+		return 0;
+	prev = gen->data_cur;
+	if (data) {
+		memcpy(gen->data_cur, data, size);
+		memcpy(gen->data_cur + size, &zero, size8 - size);
+	} else {
+		memset(gen->data_cur, 0, size8);
+	}
+	gen->data_cur += size8;
+	return prev - gen->data_start;
+}
+
+static void emit_sys_close_blob(struct bpf_gen *gen, int blob_off);
+
 void bpf_gen__init(struct bpf_gen *gen, int log_level)
 {
 	size_t stack_sz = sizeof(struct loader_stack);
@@ -120,8 +146,12 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
 
 	/* jump over cleanup code */
 	emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0,
-			      /* size of cleanup code below */
-			      (stack_sz / 4) * 3 + 2));
+			      /* size of cleanup code below (including map fd cleanup) */
+			      (stack_sz / 4) * 3 + 2 + (MAX_USED_MAPS *
+			      /* 6 insns for emit_sys_close_blob,
+			       * 6 insns for debug_regs in emit_sys_close_blob
+			       */
+			      (6 + (gen->log_level ? 6 : 0)))));
 
 	/* remember the label where all error branches will jump to */
 	gen->cleanup_label = gen->insn_cur - gen->insn_start;
@@ -131,37 +161,19 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
 		emit(gen, BPF_JMP_IMM(BPF_JSLE, BPF_REG_1, 0, 1));
 		emit(gen, BPF_EMIT_CALL(BPF_FUNC_sys_close));
 	}
+	gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
+	for (i = 0; i < MAX_USED_MAPS; i++)
+		emit_sys_close_blob(gen, blob_fd_array_off(gen, i));
 	/* R7 contains the error code from sys_bpf. Copy it into R0 and exit. */
 	emit(gen, BPF_MOV64_REG(BPF_REG_0, BPF_REG_7));
 	emit(gen, BPF_EXIT_INSN());
 }
 
-static int add_data(struct bpf_gen *gen, const void *data, __u32 size)
-{
-	__u32 size8 = roundup(size, 8);
-	__u64 zero = 0;
-	void *prev;
-
-	if (realloc_data_buf(gen, size8))
-		return 0;
-	prev = gen->data_cur;
-	if (data) {
-		memcpy(gen->data_cur, data, size);
-		memcpy(gen->data_cur + size, &zero, size8 - size);
-	} else {
-		memset(gen->data_cur, 0, size8);
-	}
-	gen->data_cur += size8;
-	return prev - gen->data_start;
-}
-
 /* Get index for map_fd/btf_fd slot in reserved fd_array, or in data relative
  * to start of fd_array. Caller can decide if it is usable or not.
  */
 static int add_map_fd(struct bpf_gen *gen)
 {
-	if (!gen->fd_array)
-		gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
 	if (gen->nr_maps == MAX_USED_MAPS) {
 		pr_warn("Total maps exceeds %d\n", MAX_USED_MAPS);
 		gen->error = -E2BIG;
@@ -174,8 +186,6 @@ static int add_kfunc_btf_fd(struct bpf_gen *gen)
 {
 	int cur;
 
-	if (!gen->fd_array)
-		gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
 	if (gen->nr_fd_array == MAX_KFUNC_DESCS) {
 		cur = add_data(gen, NULL, sizeof(int));
 		return (cur - gen->fd_array) / sizeof(int);
@@ -183,11 +193,6 @@ static int add_kfunc_btf_fd(struct bpf_gen *gen)
 	return MAX_USED_MAPS + gen->nr_fd_array++;
 }
 
-static int blob_fd_array_off(struct bpf_gen *gen, int index)
-{
-	return gen->fd_array + index * sizeof(int);
-}
-
 static int insn_bytes_to_bpf_size(__u32 sz)
 {
 	switch (sz) {
-- 
2.33.1

Re: [PATCH bpf] libbpf: Perform map fd cleanup for gen_loader in case of error

[Alexei Starovoitov]

On Sat, Nov 13, 2021 at 01:54:21AM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index 7b73f97b1fa1..558479c13c77 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -18,7 +18,7 @@
>  #define MAX_USED_MAPS	64
>  #define MAX_USED_PROGS	32
>  #define MAX_KFUNC_DESCS 256
> -#define MAX_FD_ARRAY_SZ (MAX_USED_PROGS + MAX_KFUNC_DESCS)
> +#define MAX_FD_ARRAY_SZ (MAX_USED_MAPS + MAX_KFUNC_DESCS)

Lol. Not sure how I missed it during code review :)

>  void bpf_gen__init(struct bpf_gen *gen, int log_level)
>  {
>  	size_t stack_sz = sizeof(struct loader_stack);
> @@ -120,8 +146,12 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
>  
>  	/* jump over cleanup code */
>  	emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0,
> -			      /* size of cleanup code below */
> -			      (stack_sz / 4) * 3 + 2));
> +			      /* size of cleanup code below (including map fd cleanup) */
> +			      (stack_sz / 4) * 3 + 2 + (MAX_USED_MAPS *
> +			      /* 6 insns for emit_sys_close_blob,
> +			       * 6 insns for debug_regs in emit_sys_close_blob
> +			       */
> +			      (6 + (gen->log_level ? 6 : 0)))));
>  
>  	/* remember the label where all error branches will jump to */
>  	gen->cleanup_label = gen->insn_cur - gen->insn_start;
> @@ -131,37 +161,19 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
>  		emit(gen, BPF_JMP_IMM(BPF_JSLE, BPF_REG_1, 0, 1));
>  		emit(gen, BPF_EMIT_CALL(BPF_FUNC_sys_close));
>  	}
> +	gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));

could you move this line to be the first thing in bpf_gen__init() ?
Otherwise it looks like that fd_array is only used in cleanup part while
it's actually needed everywhere.

> +	for (i = 0; i < MAX_USED_MAPS; i++)
> +		emit_sys_close_blob(gen, blob_fd_array_off(gen, i));

I confess that commit 30f51aedabda ("libbpf: Cleanup temp FDs when intermediate sys_bpf fails.")
wasn't great in terms of redundant code gen for closing all 32 + 64 FDs.
But can we make it better while we're at it?
Most bpf files don't have 32 progs and 64 maps while gen_loader emits
(32 + 64) * 6 = 576 instructions (without debug).
While debugging/developing gen_loader this large cleanup code is just noise.

I tested the following:

diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
index 75ca9fb857b2..cc486a77db65 100644
--- a/tools/lib/bpf/bpf_gen_internal.h
+++ b/tools/lib/bpf/bpf_gen_internal.h
@@ -47,8 +47,8 @@ struct bpf_gen {
        int nr_fd_array;
 };

-void bpf_gen__init(struct bpf_gen *gen, int log_level);
-int bpf_gen__finish(struct bpf_gen *gen);
+void bpf_gen__init(struct bpf_gen *gen, int log_level, int nr_progs, int nr_maps);
+int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps);
 void bpf_gen__free(struct bpf_gen *gen);
 void bpf_gen__load_btf(struct bpf_gen *gen, const void *raw_data, __u32 raw_size);
 void bpf_gen__map_create(struct bpf_gen *gen, struct bpf_create_map_params *map_attr, int map_idx);
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 7b73f97b1fa1..f7b78478a9d3 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -102,7 +102,7 @@ static void emit2(struct bpf_gen *gen, struct bpf_insn insn1, struct bpf_insn in
        emit(gen, insn2);
 }

-void bpf_gen__init(struct bpf_gen *gen, int log_level)
+void bpf_gen__init(struct bpf_gen *gen, int log_level, int nr_progs, int nr_maps)
 {
        size_t stack_sz = sizeof(struct loader_stack);
        int i;
@@ -359,10 +359,15 @@ static void emit_sys_close_blob(struct bpf_gen *gen, int blob_off)
        __emit_sys_close(gen);
 }

-int bpf_gen__finish(struct bpf_gen *gen)
+int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps)
 {
        int i;

+       if (nr_progs != gen->nr_progs || nr_maps != gen->nr_maps) {
+               pr_warn("progs/maps mismatch\n");
+               gen->error = -EFAULT;
+               return gen->error;
+       }
        emit_sys_close_stack(gen, stack_off(btf_fd));
        for (i = 0; i < gen->nr_progs; i++)
                move_stack2ctx(gen,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index de7e09a6b5ec..f6faa33c80fa 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7263,7 +7263,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
        }

        if (obj->gen_loader)
-               bpf_gen__init(obj->gen_loader, attr->log_level);
+               bpf_gen__init(obj->gen_loader, attr->log_level, obj->nr_programs, obj->nr_maps);

        err = bpf_object__probe_loading(obj);
        err = err ? : bpf_object__load_vmlinux_btf(obj, false);
@@ -7282,7 +7282,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
                for (i = 0; i < obj->nr_maps; i++)
                        obj->maps[i].fd = -1;
                if (!err)
-                       err = bpf_gen__finish(obj->gen_loader);
+                       err = bpf_gen__finish(obj->gen_loader, obj->nr_programs, obj->nr_maps);
        }

        /* clean up fd_array */

and it seems to work.

So cleanup code can close only nr_progs + nr_maps FDs.
gen_loader prog will be much shorter and will be processed by the verifier faster.
MAX_FD_ARRAY_SZ can stay fixed. Reducing data size is not worth it.
wdyt?

Re: [PATCH bpf] libbpf: Perform map fd cleanup for gen_loader in case of error

[Kumar Kartikeya Dwivedi]

On Sat, Nov 13, 2021 at 03:04:11AM IST, Alexei Starovoitov wrote:
> On Sat, Nov 13, 2021 at 01:54:21AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> > index 7b73f97b1fa1..558479c13c77 100644
> > --- a/tools/lib/bpf/gen_loader.c
> > +++ b/tools/lib/bpf/gen_loader.c
> > @@ -18,7 +18,7 @@
> >  #define MAX_USED_MAPS	64
> >  #define MAX_USED_PROGS	32
> >  #define MAX_KFUNC_DESCS 256
> > -#define MAX_FD_ARRAY_SZ (MAX_USED_PROGS + MAX_KFUNC_DESCS)
> > +#define MAX_FD_ARRAY_SZ (MAX_USED_MAPS + MAX_KFUNC_DESCS)
>
> Lol. Not sure how I missed it during code review :)
>
> >  void bpf_gen__init(struct bpf_gen *gen, int log_level)
> >  {
> >  	size_t stack_sz = sizeof(struct loader_stack);
> > @@ -120,8 +146,12 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> >
> >  	/* jump over cleanup code */
> >  	emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0,
> > -			      /* size of cleanup code below */
> > -			      (stack_sz / 4) * 3 + 2));
> > +			      /* size of cleanup code below (including map fd cleanup) */
> > +			      (stack_sz / 4) * 3 + 2 + (MAX_USED_MAPS *
> > +			      /* 6 insns for emit_sys_close_blob,
> > +			       * 6 insns for debug_regs in emit_sys_close_blob
> > +			       */
> > +			      (6 + (gen->log_level ? 6 : 0)))));
> >
> >  	/* remember the label where all error branches will jump to */
> >  	gen->cleanup_label = gen->insn_cur - gen->insn_start;
> > @@ -131,37 +161,19 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> >  		emit(gen, BPF_JMP_IMM(BPF_JSLE, BPF_REG_1, 0, 1));
> >  		emit(gen, BPF_EMIT_CALL(BPF_FUNC_sys_close));
> >  	}
> > +	gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
>
> could you move this line to be the first thing in bpf_gen__init() ?
> Otherwise it looks like that fd_array is only used in cleanup part while
> it's actually needed everywhere.
>

Ack. Also thinking of not reordering add_data as that pollutes git blame, but
just adding a declaration before use.

> > +	for (i = 0; i < MAX_USED_MAPS; i++)
> > +		emit_sys_close_blob(gen, blob_fd_array_off(gen, i));
>
> I confess that commit 30f51aedabda ("libbpf: Cleanup temp FDs when intermediate sys_bpf fails.")
> wasn't great in terms of redundant code gen for closing all 32 + 64 FDs.
> But can we make it better while we're at it?
> Most bpf files don't have 32 progs and 64 maps while gen_loader emits
> (32 + 64) * 6 = 576 instructions (without debug).
> While debugging/developing gen_loader this large cleanup code is just noise.
>

Yeah, I've been thinking about this for a while, there's also lots of similar
code gen in e.g. test_ksyms_module.o for the relocations. It might make sense to
move to subprog approach and emit a BPF_CALL, but that's a separate issue. I can
look into that too if it sounds good (but maybe you already tried this and ran
into issues).

> I tested the following:
>
> diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
> index 75ca9fb857b2..cc486a77db65 100644
> --- a/tools/lib/bpf/bpf_gen_internal.h
> +++ b/tools/lib/bpf/bpf_gen_internal.h
> @@ -47,8 +47,8 @@ struct bpf_gen {
>         int nr_fd_array;
>  };
>
> -void bpf_gen__init(struct bpf_gen *gen, int log_level);
> -int bpf_gen__finish(struct bpf_gen *gen);
> +void bpf_gen__init(struct bpf_gen *gen, int log_level, int nr_progs, int nr_maps);
> +int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps);
>  void bpf_gen__free(struct bpf_gen *gen);
>  void bpf_gen__load_btf(struct bpf_gen *gen, const void *raw_data, __u32 raw_size);
>  void bpf_gen__map_create(struct bpf_gen *gen, struct bpf_create_map_params *map_attr, int map_idx);
> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index 7b73f97b1fa1..f7b78478a9d3 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -102,7 +102,7 @@ static void emit2(struct bpf_gen *gen, struct bpf_insn insn1, struct bpf_insn in
>         emit(gen, insn2);
>  }
>
> -void bpf_gen__init(struct bpf_gen *gen, int log_level)
> +void bpf_gen__init(struct bpf_gen *gen, int log_level, int nr_progs, int nr_maps)
>  {
>         size_t stack_sz = sizeof(struct loader_stack);
>         int i;
> @@ -359,10 +359,15 @@ static void emit_sys_close_blob(struct bpf_gen *gen, int blob_off)
>         __emit_sys_close(gen);
>  }
>
> -int bpf_gen__finish(struct bpf_gen *gen)
> +int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps)
>  {
>         int i;
>
> +       if (nr_progs != gen->nr_progs || nr_maps != gen->nr_maps) {
> +               pr_warn("progs/maps mismatch\n");
> +               gen->error = -EFAULT;
> +               return gen->error;
> +       }
>         emit_sys_close_stack(gen, stack_off(btf_fd));
>         for (i = 0; i < gen->nr_progs; i++)
>                 move_stack2ctx(gen,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index de7e09a6b5ec..f6faa33c80fa 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7263,7 +7263,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>         }
>
>         if (obj->gen_loader)
> -               bpf_gen__init(obj->gen_loader, attr->log_level);
> +               bpf_gen__init(obj->gen_loader, attr->log_level, obj->nr_programs, obj->nr_maps);
>
>         err = bpf_object__probe_loading(obj);
>         err = err ? : bpf_object__load_vmlinux_btf(obj, false);
> @@ -7282,7 +7282,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>                 for (i = 0; i < obj->nr_maps; i++)
>                         obj->maps[i].fd = -1;
>                 if (!err)
> -                       err = bpf_gen__finish(obj->gen_loader);
> +                       err = bpf_gen__finish(obj->gen_loader, obj->nr_programs, obj->nr_maps);
>         }
>
>         /* clean up fd_array */
>
> and it seems to work.
>
> So cleanup code can close only nr_progs + nr_maps FDs.
> gen_loader prog will be much shorter and will be processed by the verifier faster.
> MAX_FD_ARRAY_SZ can stay fixed. Reducing data size is not worth it.
> wdyt?

Looks nice, I'll rework it like this.

--
Kartikeya

Re: [PATCH bpf] libbpf: Perform map fd cleanup for gen_loader in case of error

[Alexei Starovoitov]

On Fri, Nov 12, 2021 at 1:47 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sat, Nov 13, 2021 at 03:04:11AM IST, Alexei Starovoitov wrote:
> > On Sat, Nov 13, 2021 at 01:54:21AM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> > > index 7b73f97b1fa1..558479c13c77 100644
> > > --- a/tools/lib/bpf/gen_loader.c
> > > +++ b/tools/lib/bpf/gen_loader.c
> > > @@ -18,7 +18,7 @@
> > >  #define MAX_USED_MAPS      64
> > >  #define MAX_USED_PROGS     32
> > >  #define MAX_KFUNC_DESCS 256
> > > -#define MAX_FD_ARRAY_SZ (MAX_USED_PROGS + MAX_KFUNC_DESCS)
> > > +#define MAX_FD_ARRAY_SZ (MAX_USED_MAPS + MAX_KFUNC_DESCS)
> >
> > Lol. Not sure how I missed it during code review :)
> >
> > >  void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > >  {
> > >     size_t stack_sz = sizeof(struct loader_stack);
> > > @@ -120,8 +146,12 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > >
> > >     /* jump over cleanup code */
> > >     emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0,
> > > -                         /* size of cleanup code below */
> > > -                         (stack_sz / 4) * 3 + 2));
> > > +                         /* size of cleanup code below (including map fd cleanup) */
> > > +                         (stack_sz / 4) * 3 + 2 + (MAX_USED_MAPS *
> > > +                         /* 6 insns for emit_sys_close_blob,
> > > +                          * 6 insns for debug_regs in emit_sys_close_blob
> > > +                          */
> > > +                         (6 + (gen->log_level ? 6 : 0)))));
> > >
> > >     /* remember the label where all error branches will jump to */
> > >     gen->cleanup_label = gen->insn_cur - gen->insn_start;
> > > @@ -131,37 +161,19 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > >             emit(gen, BPF_JMP_IMM(BPF_JSLE, BPF_REG_1, 0, 1));
> > >             emit(gen, BPF_EMIT_CALL(BPF_FUNC_sys_close));
> > >     }
> > > +   gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
> >
> > could you move this line to be the first thing in bpf_gen__init() ?
> > Otherwise it looks like that fd_array is only used in cleanup part while
> > it's actually needed everywhere.
> >
>
> Ack. Also thinking of not reordering add_data as that pollutes git blame, but
> just adding a declaration before use.

git blame is not a big deal. Both options are fine.


>
> > > +   for (i = 0; i < MAX_USED_MAPS; i++)
> > > +           emit_sys_close_blob(gen, blob_fd_array_off(gen, i));
> >
> > I confess that commit 30f51aedabda ("libbpf: Cleanup temp FDs when intermediate sys_bpf fails.")
> > wasn't great in terms of redundant code gen for closing all 32 + 64 FDs.
> > But can we make it better while we're at it?
> > Most bpf files don't have 32 progs and 64 maps while gen_loader emits
> > (32 + 64) * 6 = 576 instructions (without debug).
> > While debugging/developing gen_loader this large cleanup code is just noise.
> >
>
> Yeah, I've been thinking about this for a while, there's also lots of similar
> code gen in e.g. test_ksyms_module.o for the relocations. It might make sense to
> move to subprog approach and emit a BPF_CALL, but that's a separate issue. I can
> look into that too if it sounds good (but maybe you already tried this and ran
> into issues).

Do you mean to split things like emit_sys_close into its own subprog
and call it ?
I'm not sure it's large enough to do that.
Or you mean the code that copies?
There is a lot of it in test_ksyms_module.o
Would be good to reduce it, but it's a corner case
and probably not typical.
I'm all ears.

> > So cleanup code can close only nr_progs + nr_maps FDs.
> > gen_loader prog will be much shorter and will be processed by the verifier faster.
> > MAX_FD_ARRAY_SZ can stay fixed. Reducing data size is not worth it.
> > wdyt?
>
> Looks nice, I'll rework it like this.

Thanks. Pls keep targeting bpf tree with respin.