Re: [PATCH -next] selftests/ptrace: Fix Test terminated by timeout in ptrace_attach

From: Bernd Edlinger <bernd.edlinger@hotmail.de>
To: limin <limin100@huawei.com>,
	shuah@kernel.org, keescook@chromium.org, ebiederm@xmission.com,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: songmuchun@bytedance.com, lizefan.x@bytedance.com
Subject: Re: [PATCH -next] selftests/ptrace: Fix Test terminated by timeout in ptrace_attach
Date: Mon, 28 Nov 2022 08:56:09 +0100	[thread overview]
Message-ID: <AS8P193MB1285BCEDA342A074F6A837FDE4139@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20221128070454.1850273-1-limin100@huawei.com>

Hi,

thanks for cleaning this up.

Just for completenes:

I have actually two patches submitted a while ago, but did not get any response so far,
one that would make the test case work as it is:

[PATCH v10] exec: Fix dead-lock in de_thread with ptrace_attach
https://lore.kernel.org/lkml/AM8PR10MB470801D01A0CF24BC32C25E7E40E9@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM/

and my favorite one, that would fix the dead-lock altogether (and adjust the test case accordingly):

[PATCH v11] exec: Fix dead-lock in de_thread with ptrace_attach
https://lore.kernel.org/lkml/AM8PR10MB470875B22B4C08BEAEC3F77FE4169@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM/

Note: I got actually one response from an automatic checker to the v11 patch,
(that I just discovered today, when I searched in my in-box:)

https://lore.kernel.org/lkml/202107121344.wu68hEPF-lkp@intel.com/

which is complaining about:

>> kernel/ptrace.c:425:26: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct cred const *old_cred @@     got struct cred const [noderef] __rcu *real_cred @@

   417			struct linux_binprm *bprm = task->signal->exec_bprm;
   418			const struct cred *old_cred;
   419			struct mm_struct *old_mm;
   420	
   421			retval = down_write_killable(&task->signal->exec_update_lock);
   422			if (retval)
   423				goto unlock_creds;
   424			task_lock(task);
 > 425			old_cred = task->real_cred;

probably no big issue, I guess I would just have to add a __rcu to the declaration of old_cred.
But similar warnings appear to be everywhere.

Thanks
Bernd.

On 11/28/22 8:04 AM, limin wrote:
> That is an open issue
> Bernd Edlinger wrote the test case in anticipation that all of
> patch series got accepted,but the last patch was not picked up
> for inclusion in the linux kernel.
> How to reproduce warning:
> $ make -C tools/testing/selftests TARGETS=ptrace run_tests
> Example vmaccess from 6.1.0-next source tree run fail on bare metal
>   RUN global.attach ...
>   attach: Test terminated by timeout
>   FAIL  global.attach
> 
> Link:https://lore.kernel.org/all/AM8PR10MB4708E6FF0E155261455064C2E4209@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM/
> Fixes: 2de4e82318c7 ("selftests/ptrace: add test cases for dead-locks")
> Signed-off-by: limin <limin100@huawei.com>
> ---
>  tools/testing/selftests/ptrace/vmaccess.c | 37 ++++++++---------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c
> index 4db327b44586..751a41f1163c 100644
> --- a/tools/testing/selftests/ptrace/vmaccess.c
> +++ b/tools/testing/selftests/ptrace/vmaccess.c
> @@ -45,42 +45,31 @@ TEST(vmaccess)
>  
>  TEST(attach)
>  {
> -	int s, k, pid = fork();
> +	int k;
> +	int s;
>  
> +	pid_t pid = fork();
>  	if (!pid) {
> -		pthread_t pt;
> -
> -		pthread_create(&pt, NULL, thread, NULL);
> -		pthread_join(pt, NULL);
> +		ptrace(PTRACE_TRACEME, 0, NULL, NULL);
>  		execlp("sleep", "sleep", "2", NULL);
>  	}
>  
>  	sleep(1);
>  	k = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
> -	ASSERT_EQ(errno, EAGAIN);
> +	printf("k1:%d\n", k);
> +	ASSERT_EQ(k, -1);
> +	waitpid(pid, &s, WNOHANG);
>  	ASSERT_EQ(k, -1);
> -	k = waitpid(-1, &s, WNOHANG);
> -	ASSERT_NE(k, -1);
>  	ASSERT_NE(k, 0);
>  	ASSERT_NE(k, pid);
> -	ASSERT_EQ(WIFEXITED(s), 1);
> -	ASSERT_EQ(WEXITSTATUS(s), 0);
> -	sleep(1);
> -	k = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
> -	ASSERT_EQ(k, 0);
> -	k = waitpid(-1, &s, 0);
> -	ASSERT_EQ(k, pid);
> +	if (WIFEXITED(s))
> +		ASSERT_EQ(WEXITSTATUS(s), 0);
> +	if (WIFSTOPPED(s))
> +		ASSERT_EQ(WSTOPSIG(s), SIGTRAP);
>  	ASSERT_EQ(WIFSTOPPED(s), 1);
> -	ASSERT_EQ(WSTOPSIG(s), SIGSTOP);
> -	k = ptrace(PTRACE_DETACH, pid, 0L, 0L);
> -	ASSERT_EQ(k, 0);
> -	k = waitpid(-1, &s, 0);
> -	ASSERT_EQ(k, pid);
> -	ASSERT_EQ(WIFEXITED(s), 1);
> -	ASSERT_EQ(WEXITSTATUS(s), 0);
> -	k = waitpid(-1, NULL, 0);
> +	sleep(1);
> +	ptrace(PTRACE_CONT, pid, NULL, NULL);
>  	ASSERT_EQ(k, -1);
> -	ASSERT_EQ(errno, ECHILD);
>  }
>  
>  TEST_HARNESS_MAIN
> 