[PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Mark Brown]

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(-)

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,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Ryan Roberts]

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,

Re: [PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Ryan Roberts]

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.

Re: [PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] selftests/mm: Don't needlessly use sudo to obtain root in run_vmtests.sh

[Ryan Roberts]

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.