All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] mm/ksm: fix ksm exec support for prctl
@ 2024-03-22  6:09 Jinjiang Tu
  2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
  2024-03-22  6:09 ` [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec Jinjiang Tu
  0 siblings, 2 replies; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-22  6:09 UTC (permalink / raw)
  To: akpm, david, shr, hannes, riel, wangkefeng.wang, sunnanyong, linux-mm
  Cc: tujinjiang

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. The first
patch fixes the issue.

The second patch extend the selftests of ksm to verfity the deduplication
really happens after fork/exec inherits ths KSM setting.

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 (2):
  mm/ksm: fix ksm exec support for prctl
  selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec

 fs/exec.c                                     | 10 +++
 include/linux/ksm.h                           | 13 +++
 .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
 3 files changed, 96 insertions(+), 6 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-22  6:09 [PATCH v2 0/2] mm/ksm: fix ksm exec support for prctl Jinjiang Tu
@ 2024-03-22  6:09 ` Jinjiang Tu
  2024-03-22  9:02   ` David Hildenbrand
                     ` (2 more replies)
  2024-03-22  6:09 ` [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec Jinjiang Tu
  1 sibling, 3 replies; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-22  6:09 UTC (permalink / raw)
  To: akpm, david, shr, hannes, riel, wangkefeng.wang, sunnanyong, linux-mm
  Cc: tujinjiang

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           | 10 ++++++++++
 include/linux/ksm.h | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index ff6f26671cfc..66202d016a0a 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,13 @@ 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.
+	*/
+	if (ksm_execve(mm))
+		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 +296,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



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec
  2024-03-22  6:09 [PATCH v2 0/2] mm/ksm: fix ksm exec support for prctl Jinjiang Tu
  2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
@ 2024-03-22  6:09 ` Jinjiang Tu
  2024-03-22 11:43   ` David Hildenbrand
  1 sibling, 1 reply; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-22  6:09 UTC (permalink / raw)
  To: akpm, david, shr, hannes, riel, wangkefeng.wang, sunnanyong, linux-mm
  Cc: tujinjiang

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.

Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
---
 .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index d615767e396b..01999aab2e37 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -146,6 +146,54 @@ static int ksm_unmerge(void)
 	return 0;
 }
 
+static int child_test_merge(void)
+{
+	const unsigned int size = 2 * MiB;
+	char *map;
+	int ret = -1;
+
+	/* Stabilize accounting by disabling KSM completely. */
+	if (ksm_unmerge()) {
+		ksft_print_msg("Disabling (unmerging) KSM failed\n");
+		return ret;
+	}
+
+	if (get_my_merging_pages() > 0) {
+		ksft_print_msg("Still pages merged\n");
+		return ret;
+	}
+
+	map = mmap(NULL, size, PROT_READ|PROT_WRITE,
+		   MAP_PRIVATE|MAP_ANON, -1, 0);
+	if (map == MAP_FAILED) {
+		ksft_print_msg("mmap() failed\n");
+		return ret;
+	}
+
+	/* Don't use THP. Ignore if THP are not around on a kernel. */
+	if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
+		ksft_print_msg("MADV_NOHUGEPAGE failed\n");
+		goto unmap;
+	}
+
+	memset(map, 0x1c, size);
+
+	if (ksm_merge()) {
+		ksft_print_msg("Running KSM failed\n");
+		goto unmap;
+	}
+
+	if (get_my_merging_pages() <= 0) {
+		ksft_print_msg("Fail to merge\n");
+		goto unmap;
+	}
+
+	ret = 0;
+unmap:
+	munmap(map, size);
+	return ret;
+}
+
 static char *mmap_and_merge_range(char val, unsigned long size, int prot,
 				  bool use_prctl)
 {
@@ -458,7 +506,11 @@ static void test_prctl_fork(void)
 
 	child_pid = fork();
 	if (!child_pid) {
-		exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
+		if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
+			exit(-1);
+		if (child_test_merge() != 0)
+			exit(-2);
+		exit(0);
 	} else if (child_pid < 0) {
 		ksft_test_result_fail("fork() failed\n");
 		return;
@@ -467,8 +519,14 @@ 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
+			ksft_test_result_fail("fail to merge in child\n");
 		return;
 	}
 
@@ -483,7 +541,13 @@ static void test_prctl_fork(void)
 static int ksm_fork_exec_child(void)
 {
 	/* 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;
+
+	if (child_test_merge() != 0)
+		return -2;
+
+	return 0;
 }
 
 static void test_prctl_fork_exec(void)
@@ -517,9 +581,12 @@ 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("fail to merge in child\n");
+				return;
 			}
 		} else {
 			ksft_test_result_fail("program didn't terminate normally\n");
@@ -599,7 +666,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



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
@ 2024-03-22  9:02   ` David Hildenbrand
  2024-03-25  2:24     ` Jinjiang Tu
  2024-03-24  0:03   ` kernel test robot
  2024-03-25  5:44   ` Dan Carpenter
  2 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2024-03-22  9:02 UTC (permalink / raw)
  To: Jinjiang Tu, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm

On 22.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>
> ---
>   fs/exec.c           | 10 ++++++++++
>   include/linux/ksm.h | 13 +++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index ff6f26671cfc..66202d016a0a 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,13 @@ 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.
> +	*/
> +	if (ksm_execve(mm))
> +		goto err_ksm;
> +

But now, would we revert what insert_vm_struct() did?

We're freeing the VMA later, but we might have accounted memory.


What would be cleaner is doing the ksm_execve() before the 
insert_vm_struct(), and then cleaning up in case insert_vm_struct() failed.

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec
  2024-03-22  6:09 ` [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec Jinjiang Tu
@ 2024-03-22 11:43   ` David Hildenbrand
  2024-03-25  2:24     ` Jinjiang Tu
  0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2024-03-22 11:43 UTC (permalink / raw)
  To: Jinjiang Tu, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm

On 22.03.24 07:09, Jinjiang Tu wrote:
> 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.
> 
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
> ---
>   .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
>   1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
> index d615767e396b..01999aab2e37 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -146,6 +146,54 @@ static int ksm_unmerge(void)
>   	return 0;
>   }
>   
> +static int child_test_merge(void)
> +{
> +	const unsigned int size = 2 * MiB;
> +	char *map;
> +	int ret = -1;
> +
> +	/* Stabilize accounting by disabling KSM completely. */
> +	if (ksm_unmerge()) {
> +		ksft_print_msg("Disabling (unmerging) KSM failed\n");
> +		return ret;
> +	}
> +
> +	if (get_my_merging_pages() > 0) {
> +		ksft_print_msg("Still pages merged\n");
> +		return ret;
> +	}
> +
> +	map = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +		   MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (map == MAP_FAILED) {
> +		ksft_print_msg("mmap() failed\n");
> +		return ret;
> +	}
> +
> +	/* Don't use THP. Ignore if THP are not around on a kernel. */
> +	if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
> +		ksft_print_msg("MADV_NOHUGEPAGE failed\n");
> +		goto unmap;
> +	}
> +
> +	memset(map, 0x1c, size);
> +
> +	if (ksm_merge()) {
> +		ksft_print_msg("Running KSM failed\n");
> +		goto unmap;
> +	}
> +
> +	if (get_my_merging_pages() <= 0) {
> +		ksft_print_msg("Fail to merge\n");
> +		goto unmap;
> +	}

