Re: [PATCH bpf-next v6 7/7] selftests/bpf: Convert veristat to BPF_PROG_LOAD_STRATEGY_MANUAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: veristat uninitialized memory read of log buffer on early load failure
- [Low] bpf: veristat skips cleanup and info retrieval when BPF program fd is 0
--

commit 9ac34aec88e7eee9b39c53e8055bc5989345ce5b
Author: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>
Subject: selftests/bpf: Convert veristat to BPF_PROG_LOAD_STRATEGY_MANUAL

This commit updates the veristat utility to verify programs using
BPF_PROG_LOAD_STRATEGY_MANUAL alongside bpf_program__load() and
bpf_program__unload(). It replaces the previous use of bpf_program__clone(),
ensuring RODATA maps are properly bound during verification and subsequently
dropped upon unload.

> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 9cfc9b4b41c12..0cd19b59037f6 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
[ ... ]
> @@ -1726,18 +1725,15 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>  	if (env.force_reg_invariants)
>  		bpf_program__add_flags(prog, BPF_F_TEST_REG_INVARIANTS);
>  
> -	opts.log_buf = buf;
> -	opts.log_size = buf_sz;
> -	opts.log_level = log_level;
> +	bpf_program__set_log_buf(prog, buf, buf_sz);
> +	bpf_program__set_log_level(prog, log_level);
>  
>  	cgroup_err = reset_stat_cgroup();
>  	mem_peak_a = cgroup_memory_peak();
> -	fd = bpf_program__clone(prog, &opts);
> -	if (fd < 0) {
> -		err = fd;
> -		if (env.verbose)
> -			fprintf(stderr, "Failed to load program %s %d\n", prog_name, err);
> -	}
> +	err = bpf_program__load(prog);

[Severity: Medium]
If bpf_program__load() returns an early error (e.g. from libbpf without
calling the kernel), could the dynamically allocated buf remain
uninitialized?

When env.verbose or env.top_src_lines > 0 is set, buf is allocated
via malloc() but the fallback initialization only null-terminates the
static array:

	if (env.verbose || env.top_src_lines > 0) {
		buf = malloc(buf_sz);
	...
	verif_log_buf[0] = '\0';

If bpf_program__load() returns early without modifying the buffer,
parse_verif_log(buf, ...) will read this uninitialized memory. Could
buf[0] = '\0' be set directly on buf before loading?

> +	fd = err == 0 ? bpf_program__fd(prog) : -1;

[Severity: Low]
If standard input is closed, can bpf_program__load() assign file descriptor
0 to the loaded program?

If fd 0 is assigned, later checks will explicitly skip handling it:

	if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)

This causes the program's JIT statistics to be skipped. Should this check
test for fd >= 0 instead of fd > 0?

> +	if (err && env.verbose)
> +		fprintf(stderr, "Failed to load program %s %d\n", prog_name, err);
>  	mem_peak_b = cgroup_memory_peak();
>  	if (!cgroup_err && mem_peak_a >= 0 && mem_peak_b >= 0)
>  		mem_peak = mem_peak_b - mem_peak_a;
[ ... ]
> @@ -1774,7 +1770,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>  	if (verif_log_buf != buf)
>  		free(buf);
>  	if (fd > 0)
> -		close(fd);
> +		bpf_program__unload(prog);

[Severity: Low]
Similar to the issue above, if fd is 0, will this prevent the program from
being immediately unloaded and leave it in the kernel until the entire
BPF object is closed?

Should this check also use if (fd >= 0) to ensure cleanup occurs?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010143156.379864-1-andrey.grodzovsky@crowdstrike.com?part=7