In this series, I'm trying to add 3 missing tests to vm_runtests.sh
which is used to run all the tests in mm suite. These tests weren't
running by CIs. While enabling them and through review feedback, I've
fixed some problems in tests as well. I've found more flakiness in more
tests which I'll be fixing with future patches.
hugetlb-read-hwpoison test is being added where it can only run with
newly added "-d" (destructive) flag only. Not sure why it is failing
again. So once it become stable, we can think of moving it to default
set of tests if it doesn't have any side-effect to them.
Cc: Ryan Roberts <[email protected]>
Cc: Andrew Morton <[email protected]>
---
Changes in v3:
- Add cover letter
- Fix flakiness in tests found during enablement
- Move additional tests down in the file
- Add "-d" option which poisons the pages and aren't being useable after
the test
v2: https://lore.kernel.org/all/[email protected]
Muhammad Usama Anjum (5):
selftests/mm: hugetlb_reparenting_test: do not unmount
selftests/mm: run_vmtests: remove sudo and conform to tap
selftests/mm: save and restore nr_hugepages value
selftests/mm: protection_keys: save/restore nr_hugepages settings
selftests/mm: run_vmtests.sh: add missing tests
tools/testing/selftests/mm/Makefile | 5 +++
.../selftests/mm/charge_reserved_hugetlb.sh | 4 +++
.../selftests/mm/hugetlb_reparenting_test.sh | 9 +++--
tools/testing/selftests/mm/on-fault-limit.c | 36 +++++++++----------
tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++
tools/testing/selftests/mm/run_vmtests.sh | 10 +++++-
6 files changed, 76 insertions(+), 22 deletions(-)
--
2.42.0
Do not unmount the cgroup if it wasn't mounted by the test. The earlier
patch had fixed this for charge_reserved_hugetlb, but not for this test.
I'm adding fixes tag to that earlier patch.
Fixes: 209376ed2a84 ("selftests/vm: make charge_reserved_hugetlb.sh work with existing cgroup setting")
Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
index 14d26075c8635..615c4d766c909 100755
--- a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
+++ b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
@@ -248,5 +248,7 @@ cleanup
echo ALL PASS
-umount $CGROUP_ROOT
-rm -rf $CGROUP_ROOT
+if [[ $do_umount ]]; then
+ umount $CGROUP_ROOT
+ rm -rf $CGROUP_ROOT
+fi
--
2.42.0
Save and restore nr_hugepages before changing it during the test. A test
should not change system wide settings.
Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
tools/testing/selftests/mm/charge_reserved_hugetlb.sh | 4 ++++
tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 3 +++
2 files changed, 7 insertions(+)
diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
index e14bdd4455f2d..d680c00d2853a 100755
--- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
+++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
@@ -11,6 +11,8 @@ if [[ $(id -u) -ne 0 ]]; then
exit $ksft_skip
fi
+nr_hugepgs=$(cat /proc/sys/vm/nr_hugepages)
+
fault_limit_file=limit_in_bytes
reservation_limit_file=rsvd.limit_in_bytes
fault_usage_file=usage_in_bytes
@@ -582,3 +584,5 @@ if [[ $do_umount ]]; then
umount $cgroup_path
rmdir $cgroup_path
fi
+
+echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
index 615c4d766c909..11f9bbe7dc222 100755
--- a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
+++ b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
@@ -11,6 +11,7 @@ if [[ $(id -u) -ne 0 ]]; then
exit $ksft_skip
fi
+nr_hugepgs=$(cat /proc/sys/vm/nr_hugepages)
usage_file=usage_in_bytes
if [[ "$1" == "-cgroup-v2" ]]; then
@@ -252,3 +253,5 @@ if [[ $do_umount ]]; then
umount $CGROUP_ROOT
rm -rf $CGROUP_ROOT
fi
+
+echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
--
2.42.0
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 <[email protected]>
Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
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
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
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 ;;
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
--
2.42.0
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 <[email protected]>
---
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
CATEGORY="mmap" run_test ./map_populate
--
2.42.0
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 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 <[email protected]>
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> ---
> 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.
> 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
>
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 <[email protected]>
>> Signed-off-by: Muhammad Usama Anjum <[email protected]>
>> ---
>> 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
>>
>
>
--
BR,
Muhammad Usama Anjum
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 <[email protected]>
>>> ---
>>> 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/[email protected]
>
>>> + 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 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 <[email protected]>
>> ---
>> 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/[email protected]
>> + 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
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.
>
>>
>> CATEGORY="mmap" run_test ./map_populate
>>
>
>
--
BR,
Muhammad Usama Anjum
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 <[email protected]>
> ---
> 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.
Thanks,
Joey
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 <[email protected]>
>> ---
>> 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
>
--
BR,
Muhammad Usama Anjum
On Wed, Mar 13, 2024 at 11:12:58PM +0500, Muhammad Usama Anjum wrote:
> 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 <[email protected]>
> >> ---
> >> 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, works for me:
Tested-by: Joey Gouly <[email protected]>