netdev - Re: [PATCH net-next] selftests: avoid using SKIP(exit()) in harness fixure setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 5 Mar 2024 16:54:37 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>, Mark
 Brown <broonie@...nel.org>, <ivan.orlov0322@...il.com>, <perex@...ex.cz>,
	<tiwai@...e.com>, <shuah@...nel.org>, <jglisse@...hat.com>,
	<akpm@...ux-foundation.org>, <keescook@...omium.org>,
	<linux-sound@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<linux-mm@...ck.org>
Subject: Re: [PATCH net-next] selftests: avoid using SKIP(exit()) in harness
 fixure setup

On 3/5/24 00:36, Jakub Kicinski wrote:
> selftest harness uses various exit codes to signal test
> results. Avoid calling exit() directly, otherwise tests
> may get broken by harness refactoring (like the commit
> under Fixes). SKIP() will instruct the harness that the
> test shouldn't run, it used to not be the case, but that
> has been fixed. So just return, no need to exit.
> 
> Note that for hmm-tests this actually changes the result
> from pass to skip. Which seems fair, the test is skipped,
> after all.
> 
> Reported-by: Mark Brown <broonie@...nel.org>
> Link: https://lore.kernel.org/all/05f7bf89-04a5-4b65-bf59-c19456aeb1f0@sirena.org.uk
> Fixes: a724707976b0 ("selftests: kselftest_harness: use KSFT_* exit codes")

I believe that the next patch of the linked series is a culprit,
but that does not mandate a next revision in my eyes

> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> This needs to go to net-next because that's where the breaking
> patch was (mis?)-applied.
> 
> CC: ivan.orlov0322@...il.com
> CC: perex@...ex.cz
> CC: tiwai@...e.com
> CC: broonie@...nel.org
> CC: shuah@...nel.org
> CC: jglisse@...hat.com
> CC: akpm@...ux-foundation.org
> CC: keescook@...omium.org
> CC: linux-sound@...r.kernel.org
> CC: linux-kselftest@...r.kernel.org
> CC: linux-mm@...ck.org
> ---
>   tools/testing/selftests/alsa/test-pcmtest-driver.c | 4 ++--
>   tools/testing/selftests/mm/hmm-tests.c             | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/alsa/test-pcmtest-driver.c b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> index a52ecd43dbe3..ca81afa4ee90 100644
> --- a/tools/testing/selftests/alsa/test-pcmtest-driver.c
> +++ b/tools/testing/selftests/alsa/test-pcmtest-driver.c
> @@ -127,11 +127,11 @@ FIXTURE_SETUP(pcmtest) {
>   	int err;
>   
>   	if (geteuid())
> -		SKIP(exit(-1), "This test needs root to run!");
> +		SKIP(return, "This test needs root to run!");
>   
>   	err = read_patterns();
>   	if (err)
> -		SKIP(exit(-1), "Can't read patterns. Probably, module isn't loaded");
> +		SKIP(return, "Can't read patterns. Probably, module isn't loaded");
>   
>   	card_name = malloc(127);
>   	ASSERT_NE(card_name, NULL);
> diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> index 20294553a5dd..d2cfc9b494a0 100644
> --- a/tools/testing/selftests/mm/hmm-tests.c
> +++ b/tools/testing/selftests/mm/hmm-tests.c
> @@ -138,7 +138,7 @@ FIXTURE_SETUP(hmm)
>   
>   	self->fd = hmm_open(variant->device_number);
>   	if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(return, "DEVICE_COHERENT not available");
>   	ASSERT_GE(self->fd, 0);
>   }
>   
> @@ -149,7 +149,7 @@ FIXTURE_SETUP(hmm2)
>   
>   	self->fd0 = hmm_open(variant->device_number0);
>   	if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> -		SKIP(exit(0), "DEVICE_COHERENT not available");
> +		SKIP(return, "DEVICE_COHERENT not available");
>   	ASSERT_GE(self->fd0, 0);
>   	self->fd1 = hmm_open(variant->device_number1);
>   	ASSERT_GE(self->fd1, 0);

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
- that's totally what mandates vfork() introduced in recent refactor

(Sorry for pointing the same in the Link:-ed thread, it was just
newer/higher in my email client)