On Fri, Jul 14, 2023 at 12:00:51AM +1000, Aleksa Sarai wrote:
On 2023-07-13, Christian Brauner brauner@kernel.org wrote:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 486334981e60..08f0969208eb 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -580,6 +580,10 @@ int run_syscall(int min, int max) CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break; CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break; CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;I'm not a big fan of this, it abuses the nolibc testsuite to test core kernel functionality.
Yes, this should be dropped. We need a minimal patch to fix this. This just makes backporting harder and any test doesn't need to be backported.
Alright, I'll drop it in v2 (though I'm not sure why there are tests for /proc/self and /proc/self/net then).
In fact the goal was to rely on existing entries that were certain to return certain errors, as we are testing nolibc syscalls in limited environments, such as not being able to create a new file due to another syscall not being available yet. /proc is convenient to make a number of syscalls fail. That's how the problem was detected by the way :-)
I personally don't mind that much that tests would be added, provided they really test a new syscall+error combination each. As Thomas said, here we already have other tests for chmod+EPERM so these ones do not bring value here for the purpose of this specific test.
With that in mind, if there is some perceived value in adding such tests, that's something we could discuss, either in this file as another category or (preferably) in a separate one, because the framework makes this easy. We could for example have a "proc-test" sub-project forked from this one to run various tests on /proc file permissions. This would respect a clean split, with nolibc-test assuming a valid kernel to test a libc, and proc-test assuming a valid libc to test the kernel. Just an idea.
Regards, willy