Re: [PATCH 24/27] selftests/landlock: Use ASSERT for syscall failures before errno checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: ASSERT_EQ bypasses ptrace cleanup, causing test hang
--

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

selftests/landlock: Use ASSERT for syscall failures before errno checks

This commit replaces EXPECT* with ASSERT* for syscall failures where
succeeding would result in an undefined errno. This ensures that the
tests abort early rather than checking undefined state.

> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index 75f6d0dcd9375..2266e2361d923 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> @@ -204,7 +204,7 @@ TEST_F(scoped_domains, trace)
>  		if (can_trace_parent) {
>  			EXPECT_EQ(0, ret);
>  		} else {
> -			EXPECT_EQ(-1, ret);
> +			ASSERT_EQ(-1, ret);

[Severity: Medium]
Will this new ASSERT_EQ bypass the ptrace cleanup on unexpected test failures?

If ptrace(PTRACE_ATTACH) unexpectedly succeeds when it is expected to fail,
it sends a SIGSTOP to the tracee.

With the change from EXPECT_EQ(-1, ret) to ASSERT_EQ(-1, ret), an unexpected
success where ret == 0 causes the test process to abort immediately.

Does this early abort bypass the subsequent if (ret == 0) block that performs
waitpid() and ptrace(PTRACE_DETACH) to clean up the stopped tracee?

Because the tracer dies without detaching or waking the tracee, the tracee
remains in a stopped state (SIGSTOP).

If the tracee is the parent test process, could the kselftest harness's
waitpid() block indefinitely, causing the entire test suite to hang instead
of cleanly reporting a failure?

>  			EXPECT_EQ(EPERM, errno);
>  		}
>  		if (ret == 0) {
>  			ASSERT_EQ(parent, waitpid(parent, &status, 0));
>  			ASSERT_EQ(1, WIFSTOPPED(status));
>  			ASSERT_EQ(0, ptrace(PTRACE_DETACH, parent, NULL, 0));
>  		}

[ ... ]

> @@ -276,7 +276,7 @@ TEST_F(scoped_domains, trace)
>  	if (can_trace_child) {
>  		EXPECT_EQ(0, ret);
>  	} else {
> -		EXPECT_EQ(-1, ret);
> +		ASSERT_EQ(-1, ret);

[Severity: Medium]
Similarly, if the parent's PTRACE_ATTACH on the child unexpectedly succeeds,
would the parent abort and leave the child stopped and orphaned?

>  		EXPECT_EQ(EPERM, errno);
>  	}
>  
>  	if (ret == 0) {
>  		ASSERT_EQ(child, waitpid(child, &status, 0));
>  		ASSERT_EQ(1, WIFSTOPPED(status));
>  		ASSERT_EQ(0, ptrace(PTRACE_DETACH, child, NULL, 0));
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008142604.39107-2-gnoack3000@gmail.com?part=24