Looks like all you want is use mmap_and_merge_range(), but neither 
setting the prctl nor madvise().

Two alternatives:

1) switching from "bool use_prctl" to an enum like

enum ksm_merge_mode {
	KSM_MERGE_PRCTL
	KSM_MERGE_MADVISE,
	KSM_MERGE_NONE, /* PRCTL already set */
};

Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
PROT_READ|PROT_WRITE, KSM_MERGE_NONE);

2) With "bool use_prctl", before doing the prctl(PR_SET_MEMORY_MERGE, 
...), check if it is already enabled.

As we do that already in ksm_fork_exec_child(), and fail if it isn't 
set, that should work.

Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
PROT_READ|PROT_WRITE, true);

here.

> +
> +	ret = 0;
> +unmap:
> +	munmap(map, size);
> +	return ret;
> +}
> +
>   static char *mmap_and_merge_range(char val, unsigned long size, int prot,
>   				  bool use_prctl)
>   {
> @@ -458,7 +506,11 @@ static void test_prctl_fork(void)
>   
>   	child_pid = fork();
>   	if (!child_pid) {
> -		exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
> +		if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
> +			exit(-1);
> +		if (child_test_merge() != 0)
> +			exit(-2);
> +		exit(0);
>   	} else if (child_pid < 0) {
>   		ksft_test_result_fail("fork() failed\n");
>   		return;
> @@ -467,8 +519,14 @@ 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
> +			ksft_test_result_fail("fail to merge in child\n");
>   		return;
>   	}
>   
> @@ -483,7 +541,13 @@ static void test_prctl_fork(void)
>   static int ksm_fork_exec_child(void)
>   {
>   	/* 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;
> +
> +	if (child_test_merge() != 0)

You can drop the "!=0". But maybe, you can just inline the call to 
mmap_and_merge_range() here.

> +		return -2;
> +
> +	return 0;


-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
  2024-03-22  9:02   ` David Hildenbrand
@ 2024-03-24  0:03   ` kernel test robot
  2024-03-25  5:44   ` Dan Carpenter
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-03-24  0:03 UTC (permalink / raw)
  To: Jinjiang Tu, akpm, david, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm
  Cc: llvm, oe-kbuild-all, tujinjiang

Hi Jinjiang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Jinjiang-Tu/mm-ksm-fix-ksm-exec-support-for-prctl/20240322-141317
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240322060947.3254967-2-tujinjiang%40huawei.com
patch subject: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
config: s390-allnoconfig (https://download.01.org/0day-ci/archive/20240324/202403240716.8B7CiDbr-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 23de3862dce582ce91c1aa914467d982cb1a73b4)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240324/202403240716.8B7CiDbr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403240716.8B7CiDbr-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from fs/exec.c:30:
   In file included from include/linux/mm.h:2211:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/exec.c:275:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     275 |         if (ksm_execve(mm))
         |             ^~~~~~~~~~~~~~
   fs/exec.c:305:9: note: uninitialized use occurs here
     305 |         return err;
         |                ^~~
   fs/exec.c:275:2: note: remove the 'if' if its condition is always false
     275 |         if (ksm_execve(mm))
         |         ^~~~~~~~~~~~~~~~~~~
     276 |                 goto err_ksm;
         |                 ~~~~~~~~~~~~
   fs/exec.c:257:9: note: initialize the variable 'err' to silence this warning
     257 |         int err;
         |                ^
         |                 = 0
   2 warnings generated.


vim +275 fs/exec.c

   254	
   255	static int __bprm_mm_init(struct linux_binprm *bprm)
   256	{
   257		int err;
   258		struct vm_area_struct *vma = NULL;
   259		struct mm_struct *mm = bprm->mm;
   260	
   261		bprm->vma = vma = vm_area_alloc(mm);
   262		if (!vma)
   263			return -ENOMEM;
   264		vma_set_anonymous(vma);
   265	
   266		if (mmap_write_lock_killable(mm)) {
   267			err = -EINTR;
   268			goto err_free;
   269		}
   270	
   271		/*
   272		 * Need to be called with mmap write lock
   273		 * held, to avoid race with ksmd.
   274		*/
 > 275		if (ksm_execve(mm))
   276			goto err_ksm;
   277	
   278		/*
   279		 * Place the stack at the largest stack address the architecture
   280		 * supports. Later, we'll move this to an appropriate place. We don't
   281		 * use STACK_TOP because that can depend on attributes which aren't
   282		 * configured yet.
   283		 */
   284		BUILD_BUG_ON(VM_STACK_FLAGS & VM_STACK_INCOMPLETE_SETUP);
   285		vma->vm_end = STACK_TOP_MAX;
   286		vma->vm_start = vma->vm_end - PAGE_SIZE;
   287		vm_flags_init(vma, VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP);
   288		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
   289	
   290		err = insert_vm_struct(mm, vma);
   291		if (err)
   292			goto err;
   293	
   294		mm->stack_vm = mm->total_vm = 1;
   295		mmap_write_unlock(mm);
   296		bprm->p = vma->vm_end - sizeof(void *);
   297		return 0;
   298	err:
   299		ksm_exit(mm);
   300	err_ksm:
   301		mmap_write_unlock(mm);
   302	err_free:
   303		bprm->vma = NULL;
   304		vm_area_free(vma);
   305		return err;
   306	}
   307	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-22  9:02   ` David Hildenbrand
@ 2024-03-25  2:24     ` Jinjiang Tu
  2024-03-25  8:33       ` David Hildenbrand
  0 siblings, 1 reply; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-25  2:24 UTC (permalink / raw)
  To: David Hildenbrand, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm


在 2024/3/22 17:02, David Hildenbrand 写道:
> On 22.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>
>> ---
>>   fs/exec.c           | 10 ++++++++++
>>   include/linux/ksm.h | 13 +++++++++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index ff6f26671cfc..66202d016a0a 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,13 @@ 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.
>> +    */
>> +    if (ksm_execve(mm))
>> +        goto err_ksm;
>> +
>
> But now, would we revert what insert_vm_struct() did?
>
> We're freeing the VMA later, but we might have accounted memory.
>
>
> What would be cleaner is doing the ksm_execve() before the 
> insert_vm_struct(), and then cleaning up in case insert_vm_struct() 
> failed.
In fact, ksm_execve() has been called before the insert_vm_struct() in 
this patch.


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec
  2024-03-22 11:43   ` David Hildenbrand
@ 2024-03-25  2:24     ` Jinjiang Tu
  2024-03-25  8:38       ` David Hildenbrand
  0 siblings, 1 reply; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-25  2:24 UTC (permalink / raw)
  To: David Hildenbrand, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm


在 2024/3/22 19:43, David Hildenbrand 写道:
> On 22.03.24 07:09, Jinjiang Tu wrote:
>> 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.
>>
>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ---
>>   .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
>>   1 file changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c 
>> b/tools/testing/selftests/mm/ksm_functional_tests.c
>> index d615767e396b..01999aab2e37 100644
>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>> @@ -146,6 +146,54 @@ static int ksm_unmerge(void)
>>       return 0;
>>   }
>>   +static int child_test_merge(void)
>> +{
>> +    const unsigned int size = 2 * MiB;
>> +    char *map;
>> +    int ret = -1;
>> +
>> +    /* Stabilize accounting by disabling KSM completely. */
>> +    if (ksm_unmerge()) {
>> +        ksft_print_msg("Disabling (unmerging) KSM failed\n");
>> +        return ret;
>> +    }
>> +
>> +    if (get_my_merging_pages() > 0) {
>> +        ksft_print_msg("Still pages merged\n");
>> +        return ret;
>> +    }
>> +
>> +    map = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +           MAP_PRIVATE|MAP_ANON, -1, 0);
>> +    if (map == MAP_FAILED) {
>> +        ksft_print_msg("mmap() failed\n");
>> +        return ret;
>> +    }
>> +
>> +    /* Don't use THP. Ignore if THP are not around on a kernel. */
>> +    if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
>> +        ksft_print_msg("MADV_NOHUGEPAGE failed\n");
>> +        goto unmap;
>> +    }
>> +
>> +    memset(map, 0x1c, size);
>> +
>> +    if (ksm_merge()) {
>> +        ksft_print_msg("Running KSM failed\n");
>> +        goto unmap;
>> +    }
>> +
>> +    if (get_my_merging_pages() <= 0) {
>> +        ksft_print_msg("Fail to merge\n");
>> +        goto unmap;
>> +    }
>
> Looks like all you want is use mmap_and_merge_range(), but neither 
> setting the prctl nor madvise().
>
> Two alternatives:
>
> 1) switching from "bool use_prctl" to an enum like
>
> enum ksm_merge_mode {
>     KSM_MERGE_PRCTL
>     KSM_MERGE_MADVISE,
>     KSM_MERGE_NONE, /* PRCTL already set */
> };
>
> Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
> PROT_READ|PROT_WRITE, KSM_MERGE_NONE);
I have considered this before. But, mmap_and_merge_range() calls 
ksft_test_result_fail() when error occurs, ksft_test_result_fail()
prints prefixed with ksft_fail count. When mmap_and_merge_range() is 
called in the child process, the ksft_fail isn't consisent with the
parent process due to the global variable ksft_fail is CoWed. As a 
result, ksft_print_msg() is intended to be called in child process.

