Re: [PATCH v3 2/5] selftests/mm: run_vmtests: remove sudo and conform to tap

[Ryan Roberts]

On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
> Remove sudo as some test running environments may not have sudo
> available. Instead skip the test if root privileges aren't available in
> the test.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
> ---
> Changes since v1:
> - Added this patch in v2
> 
> We are allocating 2*RLIMIT_MEMLOCK.rlim_max memory and mmap() isn't
> failing. This seems like true bug in the kernel. Even the root user
> shouldn't be able to allocate more memory than allowed MEMLOCKed memory.
> Any ideas?
> ---
>  tools/testing/selftests/mm/on-fault-limit.c | 36 ++++++++++-----------
>  tools/testing/selftests/mm/run_vmtests.sh   |  2 +-
>  2 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/on-fault-limit.c b/tools/testing/selftests/mm/on-fault-limit.c
> index b5888d613f34e..0ea98ffab3589 100644
> --- a/tools/testing/selftests/mm/on-fault-limit.c
> +++ b/tools/testing/selftests/mm/on-fault-limit.c
> @@ -5,40 +5,38 @@
>  #include <string.h>
>  #include <sys/time.h>
>  #include <sys/resource.h>
> +#include "../kselftest.h"
>  
> -static int test_limit(void)
> +static void test_limit(void)
>  {
> -	int ret = 1;
>  	struct rlimit lims;
>  	void *map;
>  
> -	if (getrlimit(RLIMIT_MEMLOCK, &lims)) {
> -		perror("getrlimit");
> -		return ret;
> -	}
> +	if (getrlimit(RLIMIT_MEMLOCK, &lims))
> +		ksft_exit_fail_msg("getrlimit: %s\n", strerror(errno));
>  
> -	if (mlockall(MCL_ONFAULT | MCL_FUTURE)) {
> -		perror("mlockall");
> -		return ret;
> -	}
> +	if (mlockall(MCL_ONFAULT | MCL_FUTURE))
> +		ksft_exit_fail_msg("mlockall: %s\n", strerror(errno));
>  
>  	map = mmap(NULL, 2 * lims.rlim_max, PROT_READ | PROT_WRITE,
>  		   MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE, -1, 0);
> +
> +	ksft_test_result(map == MAP_FAILED, "Failed mmap\n");
> +
>  	if (map != MAP_FAILED)
> -		printf("mmap should have failed, but didn't\n");
> -	else {
> -		ret = 0;
>  		munmap(map, 2 * lims.rlim_max);
> -	}
> -
>  	munlockall();
> -	return ret;
>  }
>  
>  int main(int argc, char **argv)
>  {
> -	int ret = 0;
> +	ksft_print_header();
> +	ksft_set_plan(1);
> +
> +	if (getuid())
> +		ksft_test_result_skip("Require root privileges to run\n");
> +	else
> +		test_limit();
>  
> -	ret += test_limit();
> -	return ret;
> +	ksft_finished();
>  }
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index 246d53a5d7f28..e373d592dbf5c 100755
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -291,7 +291,7 @@ echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
>  
>  CATEGORY="compaction" run_test ./compaction_test
>  
> -CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit
> +CATEGORY="mlock" run_test ./on-fault-limit

I think changing this is going to give unintended results. run_vmtests.sh must
already be running as root. "sudo -u nobody" is deprivieging the test to run as
nobody. The rlimit is not enforced for root so this test must run unprivileged
to work. See man page for getrlimit():

  Since Linux 2.6.9, no limits are placed on the amount of memory that a
  privileged process may lock, and this limit instead governs the amount of
  memory that an unprivileged process may lock

So I think the correct fix is actually to install sudo on your CI.

>  
>  CATEGORY="mmap" run_test ./map_populate
>