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

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

[limin]

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
-- 
2.33.0

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

[Bernd Edlinger]

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
> 

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

[Kees Cook]

On Mon, Nov 28, 2022 at 08:56:09AM +0100, Bernd Edlinger wrote:
> thanks for cleaning this up.

Oh, hm, I never saw the original email -- I'll check my Spam folder, it
gets overly excited sometimes.

> 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/

This fell off my radar, but let's look at it again. Is this still an
existing race after Eric's various refactorings? I assume so. Eric, can
you looked at this case?

-- 
Kees Cook

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

[Zheng Yejian]

On Thu, 1 Dec 2022 16:48:20 -0800, Kees Cook wrote:
> On Mon, Nov 28, 2022 at 08:56:09AM +0100, Bernd Edlinger wrote:
> > thanks for cleaning this up.
>
> Oh, hm, I never saw the original email -- I'll check my Spam folder, it
> gets overly excited sometimes.
>
> > 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/
>
> This fell off my radar, but let's look at it again. Is this still an
> existing race after Eric's various refactorings? I assume so. Eric, can
> you looked at this case?
>

Kindly ping :)

This discussion suspended for half a year, but testcase 'tools/testing/selftests/ptrace/vmaccess.c'
still can not pass on the newest v6.4 kernel.

Would you continue to work it out?

:)

---

Thanks,
Zheng Yejian

> --
> Kees Cook
>