Re: [RFC PATCH] bpf: test: parallel execution of basic tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] bpf: test: missing workqueue cleanup on error paths in test_bpf_init()
- [Medium] bpf: data race on global net_device dev in populate_skb()
- [Low] bpf: test: silent log drop for expected failures and generation errors
- [Low] bpf: memory leak of bpf_prog in generate_filter() error paths
--

commit eef3e387313bc16bbeb43208bc729ec2b250d2a8
Author: Dmitry Antipov <dmantipov@yandex.ru>

bpf: test: parallel execution of basic tests

This commit implements a workqueue-based parallel execution for the basic
BPF tests, adding a legacy fallback option.

> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -15012,14 +15032,14 @@ static struct bpf_prog *generate_filter(int which, int *err)
>  		fprog.len = flen;
>  
>  		*err = bpf_prog_create(&fp, &fprog);
> -		if (tests[which].aux & FLAG_EXPECTED_FAIL) {
> -			if (*err == tests[which].expected_errcode) {
> -				pr_cont("PASS\n");
> +		if (test->aux & FLAG_EXPECTED_FAIL) {
> +			if (*err == test->expected_errcode) {
> +				test_log(test, "PASS\n");
>  				/* Verifier rejected filter as expected. */
>  				*err = 0;
>  				return NULL;
>  			} else {
> -				pr_cont("UNEXPECTED_PASS\n");
> +				test_log(test, "UNEXPECTED_PASS\n");
>  				/* Verifier didn't reject the test that's
>  				 * bad enough, just return!
>  				 */
>  				*err = -EINVAL;
>  				return NULL;
>  			}

[Severity: Low]
Is there a memory leak of the fp pointer on an unexpected pass?

If bpf_prog_create successfully allocates fp (returning 0) but the test
was marked with FLAG_EXPECTED_FAIL, this branch sets *err to -EINVAL and
returns NULL without cleaning up the allocated fp.

[ ... ]

>  		/* We cannot error here as we don't need type compatibility
> @@ -15055,7 +15075,7 @@ static struct bpf_prog *generate_filter(int which, int *err)
>  		 */
>  		fp = bpf_prog_select_runtime(fp, err);
>  		if (*err) {
> -			pr_cont("FAIL to select_runtime err=%d\n", *err);
> +			test_log(test, "FAIL to select_runtime err=%d\n", *err);
>  			return NULL;
>  		}
>  		break;

[Severity: Low]
Similarly, if bpf_prog_select_runtime fails, it returns the allocated
fp. Could returning NULL here without freeing it result in a memory leak?

[ ... ]

>  static void test_bpf_work_handler(struct work_struct *work)
>  {
[ ... ]
> +	if (fp == NULL) {
> +		if (err == 0) {
> +			atomic_inc(&pass_cnt);
> +			return;
> +		}
> +		atomic_inc(&err_cnt);
> +		return;
> +	}

[Severity: Low]
Is it intended for this early return to bypass printing the test output
buffered in test->output?

For tests marked with FLAG_EXPECTED_FAIL that successfully trigger a
verifier rejection, or for unexpected generation errors, returning
early here skips the final pr_info call, which causes the test log
output to be completely lost.

[ ... ]

> +	err = run_one(fp, test);

[Severity: Medium]
Can parallel workers executing run_one() cause a data race on the global
net_device dev in populate_skb()?

Workers execute this path concurrently, and generate_test_data() calls
populate_skb(), which unconditionally writes to the shared static struct
net_device dev without synchronization:

lib/test_bpf.c:populate_skb() {
    ...
    dev_net_set(&dev, &init_net);
    skb->dev = &dev;
    skb->dev->ifindex = SKB_DEV_IFINDEX;
    skb->dev->type = SKB_DEV_TYPE;
    ...
}

[ ... ]

>  static int __init test_bpf_init(void)
>  {
[ ... ]
>  	if (!strlen(test_suite) || !strcmp(test_suite, "test_bpf")) {
>  		ret = test_bpf();
>  		if (ret)
>  			return ret;
>  	}
>  
>  	if (!strlen(test_suite) || !strcmp(test_suite, "test_tail_calls")) {
>  		ret = prepare_tail_call_tests(&progs);
>  		if (ret)
>  			return ret;
>  		ret = test_tail_calls(progs);
>  		destroy_tail_call_tests(progs);
>  		if (ret)
>  			return ret;
>  	}

[Severity: Medium]
Would these error paths leave the newly allocated bpf_wq workqueue leaked
during module initialization?

If test_bpf(), prepare_tail_call_tests(), or test_tail_calls() return an
error code, the direct return paths leave the globally allocated bpf_wq
un-freed. Should these error paths be calling destroy_workqueue(bpf_wq)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006192840.155680-1-dmantipov@yandex.ru?part=1