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.