Maybe, We could introduce a macro ksm_print() to control which function 
is called according to ksm_merge_mode :

#define ksm_print(mode, fmt, ...) do {    \
     if ((mode) == KSM_MERGE_NONE)                \
         ksft_print_msg(fmt, ##__VA_ARGS__);\
     else                        \
         ksft_test_result_fail(fmt, ##__VA_ARGS__);\
     } while (0)


>
> 2) With "bool use_prctl", before doing the prctl(PR_SET_MEMORY_MERGE, 
> ...), check if it is already enabled.
>
> As we do that already in ksm_fork_exec_child(), and fail if it isn't 
> set, that should work.
>
> Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB, 
> PROT_READ|PROT_WRITE, true);
>
> here.
>
>> +
>> +    ret = 0;
>> +unmap:
>> +    munmap(map, size);
>> +    return ret;
>> +}
>> +
>>   static char *mmap_and_merge_range(char val, unsigned long size, int 
>> prot,
>>                     bool use_prctl)
>>   {
>> @@ -458,7 +506,11 @@ static void test_prctl_fork(void)
>>         child_pid = fork();
>>       if (!child_pid) {
>> -        exit(prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0));
>> +        if (prctl(PR_GET_MEMORY_MERGE, 0, 0, 0, 0) != 1)
>> +            exit(-1);
>> +        if (child_test_merge() != 0)
>> +            exit(-2);
>> +        exit(0);
>>       } else if (child_pid < 0) {
>>           ksft_test_result_fail("fork() failed\n");
>>           return;
>> @@ -467,8 +519,14 @@ 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
>> +            ksft_test_result_fail("fail to merge in child\n");
>>           return;
>>       }
>>   @@ -483,7 +541,13 @@ static void test_prctl_fork(void)
>>   static int ksm_fork_exec_child(void)
>>   {
>>       /* 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;
>> +
>> +    if (child_test_merge() != 0)
>
> You can drop the "!=0". But maybe, you can just inline the call to 
> mmap_and_merge_range() here.
>
>> +        return -2;
>> +
>> +    return 0;
>
>


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
  2024-03-22  9:02   ` David Hildenbrand
  2024-03-24  0:03   ` kernel test robot
@ 2024-03-25  5:44   ` Dan Carpenter
  2024-03-25  6:33     ` Jinjiang Tu
  2 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2024-03-25  5:44 UTC (permalink / raw)
  To: oe-kbuild, Jinjiang Tu, akpm, david, shr, hannes, riel,
	wangkefeng.wang, sunnanyong, linux-mm
  Cc: lkp, oe-kbuild-all, tujinjiang

Hi Jinjiang,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Jinjiang-Tu/mm-ksm-fix-ksm-exec-support-for-prctl/20240322-141317
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240322060947.3254967-2-tujinjiang%40huawei.com
patch subject: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
config: openrisc-randconfig-r081-20240322 (https://download.01.org/0day-ci/archive/20240324/202403240146.Pv4gVc5N-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202403240146.Pv4gVc5N-lkp@intel.com/

smatch warnings:
fs/exec.c:305 __bprm_mm_init() error: uninitialized symbol 'err'.

vim +/err +305 fs/exec.c

b6a2fea39318e43 Ollie Wild                  2007-07-19  255  static int __bprm_mm_init(struct linux_binprm *bprm)
b6a2fea39318e43 Ollie Wild                  2007-07-19  256  {
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  257  	int err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  258  	struct vm_area_struct *vma = NULL;
b6a2fea39318e43 Ollie Wild                  2007-07-19  259  	struct mm_struct *mm = bprm->mm;
b6a2fea39318e43 Ollie Wild                  2007-07-19  260  
490fc053865c9cc Linus Torvalds              2018-07-21  261  	bprm->vma = vma = vm_area_alloc(mm);
b6a2fea39318e43 Ollie Wild                  2007-07-19  262  	if (!vma)
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  263  		return -ENOMEM;
bfd40eaff5abb9f Kirill A. Shutemov          2018-07-26  264  	vma_set_anonymous(vma);
b6a2fea39318e43 Ollie Wild                  2007-07-19  265  
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  266  	if (mmap_write_lock_killable(mm)) {
f268dfe905d4682 Michal Hocko                2016-05-23  267  		err = -EINTR;
f268dfe905d4682 Michal Hocko                2016-05-23  268  		goto err_free;
f268dfe905d4682 Michal Hocko                2016-05-23  269  	}
b6a2fea39318e43 Ollie Wild                  2007-07-19  270  
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  271  	/*
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  272  	 * Need to be called with mmap write lock
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  273  	 * held, to avoid race with ksmd.
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  274  	*/
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  275  	if (ksm_execve(mm))
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  276  		goto err_ksm;

"err" not set before the goto.

d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  277  
b6a2fea39318e43 Ollie Wild                  2007-07-19  278  	/*
b6a2fea39318e43 Ollie Wild                  2007-07-19  279  	 * Place the stack at the largest stack address the architecture
b6a2fea39318e43 Ollie Wild                  2007-07-19  280  	 * supports. Later, we'll move this to an appropriate place. We don't
b6a2fea39318e43 Ollie Wild                  2007-07-19  281  	 * use STACK_TOP because that can depend on attributes which aren't
b6a2fea39318e43 Ollie Wild                  2007-07-19  282  	 * configured yet.
b6a2fea39318e43 Ollie Wild                  2007-07-19  283  	 */
aacb3d17a73f644 Michal Hocko                2011-07-26  284  	BUILD_BUG_ON(VM_STACK_FLAGS & VM_STACK_INCOMPLETE_SETUP);
b6a2fea39318e43 Ollie Wild                  2007-07-19  285  	vma->vm_end = STACK_TOP_MAX;
b6a2fea39318e43 Ollie Wild                  2007-07-19  286  	vma->vm_start = vma->vm_end - PAGE_SIZE;
1c71222e5f2393b Suren Baghdasaryan          2023-01-26  287  	vm_flags_init(vma, VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP);
3ed75eb8f1cd895 Coly Li                     2007-10-18  288  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
462e635e5b73ba9 Tavis Ormandy               2010-12-09  289  
b6a2fea39318e43 Ollie Wild                  2007-07-19  290  	err = insert_vm_struct(mm, vma);
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  291  	if (err)
b6a2fea39318e43 Ollie Wild                  2007-07-19  292  		goto err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  293  
b6a2fea39318e43 Ollie Wild                  2007-07-19  294  	mm->stack_vm = mm->total_vm = 1;
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  295  	mmap_write_unlock(mm);
b6a2fea39318e43 Ollie Wild                  2007-07-19  296  	bprm->p = vma->vm_end - sizeof(void *);
b6a2fea39318e43 Ollie Wild                  2007-07-19  297  	return 0;
b6a2fea39318e43 Ollie Wild                  2007-07-19  298  err:
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  299  	ksm_exit(mm);
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  300  err_ksm:
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  301  	mmap_write_unlock(mm);
f268dfe905d4682 Michal Hocko                2016-05-23  302  err_free:
b6a2fea39318e43 Ollie Wild                  2007-07-19  303  	bprm->vma = NULL;
3928d4f5ee37cdc Linus Torvalds              2018-07-21  304  	vm_area_free(vma);
b6a2fea39318e43 Ollie Wild                  2007-07-19 @305  	return err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  306  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-25  5:44   ` Dan Carpenter
@ 2024-03-25  6:33     ` Jinjiang Tu
  0 siblings, 0 replies; 13+ messages in thread
From: Jinjiang Tu @ 2024-03-25  6:33 UTC (permalink / raw)
  To: Dan Carpenter, oe-kbuild, akpm, david, shr, hannes, riel,
	wangkefeng.wang, sunnanyong, linux-mm
  Cc: lkp, oe-kbuild-all


在 2024/3/25 13:44, Dan Carpenter 写道:
> Hi Jinjiang,
>
> kernel test robot noticed the following build warnings:
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jinjiang-Tu/mm-ksm-fix-ksm-exec-support-for-prctl/20240322-141317
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link:    https://lore.kernel.org/r/20240322060947.3254967-2-tujinjiang%40huawei.com
> patch subject: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
> config: openrisc-randconfig-r081-20240322 (https://download.01.org/0day-ci/archive/20240324/202403240146.Pv4gVc5N-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 13.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202403240146.Pv4gVc5N-lkp@intel.com/
>
> smatch warnings:
> fs/exec.c:305 __bprm_mm_init() error: uninitialized symbol 'err'.
>
> vim +/err +305 fs/exec.c
>
> b6a2fea39318e43 Ollie Wild                  2007-07-19  255  static int __bprm_mm_init(struct linux_binprm *bprm)
> b6a2fea39318e43 Ollie Wild                  2007-07-19  256  {
> eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  257  	int err;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  258  	struct vm_area_struct *vma = NULL;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  259  	struct mm_struct *mm = bprm->mm;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  260
> 490fc053865c9cc Linus Torvalds              2018-07-21  261  	bprm->vma = vma = vm_area_alloc(mm);
> b6a2fea39318e43 Ollie Wild                  2007-07-19  262  	if (!vma)
> eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  263  		return -ENOMEM;
> bfd40eaff5abb9f Kirill A. Shutemov          2018-07-26  264  	vma_set_anonymous(vma);
> b6a2fea39318e43 Ollie Wild                  2007-07-19  265
> d8ed45c5dcd455f Michel Lespinasse           2020-06-08  266  	if (mmap_write_lock_killable(mm)) {
> f268dfe905d4682 Michal Hocko                2016-05-23  267  		err = -EINTR;
> f268dfe905d4682 Michal Hocko                2016-05-23  268  		goto err_free;
> f268dfe905d4682 Michal Hocko                2016-05-23  269  	}
> b6a2fea39318e43 Ollie Wild                  2007-07-19  270
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  271  	/*
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  272  	 * Need to be called with mmap write lock
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  273  	 * held, to avoid race with ksmd.
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  274  	*/
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  275  	if (ksm_execve(mm))
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  276  		goto err_ksm;
>
> "err" not set before the goto.

The code should be:

err = ksm_execve(mm);
if (err)
     goto err_ksm;

I will fix in the next version.

>
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  277
> b6a2fea39318e43 Ollie Wild                  2007-07-19  278  	/*
> b6a2fea39318e43 Ollie Wild                  2007-07-19  279  	 * Place the stack at the largest stack address the architecture
> b6a2fea39318e43 Ollie Wild                  2007-07-19  280  	 * supports. Later, we'll move this to an appropriate place. We don't
> b6a2fea39318e43 Ollie Wild                  2007-07-19  281  	 * use STACK_TOP because that can depend on attributes which aren't
> b6a2fea39318e43 Ollie Wild                  2007-07-19  282  	 * configured yet.
> b6a2fea39318e43 Ollie Wild                  2007-07-19  283  	 */
> aacb3d17a73f644 Michal Hocko                2011-07-26  284  	BUILD_BUG_ON(VM_STACK_FLAGS & VM_STACK_INCOMPLETE_SETUP);
> b6a2fea39318e43 Ollie Wild                  2007-07-19  285  	vma->vm_end = STACK_TOP_MAX;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  286  	vma->vm_start = vma->vm_end - PAGE_SIZE;
> 1c71222e5f2393b Suren Baghdasaryan          2023-01-26  287  	vm_flags_init(vma, VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP);
> 3ed75eb8f1cd895 Coly Li                     2007-10-18  288  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> 462e635e5b73ba9 Tavis Ormandy               2010-12-09  289
> b6a2fea39318e43 Ollie Wild                  2007-07-19  290  	err = insert_vm_struct(mm, vma);
> eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  291  	if (err)
> b6a2fea39318e43 Ollie Wild                  2007-07-19  292  		goto err;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  293
> b6a2fea39318e43 Ollie Wild                  2007-07-19  294  	mm->stack_vm = mm->total_vm = 1;
> d8ed45c5dcd455f Michel Lespinasse           2020-06-08  295  	mmap_write_unlock(mm);
> b6a2fea39318e43 Ollie Wild                  2007-07-19  296  	bprm->p = vma->vm_end - sizeof(void *);
> b6a2fea39318e43 Ollie Wild                  2007-07-19  297  	return 0;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  298  err:
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  299  	ksm_exit(mm);
> d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  300  err_ksm:
> d8ed45c5dcd455f Michel Lespinasse           2020-06-08  301  	mmap_write_unlock(mm);
> f268dfe905d4682 Michal Hocko                2016-05-23  302  err_free:
> b6a2fea39318e43 Ollie Wild                  2007-07-19  303  	bprm->vma = NULL;
> 3928d4f5ee37cdc Linus Torvalds              2018-07-21  304  	vm_area_free(vma);
> b6a2fea39318e43 Ollie Wild                  2007-07-19 @305  	return err;
> b6a2fea39318e43 Ollie Wild                  2007-07-19  306  }
>

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
  2024-03-25  2:24     ` Jinjiang Tu
@ 2024-03-25  8:33       ` David Hildenbrand
  0 siblings, 0 replies; 13+ messages in thread
From: David Hildenbrand @ 2024-03-25  8:33 UTC (permalink / raw)
  To: Jinjiang Tu, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm

On 25.03.24 03:24, Jinjiang Tu wrote:
> 
> 在 2024/3/22 17:02, David Hildenbrand 写道:
>> On 22.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>
>>> ---
>>>    fs/exec.c           | 10 ++++++++++
>>>    include/linux/ksm.h | 13 +++++++++++++
>>>    2 files changed, 23 insertions(+)
>>>
>>> diff --git a/fs/exec.c b/fs/exec.c
>>> index ff6f26671cfc..66202d016a0a 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,13 @@ 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.
>>> +    */
>>> +    if (ksm_execve(mm))
>>> +        goto err_ksm;
>>> +
>>
>> But now, would we revert what insert_vm_struct() did?
>>
>> We're freeing the VMA later, but we might have accounted memory.
>>
>>
>> What would be cleaner is doing the ksm_execve() before the
>> insert_vm_struct(), and then cleaning up in case insert_vm_struct()
>> failed.
> In fact, ksm_execve() has been called before the insert_vm_struct() in
> this patch.
> 

Ahh, I missed that. Indeed, that works then.

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec
  2024-03-25  2:24     ` Jinjiang Tu
@ 2024-03-25  8:38       ` David Hildenbrand
  0 siblings, 0 replies; 13+ messages in thread
From: David Hildenbrand @ 2024-03-25  8:38 UTC (permalink / raw)
  To: Jinjiang Tu, akpm, shr, hannes, riel, wangkefeng.wang,
	sunnanyong, linux-mm

On 25.03.24 03:24, Jinjiang Tu wrote:
> 
> 在 2024/3/22 19:43, David Hildenbrand 写道:
>> On 22.03.24 07:09, Jinjiang Tu wrote:
>>> 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.
>>>
>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>>> ---
>>>    .../selftests/mm/ksm_functional_tests.c       | 79 +++++++++++++++++--
>>>    1 file changed, 73 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c
>>> b/tools/testing/selftests/mm/ksm_functional_tests.c
>>> index d615767e396b..01999aab2e37 100644
>>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>>> @@ -146,6 +146,54 @@ static int ksm_unmerge(void)
>>>        return 0;
>>>    }
>>>    +static int child_test_merge(void)
>>> +{
>>> +    const unsigned int size = 2 * MiB;
>>> +    char *map;
>>> +    int ret = -1;
>>> +
>>> +    /* Stabilize accounting by disabling KSM completely. */
>>> +    if (ksm_unmerge()) {
>>> +        ksft_print_msg("Disabling (unmerging) KSM failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (get_my_merging_pages() > 0) {
>>> +        ksft_print_msg("Still pages merged\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    map = mmap(NULL, size, PROT_READ|PROT_WRITE,
>>> +           MAP_PRIVATE|MAP_ANON, -1, 0);
>>> +    if (map == MAP_FAILED) {
>>> +        ksft_print_msg("mmap() failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* Don't use THP. Ignore if THP are not around on a kernel. */
>>> +    if (madvise(map, size, MADV_NOHUGEPAGE) && errno != EINVAL) {
>>> +        ksft_print_msg("MADV_NOHUGEPAGE failed\n");
>>> +        goto unmap;
>>> +    }
>>> +
>>> +    memset(map, 0x1c, size);
>>> +
>>> +    if (ksm_merge()) {
>>> +        ksft_print_msg("Running KSM failed\n");
>>> +        goto unmap;
>>> +    }
>>> +
>>> +    if (get_my_merging_pages() <= 0) {
>>> +        ksft_print_msg("Fail to merge\n");
>>> +        goto unmap;
>>> +    }
>>
>> Looks like all you want is use mmap_and_merge_range(), but neither
>> setting the prctl nor madvise().
>>
>> Two alternatives:
>>
>> 1) switching from "bool use_prctl" to an enum like
>>
>> enum ksm_merge_mode {
>>      KSM_MERGE_PRCTL
>>      KSM_MERGE_MADVISE,
>>      KSM_MERGE_NONE, /* PRCTL already set */
>> };
>>
>> Then, you can simply use mmap_and_merge_range(0x1c, 2 * MiB,
>> PROT_READ|PROT_WRITE, KSM_MERGE_NONE);
> I have considered this before. But, mmap_and_merge_range() calls
> ksft_test_result_fail() when error occurs, ksft_test_result_fail()
> prints prefixed with ksft_fail count. When mmap_and_merge_range() is
> called in the child process, the ksft_fail isn't consisent with the
> parent process due to the global variable ksft_fail is CoWed. As a
> result, ksft_print_msg() is intended to be called in child process.
> 
> Maybe, We could introduce a macro ksm_print() to control which function
> is called according to ksm_merge_mode :

As an alternative, convert all ksft_test_result_fail() in there into 
ksft_print_msg(), and in the callers of mmap_and_merge_range(), do 
something like

map = mmap_and_merge_range() ...
if (map == MAP_FAILED) {
	ksft_test_result_fail("Merging memory failed");
	return;
}


-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
@ 2024-03-23 17:24 kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-03-23 17:24 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240322060947.3254967-2-tujinjiang@huawei.com>
References: <20240322060947.3254967-2-tujinjiang@huawei.com>
TO: Jinjiang Tu <tujinjiang@huawei.com>
TO: akpm@linux-foundation.org
TO: david@redhat.com
TO: shr@devkernel.io
TO: hannes@cmpxchg.org
TO: riel@surriel.com
TO: wangkefeng.wang@huawei.com
TO: sunnanyong@huawei.com
TO: linux-mm@kvack.org
CC: tujinjiang@huawei.com

Hi Jinjiang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Jinjiang-Tu/mm-ksm-fix-ksm-exec-support-for-prctl/20240322-141317
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240322060947.3254967-2-tujinjiang%40huawei.com
patch subject: [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl
:::::: branch date: 35 hours ago
:::::: commit date: 35 hours ago
config: openrisc-randconfig-r081-20240322 (https://download.01.org/0day-ci/archive/20240324/202403240146.Pv4gVc5N-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202403240146.Pv4gVc5N-lkp@intel.com/

smatch warnings:
fs/exec.c:305 __bprm_mm_init() error: uninitialized symbol 'err'.

vim +/err +305 fs/exec.c

b6a2fea39318e43 Ollie Wild                  2007-07-19  254  
b6a2fea39318e43 Ollie Wild                  2007-07-19  255  static int __bprm_mm_init(struct linux_binprm *bprm)
b6a2fea39318e43 Ollie Wild                  2007-07-19  256  {
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  257  	int err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  258  	struct vm_area_struct *vma = NULL;
b6a2fea39318e43 Ollie Wild                  2007-07-19  259  	struct mm_struct *mm = bprm->mm;
b6a2fea39318e43 Ollie Wild                  2007-07-19  260  
490fc053865c9cc Linus Torvalds              2018-07-21  261  	bprm->vma = vma = vm_area_alloc(mm);
b6a2fea39318e43 Ollie Wild                  2007-07-19  262  	if (!vma)
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  263  		return -ENOMEM;
bfd40eaff5abb9f Kirill A. Shutemov          2018-07-26  264  	vma_set_anonymous(vma);
b6a2fea39318e43 Ollie Wild                  2007-07-19  265  
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  266  	if (mmap_write_lock_killable(mm)) {
f268dfe905d4682 Michal Hocko                2016-05-23  267  		err = -EINTR;
f268dfe905d4682 Michal Hocko                2016-05-23  268  		goto err_free;
f268dfe905d4682 Michal Hocko                2016-05-23  269  	}
b6a2fea39318e43 Ollie Wild                  2007-07-19  270  
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  271  	/*
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  272  	 * Need to be called with mmap write lock
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  273  	 * held, to avoid race with ksmd.
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  274  	*/
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  275  	if (ksm_execve(mm))
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  276  		goto err_ksm;
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  277  
b6a2fea39318e43 Ollie Wild                  2007-07-19  278  	/*
b6a2fea39318e43 Ollie Wild                  2007-07-19  279  	 * Place the stack at the largest stack address the architecture
b6a2fea39318e43 Ollie Wild                  2007-07-19  280  	 * supports. Later, we'll move this to an appropriate place. We don't
b6a2fea39318e43 Ollie Wild                  2007-07-19  281  	 * use STACK_TOP because that can depend on attributes which aren't
b6a2fea39318e43 Ollie Wild                  2007-07-19  282  	 * configured yet.
b6a2fea39318e43 Ollie Wild                  2007-07-19  283  	 */
aacb3d17a73f644 Michal Hocko                2011-07-26  284  	BUILD_BUG_ON(VM_STACK_FLAGS & VM_STACK_INCOMPLETE_SETUP);
b6a2fea39318e43 Ollie Wild                  2007-07-19  285  	vma->vm_end = STACK_TOP_MAX;
b6a2fea39318e43 Ollie Wild                  2007-07-19  286  	vma->vm_start = vma->vm_end - PAGE_SIZE;
1c71222e5f2393b Suren Baghdasaryan          2023-01-26  287  	vm_flags_init(vma, VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP);
3ed75eb8f1cd895 Coly Li                     2007-10-18  288  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
462e635e5b73ba9 Tavis Ormandy               2010-12-09  289  
b6a2fea39318e43 Ollie Wild                  2007-07-19  290  	err = insert_vm_struct(mm, vma);
eaccbfa564e48c8 Luiz Fernando N. Capitulino 2009-01-06  291  	if (err)
b6a2fea39318e43 Ollie Wild                  2007-07-19  292  		goto err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  293  
b6a2fea39318e43 Ollie Wild                  2007-07-19  294  	mm->stack_vm = mm->total_vm = 1;
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  295  	mmap_write_unlock(mm);
b6a2fea39318e43 Ollie Wild                  2007-07-19  296  	bprm->p = vma->vm_end - sizeof(void *);
b6a2fea39318e43 Ollie Wild                  2007-07-19  297  	return 0;
b6a2fea39318e43 Ollie Wild                  2007-07-19  298  err:
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  299  	ksm_exit(mm);
d282f6b19afd1a9 Jinjiang Tu                 2024-03-22  300  err_ksm:
d8ed45c5dcd455f Michel Lespinasse           2020-06-08  301  	mmap_write_unlock(mm);
f268dfe905d4682 Michal Hocko                2016-05-23  302  err_free:
b6a2fea39318e43 Ollie Wild                  2007-07-19  303  	bprm->vma = NULL;
3928d4f5ee37cdc Linus Torvalds              2018-07-21  304  	vm_area_free(vma);
b6a2fea39318e43 Ollie Wild                  2007-07-19 @305  	return err;
b6a2fea39318e43 Ollie Wild                  2007-07-19  306  }
b6a2fea39318e43 Ollie Wild                  2007-07-19  307  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-03-25  8:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-22  6:09 [PATCH v2 0/2] mm/ksm: fix ksm exec support for prctl Jinjiang Tu
2024-03-22  6:09 ` [PATCH v2 1/2] " Jinjiang Tu
2024-03-22  9:02   ` David Hildenbrand
2024-03-25  2:24     ` Jinjiang Tu
2024-03-25  8:33       ` David Hildenbrand
2024-03-24  0:03   ` kernel test robot
2024-03-25  5:44   ` Dan Carpenter
2024-03-25  6:33     ` Jinjiang Tu
2024-03-22  6:09 ` [PATCH v2 2/2] selftest/mm: ksm_functional_tests: extend test case for ksm fork/exec Jinjiang Tu
2024-03-22 11:43   ` David Hildenbrand
2024-03-25  2:24     ` Jinjiang Tu
2024-03-25  8:38       ` David Hildenbrand
2024-03-23 17:24 [PATCH v2 1/2] mm/ksm: fix ksm exec support for prctl kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.