| Message ID | 20220704173351.19595-1-adam@wowsignal.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] selftests/vm: fix errno handling in mrelease_test | expand |
On Mon, Jul 04, 2022 at 07:33:51PM +0200, Adam Sindelar wrote: Thanks for fixing this, Adam. > mrelease_test should return KSFT_SKIP when process_mrelease is not > defined, but due to a perror call consuming the errno, it returns > KSFT_FAIL. > > This patch decides the exit code before calling perror. > We should probably also include a "Fixes" line here (see [0]): Fixes: 33776141b812 ("selftests: vm: add process_mrelease tests") [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > Signed-off-by: Adam Sindelar <adam@wowsignal.io> > --- > v1->v2: Fixed second instance in the same file > > tools/testing/selftests/vm/mrelease_test.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c > index 96671c2f7d48..e8b17258579b 100644 > --- a/tools/testing/selftests/vm/mrelease_test.c > +++ b/tools/testing/selftests/vm/mrelease_test.c > @@ -100,8 +100,10 @@ int main(void) > > /* Test a wrong pidfd */ > if (!syscall(__NR_process_mrelease, -1, 0) || errno != EBADF) { > + /* perror overwrites errno, so this line must be first */ > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > perror("process_mrelease with wrong pidfd"); > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > + exit(res); > } > > /* Start the test with 1MB child memory allocation */ > @@ -156,8 +158,9 @@ int main(void) > run_negative_tests(pidfd); > > if (kill(pid, SIGKILL)) { > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > perror("kill"); > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > + exit(res); > } > > success = (syscall(__NR_process_mrelease, pidfd, 0) == 0); > -- > 2.35.1 > This looks good to me, but it looks like there are a couple of other places where we're doing the wrong thing, i.e. in run_negative_tests() and after calling waitpid(). Could you please fix those as well? Also adding Suren to cc. - David
On Tue, Jul 5, 2022 at 9:41 AM David Vernet <void@manifault.com> wrote: > > On Mon, Jul 04, 2022 at 07:33:51PM +0200, Adam Sindelar wrote: > > Thanks for fixing this, Adam. > > > mrelease_test should return KSFT_SKIP when process_mrelease is not > > defined, but due to a perror call consuming the errno, it returns > > KSFT_FAIL. > > > > This patch decides the exit code before calling perror. > > > > We should probably also include a "Fixes" line here (see [0]): > > Fixes: 33776141b812 ("selftests: vm: add process_mrelease tests") > > [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > > > Signed-off-by: Adam Sindelar <adam@wowsignal.io> > > --- > > v1->v2: Fixed second instance in the same file > > > > tools/testing/selftests/vm/mrelease_test.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c > > index 96671c2f7d48..e8b17258579b 100644 > > --- a/tools/testing/selftests/vm/mrelease_test.c > > +++ b/tools/testing/selftests/vm/mrelease_test.c > > @@ -100,8 +100,10 @@ int main(void) > > > > /* Test a wrong pidfd */ > > if (!syscall(__NR_process_mrelease, -1, 0) || errno != EBADF) { > > + /* perror overwrites errno, so this line must be first */ > > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > perror("process_mrelease with wrong pidfd"); > > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > + exit(res); > > } > > > > /* Start the test with 1MB child memory allocation */ > > @@ -156,8 +158,9 @@ int main(void) > > run_negative_tests(pidfd); > > > > if (kill(pid, SIGKILL)) { > > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > perror("kill"); > > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > + exit(res); > > } > > > > success = (syscall(__NR_process_mrelease, pidfd, 0) == 0); > > -- > > 2.35.1 > > > > This looks good to me, but it looks like there are a couple of other places > where we're doing the wrong thing, i.e. in run_negative_tests() and after > calling waitpid(). Could you please fix those as well? > > Also adding Suren to cc. Thanks for adding me, David! The fixes look good but there are 5 places in all this fix is needed: https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L68 https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L76 https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L103 https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L159 https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L175 Thanks for catching this, Adam! Suren. > > - David
On Tue, Jul 05, 2022 at 10:02:08AM -0700, Suren Baghdasaryan wrote: > On Tue, Jul 5, 2022 at 9:41 AM David Vernet <void@manifault.com> wrote: > > > > On Mon, Jul 04, 2022 at 07:33:51PM +0200, Adam Sindelar wrote: > > > > Thanks for fixing this, Adam. > > > > > mrelease_test should return KSFT_SKIP when process_mrelease is not > > > defined, but due to a perror call consuming the errno, it returns > > > KSFT_FAIL. > > > > > > This patch decides the exit code before calling perror. > > > > > > > We should probably also include a "Fixes" line here (see [0]): > > > > Fixes: 33776141b812 ("selftests: vm: add process_mrelease tests") > > > > [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > > > > > Signed-off-by: Adam Sindelar <adam@wowsignal.io> > > > --- > > > v1->v2: Fixed second instance in the same file > > > > > > tools/testing/selftests/vm/mrelease_test.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c > > > index 96671c2f7d48..e8b17258579b 100644 > > > --- a/tools/testing/selftests/vm/mrelease_test.c > > > +++ b/tools/testing/selftests/vm/mrelease_test.c > > > @@ -100,8 +100,10 @@ int main(void) > > > > > > /* Test a wrong pidfd */ > > > if (!syscall(__NR_process_mrelease, -1, 0) || errno != EBADF) { > > > + /* perror overwrites errno, so this line must be first */ > > > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > > perror("process_mrelease with wrong pidfd"); > > > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > > + exit(res); > > > } > > > > > > /* Start the test with 1MB child memory allocation */ > > > @@ -156,8 +158,9 @@ int main(void) > > > run_negative_tests(pidfd); > > > > > > if (kill(pid, SIGKILL)) { > > > + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > > perror("kill"); > > > - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); > > > + exit(res); > > > } > > > > > > success = (syscall(__NR_process_mrelease, pidfd, 0) == 0); > > > -- > > > 2.35.1 > > > > > > > This looks good to me, but it looks like there are a couple of other places > > where we're doing the wrong thing, i.e. in run_negative_tests() and after > > calling waitpid(). Could you please fix those as well? > > > > Also adding Suren to cc. > > Thanks for adding me, David! > > The fixes look good but there are 5 places in all this fix is needed: > https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L68 > https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L76 > https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L103 > https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L159 > https://elixir.bootlin.com/linux/v5.19-rc5/source/tools/testing/selftests/vm/mrelease_test.c#L175 > Thanks for catching this, Adam! > Suren. > > > > > - David Ah, well spotted. I've fixed the remaining instances in v3. -Adam
diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c index 96671c2f7d48..e8b17258579b 100644 --- a/tools/testing/selftests/vm/mrelease_test.c +++ b/tools/testing/selftests/vm/mrelease_test.c @@ -100,8 +100,10 @@ int main(void) /* Test a wrong pidfd */ if (!syscall(__NR_process_mrelease, -1, 0) || errno != EBADF) { + /* perror overwrites errno, so this line must be first */ + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); perror("process_mrelease with wrong pidfd"); - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); + exit(res); } /* Start the test with 1MB child memory allocation */ @@ -156,8 +158,9 @@ int main(void) run_negative_tests(pidfd); if (kill(pid, SIGKILL)) { + res = (errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); perror("kill"); - exit(errno == ENOSYS ? KSFT_SKIP : KSFT_FAIL); + exit(res); } success = (syscall(__NR_process_mrelease, pidfd, 0) == 0);
mrelease_test should return KSFT_SKIP when process_mrelease is not defined, but due to a perror call consuming the errno, it returns KSFT_FAIL. This patch decides the exit code before calling perror. Signed-off-by: Adam Sindelar <adam@wowsignal.io> --- v1->v2: Fixed second instance in the same file tools/testing/selftests/vm/mrelease_test.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)