Re: [PATCH bpf-next v4 3/4] selftests/bpf: Test freeing a struct_ops map under a running prog

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/bpf: Missing error path abort leads to test timeout or cascading failures
--

commit 920a79a19e26a9e6930a47b303037de287f29b6a
Author: Qiliang Yuan <odys.yuan@gmail.com>

selftests/bpf: Test freeing a struct_ops map under a running prog
    
This commit adds a new test to verify that a struct_ops map is not freed
under a running program. It refactors existing timer tests to use a shared
helper function.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
> index 461ded7223515..9d56e8cb5675a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
> @@ -157,12 +158,18 @@ static void test_st_ops_assoc_in_timer_no_uref(void)
>  	/*
>  	 * Run .test_1 by calling kfunc bpf_kfunc_multi_st_ops_test_1_prog_arg() and checks
>  	 * the return value. .test_1 will also schedule timer_cb that runs .test_1 again.
[ ... ]
>  	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.syscall_prog), NULL);
> -	ASSERT_OK(err, "bpf_prog_test_run_opts");
> +	if (!ASSERT_OK(err, "bpf_prog_test_run_opts")) {
> +		bpf_link__destroy(link);
> +		goto out;
> +	}

[Severity: Low]
Adding an abort on failure here is a nice improvement, as it prevents an
infinite loop waiting for the timer callback.

However, there are a couple of other places missing an error abort. In this
same function (run_st_ops_assoc_in_timer_no_uref), if
bpf_program__assoc_struct_ops() fails:

    err = bpf_program__assoc_struct_ops(skel->progs.syscall_prog,
                                        skel->maps.st_ops_map, NULL);
    ASSERT_OK(err, "bpf_program__assoc_struct_ops");
    
    link = bpf_map__attach_struct_ops(skel->maps.st_ops_map);

Will this cascade into failures on the subsequent attach? Should we add an
abort there as well?

Additionally, in the unmodified test_st_ops_assoc_in_timer() function in
this same file, bpf_prog_test_run_opts() is still checked with just
ASSERT_OK() and does not abort. If it fails, won't the test hang
indefinitely in its sched_yield() loop waiting for the timer callback
since the callback never runs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-selftests-bpf-struct-ops-assoc-timer-v4-0-9ad31c21220c@gmail.com?part=3