Re: [PATCH v2 26/27] selftests/landlock: Use EXPECT for closing FDs and stopping threads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/landlock: EXPECT_EQ before reading thread output in restrict()
--

commit 85f196c7c04b9c3b9c11f5b1ad1ee130ba7d4079
Author: Günther Noack <gnoack3000@gmail.com>

selftests/landlock: Use EXPECT for closing FDs and stopping threads

This commit changes ASSERT_EQ to EXPECT_EQ across Landlock selftests when
closing file descriptors, cancelling or joining threads whose results are
unused, and cleaning up at the end of named_pipe_ioctl.

> diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
> index 2b53596c986e0..aff973ba4735b 100644
> --- a/tools/testing/selftests/landlock/tsync_test.c
> +++ b/tools/testing/selftests/landlock/tsync_test.c
> @@ -152,10 +152,10 @@ TEST_F(multi_threaded, restrict)
>  	EXPECT_EQ(variant->expected_no_new_privs,
>  		  prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
>  
> -	ASSERT_EQ(0, pthread_cancel(t1));
> -	ASSERT_EQ(0, pthread_cancel(t2));
> -	ASSERT_EQ(0, pthread_join(t1, NULL));
> -	ASSERT_EQ(0, pthread_join(t2, NULL));
> +	EXPECT_EQ(0, pthread_cancel(t1));
> +	EXPECT_EQ(0, pthread_cancel(t2));
> +	EXPECT_EQ(0, pthread_join(t1, NULL));
> +	EXPECT_EQ(0, pthread_join(t2, NULL));
>  
>  	/* Checks the no_new_privs state of the sibling threads. */
>  	EXPECT_EQ(variant->expected_no_new_privs, no_new_privs1);

[Severity: Medium]
In TEST_F(multi_threaded, restrict), no_new_privs1 and no_new_privs2 are
declared uninitialized on the stack and passed to the idle() threads:

tools/testing/selftests/landlock/tsync_test.c:restrict() {
	...
	bool no_new_privs1, no_new_privs2;

	ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
	ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
	...
}

In idle(), those variables are only populated when the cancellation cleanup
handler store_no_new_privs() runs upon cancellation:

tools/testing/selftests/landlock/tsync_test.c:idle() {
	pthread_cleanup_push(store_no_new_privs, data);

	while (true)
		sleep(1);

	pthread_cleanup_pop(1);
}

Since the subsequent checks on no_new_privs1 and no_new_privs2 depend on
pthread_cancel() and pthread_join() succeeding to populate and synchronize
those values, should pthread_cancel() and pthread_join() remain ASSERT_EQ
here so that a failure does not fall through into reading uninitialized
stack variables?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-selftests-refactor-v2-0-c52f23f7ce33@gmail.com?part=26