| Message ID | 20240209-kselftest-mm-check-deps-v1-1-19b09b151522@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh | expand |
On 09/02/2024 20:21, Mark Brown wrote: > When opening yama/ptrace_scope we unconditionally use sudo to ensure we > are running as root, resulting in failures if running in a minimal root > filesystem where sudo is not installed. Since automated test systems will > typically just run all of kselftest as root (and many kselftests rely on > this for full functionality) add a check to see if we're already root and > only invoke sudo if not. I don't really see the point of this. run_vmtests.sh needs to be run as root; there are lots of operations that depend on it and most tests will fail if not root. So I think it would be much cleaner just to remove this instance sudo. The problem that I was referring to yesterday, about needing sudo was for this case: CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit Here, we are using sudo to deprivilege ourselves from root and run on-fault-limit as nobody. This is required because the test is checking an rlimit that is only enforced for normal users. Somebody on list was talking about skipping this test if sudo wasn't present a couple of weeks back. Not sure if that happened. > > Since I am unclear what the intended effect of the command being run is I > have not added any error handling for the case where we fail to obtain > root. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > tools/testing/selftests/mm/run_vmtests.sh | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh > index fe140a9f4f9d..c8ca830dba93 100755 > --- a/tools/testing/selftests/mm/run_vmtests.sh > +++ b/tools/testing/selftests/mm/run_vmtests.sh > @@ -248,6 +248,17 @@ run_test() { > > echo "TAP version 13" | tap_output > > +HAVE_ROOT=0 > +if [ "$(id -u)" = "0" ]; then > + AS_ROOT= > + HAVE_ROOT=1 > +elif [ "$(command -v sudo)" != "" ]; then > + AS_ROOT=sudo > + HAVE_ROOT=1 > +else > + echo # WARNING: Unable to run as root > +fi > + > CATEGORY="hugetlb" run_test ./hugepage-mmap > > shmmax=$(cat /proc/sys/kernel/shmmax) > @@ -363,7 +374,8 @@ CATEGORY="hmm" run_test bash ./test_hmm.sh smoke > # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests > CATEGORY="madv_populate" run_test ./madv_populate > > -(echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix > +# FIXME: What if we can't get root? > +(echo 0 | ${AS_ROOT} tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix > CATEGORY="memfd_secret" run_test ./memfd_secret > > # KSM KSM_MERGE_TIME_HUGE_PAGES test with size of 100 > > --- > base-commit: 445a555e0623387fa9b94e68e61681717e70200a > change-id: 20240209-kselftest-mm-check-deps-01a825e5fed4 > > Best regards,
On Sat, Feb 10, 2024 at 07:40:16AM +0000, Ryan Roberts wrote: > On 09/02/2024 20:21, Mark Brown wrote: > > When opening yama/ptrace_scope we unconditionally use sudo to ensure we > > are running as root, resulting in failures if running in a minimal root > > filesystem where sudo is not installed. Since automated test systems will > > typically just run all of kselftest as root (and many kselftests rely on > > this for full functionality) add a check to see if we're already root and > > only invoke sudo if not. > I don't really see the point of this. run_vmtests.sh needs to be run as root; > there are lots of operations that depend on it and most tests will fail if not > root. So I think it would be much cleaner just to remove this instance sudo. Ah, I was assuming that some of the suite ran usefully as non-root given that the only point of that sudo was to acquire root. If the whole thing needs to be root then we should instead have a check for root at the top of run_vmtests.sh and just skip the whole thing if we aren't root, but then I'm unclear why it's invoking sudo in the first place. > The problem that I was referring to yesterday, about needing sudo was for this case: > CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit > Here, we are using sudo to deprivilege ourselves from root and run > on-fault-limit as nobody. This is required because the test is checking an > rlimit that is only enforced for normal users. > Somebody on list was talking about skipping this test if sudo wasn't present a > couple of weeks back. Not sure if that happened. Yes, there's a check: if command -v sudo &> /dev/null; then CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit else echo "# SKIP ./on-fault-limit" fi
On 10/02/2024 12:35, Mark Brown wrote: > On Sat, Feb 10, 2024 at 07:40:16AM +0000, Ryan Roberts wrote: >> On 09/02/2024 20:21, Mark Brown wrote: > >>> When opening yama/ptrace_scope we unconditionally use sudo to ensure we >>> are running as root, resulting in failures if running in a minimal root >>> filesystem where sudo is not installed. Since automated test systems will >>> typically just run all of kselftest as root (and many kselftests rely on >>> this for full functionality) add a check to see if we're already root and >>> only invoke sudo if not. > >> I don't really see the point of this. run_vmtests.sh needs to be run as root; >> there are lots of operations that depend on it and most tests will fail if not >> root. So I think it would be much cleaner just to remove this instance sudo. > > Ah, I was assuming that some of the suite ran usefully as non-root given > that the only point of that sudo was to acquire root. If the whole > thing needs to be root then we should instead have a check for root at > the top of run_vmtests.sh and just skip the whole thing if we aren't > root, but then I'm unclear why it's invoking sudo in the first place. I can't speak for how others use the suite, but there are a bunch of setup operations in the script itself that require root (e.g. reserving huge pages). Some of the tests will work without root, I'm sure, but I'm not sure its hugely valuable. Personally, I'd vote for just doing a test for root at the top, as you suggest. > >> The problem that I was referring to yesterday, about needing sudo was for this case: > >> CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit > >> Here, we are using sudo to deprivilege ourselves from root and run >> on-fault-limit as nobody. This is required because the test is checking an >> rlimit that is only enforced for normal users. > >> Somebody on list was talking about skipping this test if sudo wasn't present a >> couple of weeks back. Not sure if that happened. > > Yes, there's a check: > > if command -v sudo &> /dev/null; > then > CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit > else > echo "# SKIP ./on-fault-limit" > fi Ahh that's obviously been added in the last week. The version of mm-unstable I'm looking at doesn't have that. Although the skip message could do with being TAP-compliant.
On Mon, Feb 12, 2024 at 08:32:58AM +0000, Ryan Roberts wrote: > On 10/02/2024 12:35, Mark Brown wrote: > > Ah, I was assuming that some of the suite ran usefully as non-root given > > that the only point of that sudo was to acquire root. If the whole > > thing needs to be root then we should instead have a check for root at > > the top of run_vmtests.sh and just skip the whole thing if we aren't > > root, but then I'm unclear why it's invoking sudo in the first place. > I can't speak for how others use the suite, but there are a bunch of setup > operations in the script itself that require root (e.g. reserving huge pages). > Some of the tests will work without root, I'm sure, but I'm not sure its hugely > valuable. Personally, I'd vote for just doing a test for root at the top, as you > suggest. The hugetlb tests appear to be checking for root while running... I'm not super fussed either way myself, I don't really use these tests myself except in a general "keeping an eye on CI" kind of way so I'd not object if people wanted to just go for just requiring root for the whole thing.
On 12/02/2024 19:13, Mark Brown wrote: > On Mon, Feb 12, 2024 at 08:32:58AM +0000, Ryan Roberts wrote: >> On 10/02/2024 12:35, Mark Brown wrote: > >>> Ah, I was assuming that some of the suite ran usefully as non-root given >>> that the only point of that sudo was to acquire root. If the whole >>> thing needs to be root then we should instead have a check for root at >>> the top of run_vmtests.sh and just skip the whole thing if we aren't >>> root, but then I'm unclear why it's invoking sudo in the first place. > >> I can't speak for how others use the suite, but there are a bunch of setup >> operations in the script itself that require root (e.g. reserving huge pages). >> Some of the tests will work without root, I'm sure, but I'm not sure its hugely >> valuable. Personally, I'd vote for just doing a test for root at the top, as you >> suggest. > > The hugetlb tests appear to be checking for root while running... I'm > not super fussed either way myself, I don't really use these tests > myself except in a general "keeping an eye on CI" kind of way so I'd not > object if people wanted to just go for just requiring root for the whole > thing. My vote is to keep it simple and require root for the whole thing.
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index fe140a9f4f9d..c8ca830dba93 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -248,6 +248,17 @@ run_test() { echo "TAP version 13" | tap_output +HAVE_ROOT=0 +if [ "$(id -u)" = "0" ]; then + AS_ROOT= + HAVE_ROOT=1 +elif [ "$(command -v sudo)" != "" ]; then + AS_ROOT=sudo + HAVE_ROOT=1 +else + echo # WARNING: Unable to run as root +fi + CATEGORY="hugetlb" run_test ./hugepage-mmap shmmax=$(cat /proc/sys/kernel/shmmax) @@ -363,7 +374,8 @@ CATEGORY="hmm" run_test bash ./test_hmm.sh smoke # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests CATEGORY="madv_populate" run_test ./madv_populate -(echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix +# FIXME: What if we can't get root? +(echo 0 | ${AS_ROOT} tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix CATEGORY="memfd_secret" run_test ./memfd_secret # KSM KSM_MERGE_TIME_HUGE_PAGES test with size of 100
When opening yama/ptrace_scope we unconditionally use sudo to ensure we are running as root, resulting in failures if running in a minimal root filesystem where sudo is not installed. Since automated test systems will typically just run all of kselftest as root (and many kselftests rely on this for full functionality) add a check to see if we're already root and only invoke sudo if not. Since I am unclear what the intended effect of the command being run is I have not added any error handling for the case where we fail to obtain root. Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/mm/run_vmtests.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) --- base-commit: 445a555e0623387fa9b94e68e61681717e70200a change-id: 20240209-kselftest-mm-check-deps-01a825e5fed4 Best regards,