| Message ID | 20221128070454.1850273-1-limin100@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] selftests/ptrace: Fix Test terminated by timeout in ptrace_attach | expand |
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 >
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?
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 >
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
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(-)