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@collabora.com

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

On 01/02/2024 12:24, Muhammad Usama Anjum wrote:

On 2/1/24 5:04 PM, Ryan Roberts wrote:

...

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@collabora.com

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");
  

I'd sent a patch to fix this behavior today. This test should run without root privileges. https://lore.kernel.org/all/20240201071307.592317-1-usama.anjum@collabora.co...

...

...

• 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.

run_vmtests.sh is invoked without sudo with following: make -C tools/testing/selftests/mm run_tests

Unfortunately, I live in a world where my build machine isn't always the same as the target machine, so I'm not too familiar with this method of invocation.

Regardless, the vast majority of the tests in run_vmtests.sh (as well as the configuration code in the script itself) require root. So invoking run_vmtests.sh as anything other than root is a BadIdea (TM). And when run_vmtests.sh is running as root, then you need the "sudo -u nobody" to deprivilege this particular test.

Installing sudo in rootfs wouldn't be trivial enough on the CI side. Alternatively, we can check if sudo is present before executing this test to avoid error that sudo isn't found.

Yeah, that's probably the easiest solution; just skip it if the requirements are not met.

...

...

CATEGORY="mmap" run_test ./map_populate

On 3/13/24 7:58 PM, Joey Gouly wrote:

Hi Muhammad,

On Thu, Jan 25, 2024 at 08:46:07PM +0500, Muhammad Usama Anjum wrote:

...

Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.

Fixes: 5f23f6d082a9 ("x86/pkeys: Add self-tests") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

---

tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 48dc151f8fca8..f822ae31af22e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,7 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal; char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE]; +char buf[256]; void cat_into_file(char *str, char *file) { @@ -1744,6 +1745,38 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); } +void restore_settings_atexit(void) +{

• cat_into_file(buf, "/proc/sys/vm/nr_hugepages");

+}

+void save_settings(void) +{

• int fd;
• int err;
• if (geteuid())

• return;
  

• fd = open("/proc/sys/vm/nr_hugepages", O_RDONLY);
• if (fd < 0) {

• fprintf(stderr, "error opening\n");
  

• perror("error: ");
  

• exit(__LINE__);
  

• }
• /* -1 to guarantee leaving the trailing \0 */
• err = read(fd, buf, sizeof(buf)-1);
• if (err < 0) {

• fprintf(stderr, "error reading\n");
  

• perror("error: ");
  

• exit(__LINE__);
  

• }
• atexit(restore_settings_atexit);
• close(fd);

+}

int main(void) { int nr_iterations = 22; @@ -1751,6 +1784,7 @@ int main(void) srand((unsigned int)time(NULL));

• save_settings(); setup_handlers();

printf("has pkeys: %d\n", pkeys_supported); -- 2.42.0

This break the tests for me:

assert() at protection_keys.c::812 test_nr: 19 iteration: 1 running abort_hooks()...

This is because some of the tests fork, so on their atexit() they will set the nr_hugepages back to the previous setting. Specifically the test_pkey_alloc_exhaust() test.

Thank you for reporting. Please can you test the following patch:

--- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -1745,9 +1745,12 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); }

+pid_t parent_pid; + void restore_settings_atexit(void) { - cat_into_file(buf, "/proc/sys/vm/nr_hugepages"); + if (parent_pid == getpid()) + cat_into_file(buf, "/proc/sys/vm/nr_hugepages"); }

void save_settings(void) @@ -1773,6 +1776,7 @@ void save_settings(void) exit(__LINE__); }

+ parent_pid = getpid(); atexit(restore_settings_atexit); close(fd); }

Thanks, Joey

On 2/1/24 5:11 PM, Ryan Roberts wrote:

On 25/01/2024 15:46, Muhammad Usama Anjum wrote:

...

Add missing tests to run_vmtests.sh. The mm kselftests are run through run_vmtests.sh. If a test isn't present in this script, it'll not run with run_tests or `make -C tools/testing/selftests/mm run_tests`.

Cc: Ryan Roberts ryan.roberts@arm.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

---

Changes since v1:

• Copy the original scripts and their dependence script to install directory as well

Changes since v2:

• Add a comment
• Move tests down in the file
• Add "-d" option which poisons the pages and aren't being useable after the test

---

tools/testing/selftests/mm/Makefile | 5 +++++ tools/testing/selftests/mm/run_vmtests.sh | 8 ++++++++ 2 files changed, 13 insertions(+)

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 2453add65d12f..f3aec7be80730 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -114,6 +114,11 @@ TEST_PROGS := run_vmtests.sh TEST_FILES := test_vmalloc.sh TEST_FILES += test_hmm.sh TEST_FILES += va_high_addr_switch.sh +TEST_FILES += charge_reserved_hugetlb.sh +TEST_FILES += hugetlb_reparenting_test.sh

+# required by charge_reserved_hugetlb.sh +TEST_FILES += write_hugetlb_memory.sh include ../lib.mk diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index e373d592dbf5c..a0f37e4438937 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -19,6 +19,7 @@ usage: ${BASH_SOURCE[0]:-$0} [ options ] -t: specify specific categories to tests to run -h: display this message -n: disable TAP output

• -d: run destructive tests

You probably want to clarify the behaviour for -a (all). I guess providing -a should NOT run destructive tests unless -d is also explicitly provided.

...

The default behavior is to run required tests only. If -a is specified, will run all tests. @@ -79,6 +80,7 @@ EOF } RUN_ALL=false +RUN_DESTRUCTIVE_TEST=false

Either call this RUN_DESTRUCTIVE (my preference) or at least make it plural (RUN_DESTRUCTIVE_TESTS).

...

TAP_PREFIX="# " while getopts "aht:n" OPT; do @@ -87,6 +89,7 @@ while getopts "aht:n" OPT; do "h") usage ;; "t") VM_SELFTEST_ITEMS=${OPTARG} ;; "n") TAP_PREFIX= ;;

• "a") RUN_DESTRUCTIVE_TEST=true ;;
  

The help you added says the option is -d, but this is looking for -a, and conflicting with the existing -a=all option.

Sorry, that's a typo. I'll resolve your above comments with fix patch as well.

...

esac done shift $((OPTIND -1)) @@ -304,6 +307,11 @@ CATEGORY="process_mrelease" run_test ./mrelease_test CATEGORY="mremap" run_test ./mremap_test CATEGORY="hugetlb" run_test ./thuge-gen +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2 +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2 +if $RUN_DESTRUCTIVE_TEST; then +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison +fi if [ $VADDR64 -ne 0 ]; then