[PATCH v3 0/3] mm/ksm: fix ksm exec support for prctl

[PATCH v3 0/3] mm/ksm: fix ksm exec support for prctl

[Jinjiang Tu]

commit 3c6f33b7273a ("mm/ksm: support fork/exec for prctl") inherits
MMF_VM_MERGE_ANY flag when a task calls execve(). However, it doesn't
create the mm_slot, so ksmd will not try to scan this task. The first
patch fixes the issue.

The second patch refactors to prepare for the third patch. The third patch
extends the selftests of ksm to verfity the deduplication really happens
after fork/exec inherits ths KSM setting.

Changelog since v2:
  - fix uninitialized warning reported by kernel test robot.
  - refactor selftests.

Changelog since v1:
  - Add ksm cleanup in __bprm_mm_init() when error occurs.
  - Add some comment.
  - Extend the selftests of ksm fork/exec.

Jinjiang Tu (3):
  mm/ksm: fix ksm exec support for prctl
  selftest/mm: ksm_functional_tests: refactor mmap_and_merge_range()
  selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec

 fs/exec.c                                     |  11 ++
 include/linux/ksm.h                           |  13 ++
 .../selftests/mm/ksm_functional_tests.c       | 145 +++++++++++++-----
 3 files changed, 133 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] mm/ksm: fix ksm exec support for prctl

[Jinjiang Tu]

commit 3c6f33b7273a ("mm/ksm: support fork/exec for prctl") inherits
MMF_VM_MERGE_ANY flag when a task calls execve(). Howerver, it doesn't
create the mm_slot, so ksmd will not try to scan this task.

To fix it, allocate and add the mm_slot to ksm_mm_head in __bprm_mm_init()
when the mm has MMF_VM_MERGE_ANY flag.

Fixes: 3c6f33b7273a ("mm/ksm: support fork/exec for prctl")
Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
---
 fs/exec.c           | 11 +++++++++++
 include/linux/ksm.h | 13 +++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index ff6f26671cfc..c2890d32232e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -67,6 +67,7 @@
 #include <linux/time_namespace.h>
 #include <linux/user_events.h>
 #include <linux/rseq.h>
+#include <linux/ksm.h>
 
 #include <linux/uaccess.h>
 #include <asm/mmu_context.h>
@@ -267,6 +268,14 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 		goto err_free;
 	}
 
