[PATCH v1 0/2] mm/ksm: add fork-exec support for prctl

[PATCH v1 0/2] mm/ksm: add fork-exec support for prctl

[Stefan Roesch]

A process can enable KSM with the prctl system call. When the process is
forked the KSM flag is inherited by the child process. However if the
process is executing an exec system call directly after the fork, the
KSM setting is cleared. This patch series addresses this problem.

1) Change the mask in coredump.h for execing a new process
2) Add a new test case in ksm_functional_tests



Stefan Roesch (2):
  mm/ksm: support fork/exec for prctl
  mm/ksm: Test case for prctl fork/exec workflow

 include/linux/sched/coredump.h                |  7 ++-
 tools/testing/selftests/mm/Makefile           |  2 +
 .../selftests/mm/ksm_fork_exec_child.c        |  9 ++++
 .../selftests/mm/ksm_functional_tests.c       | 50 ++++++++++++++++++-
 4 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/mm/ksm_fork_exec_child.c


base-commit: 15bcc9730fcd7526a3b92eff105d6701767a53bb
-- 
2.39.3

[PATCH v1 1/2] mm/ksm: support fork/exec for prctl

[Stefan Roesch]

A process can enable KSM with the prctl system call. When the process is
forked the KSM flag is inherited by the child process. However if the
process is executing an exec system call directly after the fork, the
KSM setting is cleared. This patch addresses this problem.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
Reported-by: Carl Klemm <carl@uvos.xyz>
---
 include/linux/sched/coredump.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
index 0ee96ea7a0e9..205aa9917394 100644
--- a/include/linux/sched/coredump.h
+++ b/include/linux/sched/coredump.h
@@ -87,8 +87,11 @@ static inline int get_dumpable(struct mm_struct *mm)
 
 #define MMF_DISABLE_THP_MASK	(1 << MMF_DISABLE_THP)
 
+#define MMF_VM_MERGE_ANY	29
+#define MMF_VM_MERGE_ANY_MASK	(1 << MMF_VM_MERGE_ANY)
+
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK |\
-				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK)
+				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK |\
+				 MMF_VM_MERGE_ANY_MASK)
 
-#define MMF_VM_MERGE_ANY	29
 #endif /* _LINUX_SCHED_COREDUMP_H */
-- 
2.39.3

Re: [PATCH v1 1/2] mm/ksm: support fork/exec for prctl

[Carl Klemm]

On Tue, 2023-09-19 at 13:51 -0700, Stefan Roesch wrote:
> A process can enable KSM with the prctl system call. When the process
> is
> forked the KSM flag is inherited by the child process. However if the
> process is executing an exec system call directly after the fork, the
> KSM setting is cleared. This patch addresses this problem.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> Reported-by: Carl Klemm <carl@uvos.xyz>
> ---
>  include/linux/sched/coredump.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched/coredump.h
> b/include/linux/sched/coredump.h
> index 0ee96ea7a0e9..205aa9917394 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -87,8 +87,11 @@ static inline int get_dumpable(struct mm_struct
> *mm)
>  
>  #define MMF_DISABLE_THP_MASK   (1 << MMF_DISABLE_THP)
>  
> +#define MMF_VM_MERGE_ANY       29
> +#define MMF_VM_MERGE_ANY_MASK  (1 << MMF_VM_MERGE_ANY)
> +
>  #define MMF_INIT_MASK          (MMF_DUMPABLE_MASK |
> MMF_DUMP_FILTER_MASK |\
> -                                MMF_DISABLE_THP_MASK |
> MMF_HAS_MDWE_MASK)
> +                                MMF_DISABLE_THP_MASK |
> MMF_HAS_MDWE_MASK |\
> +                                MMF_VM_MERGE_ANY_MASK)
>  
> -#define MMF_VM_MERGE_ANY       29
>  #endif /* _LINUX_SCHED_COREDUMP_H */

I can confirm that this patch works as intended for my usecase, you can
have my:

Tested-by: Carl Klemm <carl@uvos.xyz>

Re: [PATCH v1 1/2] mm/ksm: support fork/exec for prctl

[David Hildenbrand]

