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 <[email protected]>
---
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
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/[email protected]/
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 <[email protected]>
> ---
> 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
>
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
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
>