+	/*
+	 * Need to be called with mmap write lock
+	 * held, to avoid race with ksmd.
+	 */
+	err = ksm_execve(mm);
+	if (err)
+		goto err_ksm;
+
 	/*
 	 * Place the stack at the largest stack address the architecture
 	 * supports. Later, we'll move this to an appropriate place. We don't
@@ -288,6 +297,8 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 	bprm->p = vma->vm_end - sizeof(void *);
 	return 0;
 err:
+	ksm_exit(mm);
+err_ksm:
 	mmap_write_unlock(mm);
 err_free:
 	bprm->vma = NULL;
diff --git a/include/linux/ksm.h b/include/linux/ksm.h
index 401348e9f92b..7e2b1de3996a 100644
--- a/include/linux/ksm.h
+++ b/include/linux/ksm.h
@@ -59,6 +59,14 @@ static inline int ksm_fork(struct mm_struct *mm, struct mm_struct *oldmm)
 	return 0;
 }
 
+static inline int ksm_execve(struct mm_struct *mm)
+{
+	if (test_bit(MMF_VM_MERGE_ANY, &mm->flags))
+		return __ksm_enter(mm);
+
+	return 0;
+}
+
 static inline void ksm_exit(struct mm_struct *mm)
 {
 	if (test_bit(MMF_VM_MERGEABLE, &mm->flags))
@@ -107,6 +115,11 @@ static inline int ksm_fork(struct mm_struct *mm, struct mm_struct *oldmm)
 	return 0;
 }
 
+static inline int ksm_execve(struct mm_struct *mm)
+{
+	return 0;
+}
+
 static inline void ksm_exit(struct mm_struct *mm)
 {
 }
-- 
2.25.1

[PATCH v3 2/3] selftest/mm: ksm_functional_tests: refactor mmap_and_merge_range()

[Jinjiang Tu]

In order to extend test_prctl_fork() and test_prctl_fork_exec() to make
sure that deduplication really happens, mmap_and_merge_range() needs to be
refactored.

Firstly, mmap_and_merge_range() will be called with no need to call enable
KSM by madvise or prctl. So, switch the 'bool use_prctl' parameter to enum
ksm_merge_mode.

Secondly, mmap_and_merge_range() will be called in child process in the
two testcases, it isn't appropriate to call ksft_test_result_{fail, skip},
because the global variables ksft_{fail, skip} aren't consistent with the
parent process. Thus, convert calls of ksft_test_result_{fail, skip} to
ksft_print_msg() and return differrent error according to the two cases.
The callers of mmap_and_merge_range() then call ksft_test_result_{fail,
skip} according to the return value. Introduce mmap_and_merge_err_print()
helper to make it easier.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
---
 .../selftests/mm/ksm_functional_tests.c       | 93 +++++++++++++------
 1 file changed, 63 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index d615767e396b..da58545e58e1 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -28,6 +28,15 @@
 #define MiB (1024 * KiB)
 #define FORK_EXEC_CHILD_PRG_NAME "ksm_fork_exec_child"
 
+#define MAP_MERGE_FAIL ((void *)-1)
+#define MAP_MERGE_SKIP ((void *)-2)
+
+enum ksm_merge_mode {
+	KSM_MERGE_PRCTL,
+	KSM_MERGE_MADVISE,
+	KSM_MERGE_NONE, /* PRCTL already set */
+};
+
 static int mem_fd;
 static int ksm_fd;
 static int ksm_full_scans_fd;
@@ -147,32 +156,33 @@ static int ksm_unmerge(void)
 }
 
 static char *mmap_and_merge_range(char val, unsigned long size, int prot,
-				  bool use_prctl)
+				  enum ksm_merge_mode mode)
 {
 	char *map;
+	char *err_map = MAP_MERGE_FAIL;
 	int ret;
 
 	/* Stabilize accounting by disabling KSM completely. */
 	if (ksm_unmerge()) {
-		ksft_test_result_fail("Disabling (unmerging) KSM failed\n");
-		return MAP_FAILED;
+		ksft_print_msg("Disabling (unmerging) KSM failed\n");
+		return err_map;
 	}
 
 	if (get_my_merging_pages() > 0) {
-		ksft_test_result_fail("Still pages merged\n");
-		return MAP_FAILED;
+		ksft_print_msg("Still pages merged\n");
+		return err_map;
 	}
 
 	map = mmap(NULL, size, PROT_READ|PROT_WRITE,
 		   MAP_PRIVATE|MAP_ANON, -1, 0);
 	if (map == MAP_FAILED) {
-		ksft_test_result_fail("mmap() failed\n");
-		return MAP_FAILED;
+		ksft_print_msg("mmap() failed\n");
+		return err_map;
 	}
 
 	/* Don't use THP. Ignore if THP are not around on a kernel. */
 	if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
-		ksft_test_result_fail("MADV_NOHUGEPAGE failed\n");
+		ksft_print_msg("MADV_NOHUGEPAGE failed\n");
 		goto unmap;
 	}
 