On 19.09.23 22:51, Stefan Roesch wrote:
> A process can enable KSM with the prctl system call. When the process is
> forked the KSM flag is inherited by the child process. However if the
> process is executing an exec system call directly after the fork, the
> KSM setting is cleared. This patch addresses this problem.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> Reported-by: Carl Klemm <carl@uvos.xyz>
> ---
>   include/linux/sched/coredump.h | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 0ee96ea7a0e9..205aa9917394 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -87,8 +87,11 @@ static inline int get_dumpable(struct mm_struct *mm)
>   
>   #define MMF_DISABLE_THP_MASK	(1 << MMF_DISABLE_THP)
>   
> +#define MMF_VM_MERGE_ANY	29
> +#define MMF_VM_MERGE_ANY_MASK	(1 << MMF_VM_MERGE_ANY)
> +
>   #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK |\
> -				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK)
> +				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK |\
> +				 MMF_VM_MERGE_ANY_MASK)
>   
> -#define MMF_VM_MERGE_ANY	29
>   #endif /* _LINUX_SCHED_COREDUMP_H */

Fixes: tag?

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm/ksm: support fork/exec for prctl

[Stefan Roesch]

David Hildenbrand <david@redhat.com> writes:

> On 19.09.23 22:51, Stefan Roesch wrote:
>> A process can enable KSM with the prctl system call. When the process is
>> forked the KSM flag is inherited by the child process. However if the
>> process is executing an exec system call directly after the fork, the
>> KSM setting is cleared. This patch addresses this problem.
>> Signed-off-by: Stefan Roesch <shr@devkernel.io>
>> Reported-by: Carl Klemm <carl@uvos.xyz>
>> ---
>>   include/linux/sched/coredump.h | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
>> index 0ee96ea7a0e9..205aa9917394 100644
>> --- a/include/linux/sched/coredump.h
>> +++ b/include/linux/sched/coredump.h
>> @@ -87,8 +87,11 @@ static inline int get_dumpable(struct mm_struct *mm)
>>     #define MMF_DISABLE_THP_MASK	(1 << MMF_DISABLE_THP)
>>   +#define MMF_VM_MERGE_ANY	29
>> +#define MMF_VM_MERGE_ANY_MASK	(1 << MMF_VM_MERGE_ANY)
>> +
>>   #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK |\
>> -				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK)
>> +				 MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK |\
>> +				 MMF_VM_MERGE_ANY_MASK)
>>   -#define MMF_VM_MERGE_ANY	29
>>   #endif /* _LINUX_SCHED_COREDUMP_H */
>
> Fixes: tag?
>

Added fixes tag

> Reviewed-by: David Hildenbrand <david@redhat.com>

[PATCH v1 2/2] mm/ksm: Test case for prctl fork/exec workflow

[Stefan Roesch]

This adds a new test case to the ksm functional tests to make sure that
the KSM setting is inherited by the child process when doing a
fork/exec.

Signed-off-by: Stefan Roesch <shr@devkernel.io>
---
 tools/testing/selftests/mm/Makefile           |  2 +
 .../selftests/mm/ksm_fork_exec_child.c        |  9 ++++
 .../selftests/mm/ksm_functional_tests.c       | 50 ++++++++++++++++++-
 3 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/mm/ksm_fork_exec_child.c

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 6a9fc5693145..9ab6aa402544 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -73,6 +73,8 @@ ifneq ($(ARCH),arm64)
 TEST_GEN_PROGS += soft-dirty
 endif
 
+TEST_GEN_PROGS += ksm_fork_exec_child
+
 ifeq ($(ARCH),x86_64)
 CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
 CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
diff --git a/tools/testing/selftests/mm/ksm_fork_exec_child.c b/tools/testing/selftests/mm/ksm_fork_exec_child.c
new file mode 100644
index 000000000000..298439f0d55f
--- /dev/null
+++ b/tools/testing/selftests/mm/ksm_fork_exec_child.c
@@ -0,0 +1,9 @@
+#include <sys/prctl.h>
+#include <stdlib.h>
+
+int main()
+{
+	/* Test if KSM is enabled for the process. */
+	int ksm = prctl(68, 0, 0, 0, 0);
+	exit(ksm == 1 ? 0 : 1);
+}
diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index 901e950f9138..4dc0bb522c07 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -479,6 +479,53 @@ static void test_prctl_fork(void)
 	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
 }
 
