diff mbox series

[v3,4/5] selftests/mm: protection_keys: save/restore nr_hugepages settings

Message ID 20240125154608.720072-5-usama.anjum@collabora.com (mailing list archive)
State New
Headers show
Series selftests/mm: Improve run_vmtests.sh | expand

Commit Message

Muhammad Usama Anjum Jan. 25, 2024, 3:46 p.m. UTC 
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(+)

Comments

Joey Gouly March 13, 2024, 2:58 p.m. UTC | #1 
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.

Thanks,
Joey
Muhammad Usama Anjum March 13, 2024, 6:12 p.m. UTC | #2 
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
>
Joey Gouly March 14, 2024, 9:31 a.m. UTC | #3 
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 <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, works for me:

Tested-by: Joey Gouly <joey.gouly@arm.com>
diff mbox series

Patch

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