@@ -180,27 +190,36 @@ static char *mmap_and_merge_range(char val, unsigned long size, int prot,
 	memset(map, val, size);
 
 	if (mprotect(map, size, prot)) {
-		ksft_test_result_skip("mprotect() failed\n");
+		ksft_print_msg("mprotect() failed\n");
+		err_map = MAP_MERGE_SKIP;
 		goto unmap;
 	}
 
-	if (use_prctl) {
+	switch (mode) {
+	case KSM_MERGE_PRCTL:
 		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");
+			ksft_print_msg("PR_SET_MEMORY_MERGE not supported\n");
+			err_map = MAP_MERGE_SKIP;
 			goto unmap;
 		} else if (ret) {
-			ksft_test_result_fail("PR_SET_MEMORY_MERGE=1 failed\n");
+			ksft_print_msg("PR_SET_MEMORY_MERGE=1 failed\n");
 			goto unmap;
 		}
-	} else if (madvise(map, size, MADV_MERGEABLE)) {
-		ksft_test_result_fail("MADV_MERGEABLE failed\n");
-		goto unmap;
+		break;
+	case KSM_MERGE_MADVISE:
+		if (madvise(map, size, MADV_MERGEABLE)) {
+			ksft_print_msg("MADV_MERGEABLE failed\n");
+			goto unmap;
+		}
+		break;
+	case KSM_MERGE_NONE:
+		break;
 	}
 
 	/* Run KSM to trigger merging and wait. */
 	if (ksm_merge()) {
-		ksft_test_result_fail("Running KSM failed\n");
+		ksft_print_msg("Running KSM failed\n");
 		goto unmap;
 	}
 
@@ -209,14 +228,28 @@ static char *mmap_and_merge_range(char val, unsigned long size, int prot,
 	 * accounted differently (depending on kernel support).
 	 */
 	if (val && !get_my_merging_pages()) {
-		ksft_test_result_fail("No pages got merged\n");
+		ksft_print_msg("No pages got merged\n");
 		goto unmap;
 	}
 
 	return map;
 unmap:
 	munmap(map, size);
-	return MAP_FAILED;
+	return err_map;
+}
+
+static inline int mmap_and_merge_err_print(char *map)
+{
+	int ret = -1;
+
+	if (map == MAP_MERGE_FAIL)
+		ksft_test_result_fail("Merging memory failed");
+	else if (map == MAP_MERGE_FAIL)
+		ksft_test_result_skip("Merging memory skiped");
+	else
+		ret = 0;
+
+	return ret;
 }
 
 static void test_unmerge(void)
@@ -226,8 +259,8 @@ static void test_unmerge(void)
 
 	ksft_print_msg("[RUN] %s\n", __func__);
 
-	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, false);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
+	if (mmap_and_merge_err_print(map))
 		return;
 
 	if (madvise(map, size, MADV_UNMERGEABLE)) {
@@ -264,8 +297,8 @@ static void test_unmerge_zero_pages(void)
 	}
 
 	/* Let KSM deduplicate zero pages. */
-	map = mmap_and_merge_range(0x00, size, PROT_READ | PROT_WRITE, false);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0x00, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
+	if (mmap_and_merge_err_print(map))
 		return;
 
 	/* Check if ksm_zero_pages is updated correctly after KSM merging */
@@ -312,8 +345,8 @@ static void test_unmerge_discarded(void)
 
 	ksft_print_msg("[RUN] %s\n", __func__);
 
-	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, false);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
+	if (mmap_and_merge_err_print(map))
 		return;
 
 	/* Discard half of all mapped pages so we have pte_none() entries. */
@@ -344,8 +377,8 @@ static void test_unmerge_uffd_wp(void)
 
 	ksft_print_msg("[RUN] %s\n", __func__);
 
-	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, false);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
+	if (mmap_and_merge_err_print(map))
 		return;
 
 	/* See if UFFD is around. */
@@ -545,8 +578,8 @@ static void test_prctl_unmerge(void)
 
 	ksft_print_msg("[RUN] %s\n", __func__);
 