+static void test_prctl_fork_exec(void)
+{
+	int ret, status;
+	pid_t child_pid;
+
+	ksft_print_msg("[RUN] %s\n", __func__);
+
+	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
+	if (ret < 0 && errno == EINVAL) {
+		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
+		return;
+	} else if (ret) {
+		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
+		return;
+	}
+
+	child_pid = fork();
+	if (child_pid == -1) {
+		ksft_test_result_skip("fork() failed\n");
+		return;
+	} else if (child_pid == 0) {
+		char *filename = "./ksm_fork_exec_child";
+		char *argv_for_program[] = { filename, NULL };
+
+		execv(filename, argv_for_program);;
+	} else {
+		if (waitpid(child_pid, &status, 0) > 0) {
+			if (WIFEXITED(status)) {
+				status = WEXITSTATUS(status);
+				if (status) {
+					ksft_test_result_fail("KSM not enabled\n");
+					return;
+				}
+
+			} else {
+				ksft_test_result_fail("program didn't terminate normally\n");
+				return;
+			}
+		} else {
+			ksft_test_result_fail("waitpid() failed\n");
+			return;
+		}
+	}
+
+	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
+}
+
 static void test_prctl_unmerge(void)
 {
 	const unsigned int size = 2 * MiB;
@@ -536,7 +583,7 @@ static void test_prot_none(void)
 
 int main(int argc, char **argv)
 {
-	unsigned int tests = 7;
+	unsigned int tests = 8;
 	int err;
 
 #ifdef __NR_userfaultfd
@@ -576,6 +623,7 @@ int main(int argc, char **argv)
 
 	test_prctl();
 	test_prctl_fork();
+	test_prctl_fork_exec();
 	test_prctl_unmerge();
 
 	err = ksft_get_fail_cnt();
-- 
2.39.3

Re: [PATCH v1 2/2] mm/ksm: Test case for prctl fork/exec workflow

[David Hildenbrand]

On 19.09.23 22:51, Stefan Roesch wrote:
> This adds a new test case to the ksm functional tests to make sure that
> the KSM setting is inherited by the child process when doing a
> fork/exec.
> 
> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> ---
>   tools/testing/selftests/mm/Makefile           |  2 +
>   .../selftests/mm/ksm_fork_exec_child.c        |  9 ++++
>   .../selftests/mm/ksm_functional_tests.c       | 50 ++++++++++++++++++-
>   3 files changed, 60 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/mm/ksm_fork_exec_child.c
> 
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index 6a9fc5693145..9ab6aa402544 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -73,6 +73,8 @@ ifneq ($(ARCH),arm64)
>   TEST_GEN_PROGS += soft-dirty
>   endif
>   
> +TEST_GEN_PROGS += ksm_fork_exec_child

It's not a test itself, so it shouldn't be run when running all tests. 
See below.

> +
>   ifeq ($(ARCH),x86_64)
>   CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
>   CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
> diff --git a/tools/testing/selftests/mm/ksm_fork_exec_child.c b/tools/testing/selftests/mm/ksm_fork_exec_child.c
> new file mode 100644
> index 000000000000..298439f0d55f
> --- /dev/null
> +++ b/tools/testing/selftests/mm/ksm_fork_exec_child.c
> @@ -0,0 +1,9 @@
> +#include <sys/prctl.h>
> +#include <stdlib.h>
> +
> +int main()
> +{
> +	/* Test if KSM is enabled for the process. */
> +	int ksm = prctl(68, 0, 0, 0, 0);

Can we use the define from a header? (PR_SET_MEMORY_MERGE)

I was wondering if we could simply exec() ourself (same binary), but 
pass a special cmdline argument. Then you don't have to build a separate 
binary.

Just special-case in main() on that argument and perform this check.

> +	exit(ksm == 1 ? 0 : 1);
> +}
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
> index 901e950f9138..4dc0bb522c07 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -479,6 +479,53 @@ static void test_prctl_fork(void)
>   	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
>   }
>   
> +static void test_prctl_fork_exec(void)
> +{
> +	int ret, status;
> +	pid_t child_pid;
> +
> +	ksft_print_msg("[RUN] %s\n", __func__);
> +
> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> +	if (ret < 0 && errno == EINVAL) {
> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
> +		return;
> +	} else if (ret) {
> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
> +		return;
> +	}
> +
> +	child_pid = fork();
> +	if (child_pid == -1) {
> +		ksft_test_result_skip("fork() failed\n");
> +		return;
> +	} else if (child_pid == 0) {
> +		char *filename = "./ksm_fork_exec_child";
> +		char *argv_for_program[] = { filename, NULL };
> +
> +		execv(filename, argv_for_program);;

s/;;/;/

Add a return; so you can simplify the code below (no need for the "else")

> +	} else {
> +		if (waitpid(child_pid, &status, 0) > 0) {
> +			if (WIFEXITED(status)) {
> +				status = WEXITSTATUS(status);
> +				if (status) {
> +					ksft_test_result_fail("KSM not enabled\n");
> +					return;
> +				}
> +
> +			} else {
> +				ksft_test_result_fail("program didn't terminate normally\n");
> +				return;
> +			}
> +		} else {
> +			ksft_test_result_fail("waitpid() failed\n");
> +			return;
> +		}
> +	}
> +
> +	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");

It's probably the cleanest to disable PR_SET_MEMORY_MERGE again when 
returning form this function, so the other tests have a clean slate.


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/ksm: Test case for prctl fork/exec workflow

[Stefan Roesch]

David Hildenbrand <david@redhat.com> writes:

> On 19.09.23 22:51, Stefan Roesch wrote:
>> This adds a new test case to the ksm functional tests to make sure that
>> the KSM setting is inherited by the child process when doing a
>> fork/exec.
>> Signed-off-by: Stefan Roesch <shr@devkernel.io>
>> ---
>>   tools/testing/selftests/mm/Makefile           |  2 +
>>   .../selftests/mm/ksm_fork_exec_child.c        |  9 ++++
>>   .../selftests/mm/ksm_functional_tests.c       | 50 ++++++++++++++++++-
>>   3 files changed, 60 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/testing/selftests/mm/ksm_fork_exec_child.c
>> diff --git a/tools/testing/selftests/mm/Makefile
>> b/tools/testing/selftests/mm/Makefile
>> index 6a9fc5693145..9ab6aa402544 100644
>> --- a/tools/testing/selftests/mm/Makefile
>> +++ b/tools/testing/selftests/mm/Makefile
>> @@ -73,6 +73,8 @@ ifneq ($(ARCH),arm64)
>>   TEST_GEN_PROGS += soft-dirty
>>   endif
>>   +TEST_GEN_PROGS += ksm_fork_exec_child
>
> It's not a test itself, so it shouldn't be run when running all tests. See
> below.
>
>> +
>>   ifeq ($(ARCH),x86_64)
>>   CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
>>   CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
>> diff --git a/tools/testing/selftests/mm/ksm_fork_exec_child.c b/tools/testing/selftests/mm/ksm_fork_exec_child.c
>> new file mode 100644
>> index 000000000000..298439f0d55f
>> --- /dev/null
>> +++ b/tools/testing/selftests/mm/ksm_fork_exec_child.c
>> @@ -0,0 +1,9 @@
>> +#include <sys/prctl.h>
>> +#include <stdlib.h>
>> +
>> +int main()
>> +{
>> +	/* Test if KSM is enabled for the process. */
>> +	int ksm = prctl(68, 0, 0, 0, 0);
>
> Can we use the define from a header? (PR_SET_MEMORY_MERGE)
>

I changed it.

> I was wondering if we could simply exec() ourself (same binary), but pass a
> special cmdline argument. Then you don't have to build a separate binary.
>

I'm execing the same executable with an additional parameter.

> Just special-case in main() on that argument and perform this check.
>

I added a check in main and invoke a new function for the child.

>> +	exit(ksm == 1 ? 0 : 1);
>> +}
>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
>> index 901e950f9138..4dc0bb522c07 100644
>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>> @@ -479,6 +479,53 @@ static void test_prctl_fork(void)
>>   	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
>>   }
>>   +static void test_prctl_fork_exec(void)
>> +{
>> +	int ret, status;
>> +	pid_t child_pid;
>> +
>> +	ksft_print_msg("[RUN] %s\n", __func__);
>> +
>> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
>> +	if (ret < 0 && errno == EINVAL) {
>> +		ksft_test_result_skip("PR_SET_MEMORY_MERGE not supported\n");
>> +		return;
>> +	} else if (ret) {
>> +		ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
>> +		return;
>> +	}
>> +
>> +	child_pid = fork();
>> +	if (child_pid == -1) {
>> +		ksft_test_result_skip("fork() failed\n");
>> +		return;
>> +	} else if (child_pid == 0) {
>> +		char *filename = "./ksm_fork_exec_child";
>> +		char *argv_for_program[] = { filename, NULL };
>> +
>> +		execv(filename, argv_for_program);;
>
> s/;;/;/
>

Fixed.

> Add a return; so you can simplify the code below (no need for the "else")
>

Done.


>> +	} else {
>> +		if (waitpid(child_pid, &status, 0) > 0) {
>> +			if (WIFEXITED(status)) {
>> +				status = WEXITSTATUS(status);
>> +				if (status) {
>> +					ksft_test_result_fail("KSM not enabled\n");
>> +					return;
>> +				}
>> +
>> +			} else {
>> +				ksft_test_result_fail("program didn't terminate normally\n");
>> +				return;
>> +			}
>> +		} else {
>> +			ksft_test_result_fail("waitpid() failed\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	ksft_test_result_pass("PR_SET_MEMORY_MERGE value is inherited\n");
>
> It's probably the cleanest to disable PR_SET_MEMORY_MERGE again when returning
> form this function, so the other tests have a clean slate.

In the next version it is disabled at the end.