Re: [PATCH bpf] libbpf: Enforce program limit only for loaded programs

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5116 bytes --]

> libbpf: Enforce program limit only for loaded programs

TL;DR:
- Patch is based on a tree lacking its Fixes: target aac5ba0a4d52; applied
  there, the early E2BIG check in bpf_gen__init() may still reject the object
  and the new clamp is dead code
- Fixes: tag may need to name ba05fd36b851

>     libbpf: Enforce program limit only for loaded programs
>
>     As explained in commit '259172bb6514 ("libbpf: Fix gen_loader assumption
>     on number of programs.")', nr_progs which is obj->nr_programs at this
>     stage is including subprogs and autoload=false progs which are filtered
>     later during the generation.
>     Quoting Alexei, previous patch 'aac5ba0a4d52 ("libbpf: Reject excessive
>     program counts in generated loaders")' does move rejection at gen time
>     instead of load time, but some programs are still wrongfully rejected:
>
>       Two objects with obj->nr_programs == 33:
>
>         object               before aac5ba0a  after aac5ba0a should be
>         -------------------  ---------------  -------------  -------------
>         33 progs             -EACCES at load  -E2BIG at gen  -E2BIG at gen
>         1 prog, 32 subprogs  -EACCES at load  -E2BIG at gen  loads
>
>       The second object fails today because of the cleanup loop in
>       bpf_gen__init(). It closes nr_progs slots of prog_fd[], and
>       prog_fd[32] is at r10 + 0:
>
>         r1 = *(u32 *)(r10 +0)
>         invalid read from stack R10 off=0 size=4
>
>     We now clamp the loop generating the cleanups, which may still be more
>     numerous than necessary and do the actual E2BIG check just before
>     writing the actual prog_fd into the stack for the cleanups, after
>     bpf_object__load_progs() has been doing the filtering on progs.
>
>     Fixes: aac5ba0a4d52 ("libbpf: Reject excessive program counts in generated loaders")
>     Suggested-by: Alexei Starovoitov <ast@kernel.org>
>     Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>

The commit message's own table shows that the "1 prog, 32 subprogs" object
already failed with -EACCES at load time before aac5ba0a4d52, and the cause
it names is the cleanup loop in bpf_gen__init() that closes nr_progs slots of
prog_fd[].

That unclamped nr_progs-sized loop was introduced by ba05fd36b851 ("libbpf:
Perform map fd cleanup for gen_loader in case of error"), where
bpf_gen__init() computes nr_progs_sz from obj->nr_programs.  Commit
259172bb6514 ("libbpf: Fix gen_loader assumption on number of programs.")
then relaxed bpf_gen__finish() so that such objects reached load time, and is
a possible alternative target.

aac5ba0a4d52 only moved the failure from load time to gen time.

Should the Fixes: tag point at the commit that introduced the problem
instead, so the stable backport scope covers kernels since 5.16:

  Fixes: ba05fd36b851 ("libbpf: Perform map fd cleanup for gen_loader in case of error")

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index 251392aa8b41..555d4757a898 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -139,6 +139,8 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level, int nr_progs, int nr_maps
>  	emit(gen, BPF_MOV64_IMM(BPF_REG_3, 0));
>  	emit(gen, BPF_EMIT_CALL(BPF_FUNC_probe_read_kernel));
>
> +	/* nr_progs includes subprogs and progs that are not loaded */
> +	nr_progs = min(nr_progs, MAX_USED_PROGS);
>  	/* amount of stack actually used, only used to calculate iterations, not stack offset */
>  	nr_progs_sz = offsetof(struct loader_stack, prog_fd[nr_progs]);
>  	/* jump over cleanup code */

Does this fix work on a tree that contains the Fixes: target?

The preimage blob of this diff (251392aa8b41) is the file as it was before
aac5ba0a4d52, and aac5ba0a4d52 is not an ancestor of this commit.  The patch
applies to aac5ba0a4d52:tools/lib/bpf/gen_loader.c with only an offset, and
leaves this check in bpf_gen__init() untouched:

	if (nr_progs > MAX_USED_PROGS) {
		pr_warn("Total programs exceeds %d\n", MAX_USED_PROGS);
		gen->error = -E2BIG;
		return;
	}

With that check present, bpf_object_load() still passes obj->nr_programs ==
33 to bpf_gen__init() for the "1 prog, 32 subprogs" object.  The early return
sets gen->error = -E2BIG before the new min() clamp is reached, and
bpf_gen__finish() returns -E2BIG, so the object still fails at gen time
instead of loading.

The new min(nr_progs, MAX_USED_PROGS) is also dead code there, since nr_progs
can never exceed MAX_USED_PROGS once that check has passed.

In the tree this diff was generated against, which lacks aac5ba0a4d52, the
code is correct, but then the Fixes: tag names a commit that is not in this
history.

Should this be rebased onto aac5ba0a4d52 and also delete the early nr_progs
check in bpf_gen__init(), or should the Fixes: tag name the commit that
actually introduced the problem (ba05fd36b851)?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41035838634