-	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, true);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_PRCTL);
+	if (mmap_and_merge_err_print(map))
 		return;
 
 	if (prctl(PR_SET_MEMORY_MERGE, 0, 0, 0, 0)) {
@@ -568,8 +601,8 @@ static void test_prot_none(void)
 
 	ksft_print_msg("[RUN] %s\n", __func__);
 
-	map = mmap_and_merge_range(0x11, size, PROT_NONE, false);
-	if (map == MAP_FAILED)
+	map = mmap_and_merge_range(0x11, size, PROT_NONE, KSM_MERGE_MADVISE);
+	if (mmap_and_merge_err_print(map))
 		goto unmap;
 
 	/* Store a unique value in each page on one half using ptrace */
-- 
2.25.1

[PATCH v3 3/3] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec

[Jinjiang Tu]

This extends test_prctl_fork() and test_prctl_fork_exec() to make sure
that deduplication really happens, instead of only test the
MMF_VM_MERGE_ANY flag is set.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
---
 .../selftests/mm/ksm_functional_tests.c       | 52 ++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index da58545e58e1..c56e34946096 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -491,7 +491,20 @@ static void test_prctl_fork(void)
 
 	child_pid = fork();
 	if (!child_pid) {
-		exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
+		const unsigned int size = 2 * MiB;
+		char *map;
+
+		if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
+			exit(-1);
+
+		map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_NONE);
+		if (map == MAP_MERGE_FAIL)
+			exit(-2);
+		else if (map == MAP_MERGE_SKIP)
+			exit(-3);
+
+		munmap(map, size);
+		exit(0);
 	} else if (child_pid < 0) {
 		ksft_test_result_fail("fork() failed\n");
 		return;
@@ -500,8 +513,16 @@ static void test_prctl_fork(void)
 	if (waitpid(child_pid, &status, 0) < 0) {
 		ksft_test_result_fail("waitpid() failed\n");
 		return;
-	} else if (WEXITSTATUS(status) != 1) {
-		ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE result in child\n");
+	}
+
+	status = WEXITSTATUS(status);
+	if (status != 0) {
+		if (status == -1)
+			ksft_test_result_fail("unexpected PR_GET_MEMORY_MERGE result in child\n");
+		else if (status == -2)
+			ksft_test_result_fail("Merge in child failed\n");
+		else if (status == -3)
+			ksft_test_result_skip("Merge in child skiped\n");
 		return;
 	}
 
@@ -515,8 +536,21 @@ static void test_prctl_fork(void)
 
 static int ksm_fork_exec_child(void)
 {
+	const unsigned int size = 2 * MiB;
+	char *map;
+
 	/* Test if KSM is enabled for the process. */
-	return prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) == 1;
+	if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
+		return -1;
+
+	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_NONE);
+	if (map == MAP_MERGE_FAIL)
+		return -2;
+	else if (map == MAP_MERGE_SKIP)
+		return -3;
+
+	munmap(map, size);
+	return 0;
 }
 
 static void test_prctl_fork_exec(void)
@@ -550,9 +584,15 @@ static void test_prctl_fork_exec(void)
 	if (waitpid(child_pid, &status, 0) > 0) {
 		if (WIFEXITED(status)) {
 			status = WEXITSTATUS(status);
-			if (status) {
+			if (status == -1) {
 				ksft_test_result_fail("KSM not enabled\n");
 				return;
+			} else if (status == -2) {
+				ksft_test_result_fail("Merge in child failed\n");
+				return;
+			} else if (status == -3) {
+				ksft_test_result_skip("Merge in child skiped\n");
+				return;
 			}
 		} else {
 			ksft_test_result_fail("program didn't terminate normally\n");
@@ -632,7 +672,7 @@ int main(int argc, char **argv)
 	int err;
 
 	if (argc > 1 && !strcmp(argv[1], FORK_EXEC_CHILD_PRG_NAME)) {
-		exit(ksm_fork_exec_child() == 1 ? 0 : 1);
+		exit(ksm_fork_exec_child());
 	}
 
 #ifdef __NR_userfaultfd
-- 
2.25.1

Re: [PATCH v3 1/3] mm/ksm: fix ksm exec support for prctl

[David Hildenbrand]

On 27.03.24 07:09, Jinjiang Tu wrote:
> commit 3c6f33b7273a ("mm/ksm: support fork/exec for prctl") inherits
> MMF_VM_MERGE_ANY flag when a task calls execve(). Howerver, it doesn't
> create the mm_slot, so ksmd will not try to scan this task.
> 
> To fix it, allocate and add the mm_slot to ksm_mm_head in __bprm_mm_init()
> when the mm has MMF_VM_MERGE_ANY flag.
> 
> Fixes: 3c6f33b7273a ("mm/ksm: support fork/exec for prctl")
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---



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

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/3] selftest/mm: ksm_functional_tests: refactor mmap_and_merge_range()

[David Hildenbrand]

On 27.03.24 07:09, Jinjiang Tu wrote:
> In order to extend test_prctl_fork() and test_prctl_fork_exec() to make
> sure that deduplication really happens, mmap_and_merge_range() needs to be
> refactored.
> 
> Firstly, mmap_and_merge_range() will be called with no need to call enable
> KSM by madvise or prctl. So, switch the 'bool use_prctl' parameter to enum
> ksm_merge_mode.
> 
> Secondly, mmap_and_merge_range() will be called in child process in the
> two testcases, it isn't appropriate to call ksft_test_result_{fail, skip},
> because the global variables ksft_{fail, skip} aren't consistent with the
> parent process. Thus, convert calls of ksft_test_result_{fail, skip} to
> ksft_print_msg() and return differrent error according to the two cases.
> The callers of mmap_and_merge_range() then call ksft_test_result_{fail,
> skip} according to the return value. Introduce mmap_and_merge_err_print()
> helper to make it easier.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---

[...]

>   
> @@ -209,14 +228,28 @@ static char *mmap_and_merge_range(char val, unsigned long size, int prot,
>   	 * accounted differently (depending on kernel support).
>   	 */
>   	if (val && !get_my_merging_pages()) {
> -		ksft_test_result_fail("No pages got merged\n");
> +		ksft_print_msg("No pages got merged\n");
>   		goto unmap;
>   	}
>   
>   	return map;
>   unmap:
>   	munmap(map, size);
> -	return MAP_FAILED;
> +	return err_map;
> +}
> +
> +static inline int mmap_and_merge_err_print(char *map)
> +{
> +	int ret = -1;
> +
> +	if (map == MAP_MERGE_FAIL)
> +		ksft_test_result_fail("Merging memory failed");
> +	else if (map == MAP_MERGE_FAIL)

MAP_MERGE_SKIP

> +		ksft_test_result_skip("Merging memory skiped");

"skipped"

> +	else
> +		ret = 0;
> +
> +	return ret;
>   }
>   
>   static void test_unmerge(void)
> @@ -226,8 +259,8 @@ static void test_unmerge(void)
>   
>   	ksft_print_msg("[RUN] %s\n", __func__);
>   
> -	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, false);
> -	if (map == MAP_FAILED)
> +	map = mmap_and_merge_range(0xcf, size, PROT_READ | PROT_WRITE, KSM_MERGE_MADVISE);
> +	if (mmap_and_merge_err_print(map))

I like the concept.

The following would be cleaner and result in less churn:

1) Rename mmap_and_merge_range() to "__mmap_and_merge_range()"

2) Introduce a new mmap_and_merge_range()

static inline char *mmap_and_merge_range(char val, unsigned long size,
		int prot, enum ksm_merge_mode mode)
{
	char *map = __mmap_and_merge_range(val, size, prot, mode);	
	char *ret = MAP_FAILED;

	if (map == MAP_MERGE_FAIL)
		ksft_test_result_fail("Merging memory failed");
	else if (map == MAP_MERGE_SKIP)
		ksft_test_result_skip("Merging memory skipped");
	else
		ret = map;

	return ret;
}

3) Use __mmap_and_merge_range() in patch #3.

No need to adjust any existing callers of mmap_and_merge_range().

-- 
Cheers,

David / dhildenb