Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1472411 > unrolled thread

[PATCH 0/4] mm: mlock: fix some locked_vm counting issues

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] mm: mlock: fix some locked_vm counting issues wei.guo.simon@gmail.com - 2016-08-30 13:10 +0200
    [PATCH 1/4] mm: mlock: check against vma for actual mlock() size wei.guo.simon@gmail.com - 2016-08-30 13:10 +0200
      Re: [PATCH 1/4] mm: mlock: check against vma for actual mlock() size "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-08-30 13:40 +0200
    [PATCH 3/4] selftest: split mlock2_ funcs into separate mlock2.h wei.guo.simon@gmail.com - 2016-08-30 13:10 +0200
    [PATCH 2/4] mm: mlock: avoid increase mm->locked_vm on mlock() when already mlock2(,MLOCK_ONFAULT) wei.guo.simon@gmail.com - 2016-08-30 13:10 +0200
      Re: [PATCH 2/4] mm: mlock: avoid increase mm->locked_vm on mlock()  when already mlock2(,MLOCK_ONFAULT) "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-08-30 13:40 +0200
    [PATCH 4/4] selftests/vm: add test for mlock() when areas are intersected. wei.guo.simon@gmail.com - 2016-08-30 13:10 +0200
      Re: [PATCH 4/4] selftests/vm: add test for mlock() when areas are  intersected. David Rientjes <rientjes@google.com> - 2016-09-01 01:20 +0200
        Re: [PATCH 4/4] selftests/vm: add test for mlock() when areas are  intersected. Simon Guo <wei.guo.simon@gmail.com> - 2016-09-01 09:20 +0200

#1472411 — [PATCH 0/4] mm: mlock: fix some locked_vm counting issues

From: Simon Guo <wei.guo.simon@gmail.com>

This patch set fixes some mlock() misbehavior when mlock()/mlock2() 
is invoked multiple times on intersect or same address regions.

And add selftest for this case.

Simon Guo (4):
  mm: mlock: check against vma for actual mlock() size
  mm: mlock: avoid increase mm->locked_vm on mlock() when already
    mlock2(,MLOCK_ONFAULT)
  selftest: split mlock2_ apis into separate mlock2.h
  selftests/vm: add test for mlock() when areas are intersected.

 mm/mlock.c                                        | 53 ++++++++++++++++
 tools/testing/selftests/vm/.gitignore             |  1 +
 tools/testing/selftests/vm/Makefile               |  4 ++
 tools/testing/selftests/vm/mlock-intersect-test.c | 76 +++++++++++++++++++++++
 tools/testing/selftests/vm/mlock2-tests.c         | 21 +------
 tools/testing/selftests/vm/mlock2.h               | 21 +++++++
 6 files changed, 156 insertions(+), 20 deletions(-)
 create mode 100644 tools/testing/selftests/vm/mlock-intersect-test.c
 create mode 100644 tools/testing/selftests/vm/mlock2.h

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1472412 — [PATCH 1/4] mm: mlock: check against vma for actual mlock() size

From: Simon Guo <wei.guo.simon@gmail.com>

In do_mlock(), the check against locked memory limitation
has a hole which will fail following cases at step 3):
1) User has a memory chunk from addressA with 50k, and user
mem lock rlimit is 64k.
2) mlock(addressA, 30k)
3) mlock(addressA, 40k)

The 3rd step should have been allowed since the 40k request
is intersected with the previous 30k at step 2), and the
3rd step is actually for mlock on the extra 10k memory.

This patch checks vma to caculate the actual "new" mlock
size, if necessary, and ajust the logic to fix this issue.

Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 mm/mlock.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/mm/mlock.c b/mm/mlock.c
index 14645be..9283187 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -617,6 +617,43 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
 	return error;
 }
 
+/*
+ * Go through vma areas and sum size of mlocked
+ * vma pages, as return value.
+ * Note deferred memory locking case(mlock2(,,MLOCK_ONFAULT)
+ * is also counted.
+ * Return value: previously mlocked page counts
+ */
+static int count_mm_mlocked_page_nr(struct mm_struct *mm,
+		unsigned long start, size_t len)
+{
+	struct vm_area_struct *vma;
+	int count = 0;
+
+	if (mm == NULL)
+		mm = current->mm;
+
+	vma = find_vma(mm, start);
+	if (vma == NULL)
+		vma = mm->mmap;
+
+	for (; vma ; vma = vma->vm_next) {
+		if (start + len <=  vma->vm_start)
+			break;
+		if (vma->vm_flags && VM_LOCKED) {
+			if (start > vma->vm_start)
+				count -= (start - vma->vm_start);
+			if (start + len < vma->vm_end) {
+				count += start + len - vma->vm_start;
+				break;
+			}
+			count += vma->vm_end - vma->vm_start;
+		}
+	}
+
+	return (PAGE_ALIGN(count) >> PAGE_SHIFT);
+}
+
 static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t flags)
 {
 	unsigned long locked;
@@ -639,6 +676,18 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
 		return -EINTR;
 
 	locked += current->mm->locked_vm;
+	if ((locked > lock_limit) && (!capable(CAP_IPC_LOCK))) {
+		/*
+		 * It is possible that the regions requested
+		 * intersect with previously mlocked areas,
+		 * that part area in "mm->locked_vm" should
+		 * not be counted to new mlock increment
+		 * count. So check and adjust locked count
+		 * if necessary.
+		 */
+		locked -= count_mm_mlocked_page_nr(current->mm,
+				start, len);
+	}
 
 	/* check against resource limits */
 	if ((locked <= lock_limit) || capable(CAP_IPC_LOCK))
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1472426 — Re: [PATCH 1/4] mm: mlock: check against vma for actual mlock() size

On Tue, Aug 30, 2016 at 06:59:38PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
> 
> In do_mlock(), the check against locked memory limitation
> has a hole which will fail following cases at step 3):
> 1) User has a memory chunk from addressA with 50k, and user
> mem lock rlimit is 64k.
> 2) mlock(addressA, 30k)
> 3) mlock(addressA, 40k)
> 
> The 3rd step should have been allowed since the 40k request
> is intersected with the previous 30k at step 2), and the
> 3rd step is actually for mlock on the extra 10k memory.
> 
> This patch checks vma to caculate the actual "new" mlock
> size, if necessary, and ajust the logic to fix this issue.
> 
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>

Looks reasonable to me. Few nitpicks below.

> ---
>  mm/mlock.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 14645be..9283187 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -617,6 +617,43 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>  	return error;
>  }
>  
> +/*
> + * Go through vma areas and sum size of mlocked
> + * vma pages, as return value.
> + * Note deferred memory locking case(mlock2(,,MLOCK_ONFAULT)
> + * is also counted.
> + * Return value: previously mlocked page counts
> + */
> +static int count_mm_mlocked_page_nr(struct mm_struct *mm,
> +		unsigned long start, size_t len)
> +{
> +	struct vm_area_struct *vma;
> +	int count = 0;
> +
> +	if (mm == NULL)
> +		mm = current->mm;
> +
> +	vma = find_vma(mm, start);
> +	if (vma == NULL)
> +		vma = mm->mmap;
> +
> +	for (; vma ; vma = vma->vm_next) {
> +		if (start + len <=  vma->vm_start)
> +			break;

	for (; vma && start + len <= vma->vm_start; vma = vma->vm_next) {

> +		if (vma->vm_flags && VM_LOCKED) {
> +			if (start > vma->vm_start)
> +				count -= (start - vma->vm_start);
> +			if (start + len < vma->vm_end) {
> +				count += start + len - vma->vm_start;
> +				break;
> +			}
> +			count += vma->vm_end - vma->vm_start;
> +		}
> +	}
> +
> +	return (PAGE_ALIGN(count) >> PAGE_SHIFT);

Redundant parenthesis.

And do we need PAGE_ALIGN() here? Caller already aligned 'len', and vma
boundaries are alinged.

> +}
> +
>  static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t flags)
>  {
>  	unsigned long locked;
> @@ -639,6 +676,18 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>  		return -EINTR;
>  
>  	locked += current->mm->locked_vm;
> +	if ((locked > lock_limit) && (!capable(CAP_IPC_LOCK))) {
> +		/*
> +		 * It is possible that the regions requested
> +		 * intersect with previously mlocked areas,
> +		 * that part area in "mm->locked_vm" should
> +		 * not be counted to new mlock increment
> +		 * count. So check and adjust locked count
> +		 * if necessary.
> +		 */
> +		locked -= count_mm_mlocked_page_nr(current->mm,
> +				start, len);
> +	}
>  
>  	/* check against resource limits */
>  	if ((locked <= lock_limit) || capable(CAP_IPC_LOCK))
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1472413 — [PATCH 3/4] selftest: split mlock2_ funcs into separate mlock2.h

From: Simon Guo <wei.guo.simon@gmail.com>

To prepare mlock2.h whose functionality will be reused.

Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 tools/testing/selftests/vm/mlock2-tests.c | 21 +--------------------
 tools/testing/selftests/vm/mlock2.h       | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 20 deletions(-)
 create mode 100644 tools/testing/selftests/vm/mlock2.h

diff --git a/tools/testing/selftests/vm/mlock2-tests.c b/tools/testing/selftests/vm/mlock2-tests.c
index 02ca5e0..7cb13ce 100644
--- a/tools/testing/selftests/vm/mlock2-tests.c
+++ b/tools/testing/selftests/vm/mlock2-tests.c
@@ -7,27 +7,8 @@
 #include <string.h>
 #include <sys/time.h>
 #include <sys/resource.h>
-#include <syscall.h>
-#include <errno.h>
 #include <stdbool.h>
-
-#ifndef MLOCK_ONFAULT
-#define MLOCK_ONFAULT 1
-#endif
-
-#ifndef MCL_ONFAULT
-#define MCL_ONFAULT (MCL_FUTURE << 1)
-#endif
-
-static int mlock2_(void *start, size_t len, int flags)
-{
-#ifdef __NR_mlock2
-	return syscall(__NR_mlock2, start, len, flags);
-#else
-	errno = ENOSYS;
-	return -1;
-#endif
-}
+#include "mlock2.h"
 
 struct vm_boundaries {
 	unsigned long start;
diff --git a/tools/testing/selftests/vm/mlock2.h b/tools/testing/selftests/vm/mlock2.h
new file mode 100644
index 0000000..b9c6d9f
--- /dev/null
+++ b/tools/testing/selftests/vm/mlock2.h
@@ -0,0 +1,20 @@
+#include <syscall.h>
+#include <errno.h>
+
+#ifndef MLOCK_ONFAULT
+#define MLOCK_ONFAULT 1
+#endif
+
+#ifndef MCL_ONFAULT
+#define MCL_ONFAULT (MCL_FUTURE << 1)
+#endif
+
+static int mlock2_(void *start, size_t len, int flags)
+{
+#ifdef __NR_mlock2
+	return syscall(__NR_mlock2, start, len, flags);
+#else
+	errno = ENOSYS;
+	return -1;
+#endif
+}
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1472416 — [PATCH 2/4] mm: mlock: avoid increase mm->locked_vm on mlock() when already mlock2(,MLOCK_ONFAULT)

From: Simon Guo <wei.guo.simon@gmail.com>

When one vma was with flag VM_LOCKED|VM_LOCKONFAULT (by invoking
mlock2(,MLOCK_ONFAULT)), it can again be populated with mlock() with
VM_LOCKED flag only.

There is a hole in mlock_fixup() which increase mm->locked_vm twice even
the two operations are on the same vma and both with VM_LOCKED flags.

The issue can be reproduced by following code:
mlock2(p, 1024 * 64, MLOCK_ONFAULT); //VM_LOCKED|VM_LOCKONFAULT
mlock(p, 1024 * 64);  //VM_LOCKED
Then check the increase VmLck field in /proc/pid/status(to 128k).

When vma is set with different vm_flags, and the new vm_flags is with
VM_LOCKED, it is not necessarily be a "new locked" vma.  This patch
corrects this bug by prevent mm->locked_vm from increment when old
vm_flags is already VM_LOCKED.

Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 mm/mlock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/mlock.c b/mm/mlock.c
index 9283187..df29aad 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -516,6 +516,7 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev,
 	int nr_pages;
 	int ret = 0;
 	int lock = !!(newflags & VM_LOCKED);
+	vm_flags_t old_flags = vma->vm_flags;
 
 	if (newflags == vma->vm_flags || (vma->vm_flags & VM_SPECIAL) ||
 	    is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm))
@@ -550,6 +551,8 @@ success:
 	nr_pages = (end - start) >> PAGE_SHIFT;
 	if (!lock)
 		nr_pages = -nr_pages;
+	else if (old_flags & VM_LOCKED)
+		nr_pages = 0;
 	mm->locked_vm += nr_pages;
 
 	/*
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1472427 — Re: [PATCH 2/4] mm: mlock: avoid increase mm->locked_vm on mlock() when already mlock2(,MLOCK_ONFAULT)

On Tue, Aug 30, 2016 at 06:59:39PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
> 
> When one vma was with flag VM_LOCKED|VM_LOCKONFAULT (by invoking
> mlock2(,MLOCK_ONFAULT)), it can again be populated with mlock() with
> VM_LOCKED flag only.
> 
> There is a hole in mlock_fixup() which increase mm->locked_vm twice even
> the two operations are on the same vma and both with VM_LOCKED flags.
> 
> The issue can be reproduced by following code:
> mlock2(p, 1024 * 64, MLOCK_ONFAULT); //VM_LOCKED|VM_LOCKONFAULT
> mlock(p, 1024 * 64);  //VM_LOCKED
> Then check the increase VmLck field in /proc/pid/status(to 128k).
> 
> When vma is set with different vm_flags, and the new vm_flags is with
> VM_LOCKED, it is not necessarily be a "new locked" vma.  This patch
> corrects this bug by prevent mm->locked_vm from increment when old
> vm_flags is already VM_LOCKED.
> 
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1472417 — [PATCH 4/4] selftests/vm: add test for mlock() when areas are intersected.

From: Simon Guo <wei.guo.simon@gmail.com>

This patch adds mlock() test for multiple invocation on
the same address area, and verify it doesn't mess the
rlimit mlock limitation.

Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
---
 tools/testing/selftests/vm/.gitignore             |  1 +
 tools/testing/selftests/vm/Makefile               |  4 ++
 tools/testing/selftests/vm/mlock-intersect-test.c | 76 +++++++++++++++++++++++
 3 files changed, 81 insertions(+)
 create mode 100644 tools/testing/selftests/vm/mlock-intersect-test.c

diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
index a937a9d..142c565 100644
--- a/tools/testing/selftests/vm/.gitignore
+++ b/tools/testing/selftests/vm/.gitignore
@@ -7,3 +7,4 @@ mlock2-tests
 on-fault-limit
 transhuge-stress
 userfaultfd
+mlock-intersect-test
diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index e4bb1de..a0412a8 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -10,6 +10,7 @@ BINARIES += on-fault-limit
 BINARIES += thuge-gen
 BINARIES += transhuge-stress
 BINARIES += userfaultfd
+BINARIES += mlock-intersect-test
 
 all: $(BINARIES)
 %: %.c
@@ -17,6 +18,9 @@ all: $(BINARIES)
 userfaultfd: userfaultfd.c ../../../../usr/include/linux/kernel.h
 	$(CC) $(CFLAGS) -O2 -o $@ $< -lpthread
 
+mlock-intersect-test: mlock-intersect-test.c
+	$(CC) $(CFLAGS) -o $@ $< -lcap
+
 ../../../../usr/include/linux/kernel.h:
 	make -C ../../../.. headers_install
 
diff --git a/tools/testing/selftests/vm/mlock-intersect-test.c b/tools/testing/selftests/vm/mlock-intersect-test.c
new file mode 100644
index 0000000..f78e68a
--- /dev/null
+++ b/tools/testing/selftests/vm/mlock-intersect-test.c
@@ -0,0 +1,76 @@
+/*
+ * It tests the duplicate mlock result:
+ * - the ulimit of lock page is 64k
+ * - allocate address area 64k starting from p
+ * - mlock   [p -- p + 30k]
+ * - Then mlock address  [ p -- p + 40k ]
+ *
+ * It should succeed since totally we locked
+ * 40k < 64k limitation.
+ *
+ * It should not be run with CAP_IPC_LOCK.
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/resource.h>
+#include <sys/capability.h>
+#include <sys/mman.h>
+#include "mlock2.h"
+
+int main(int argc, char **argv)
+{
+	struct rlimit new;
+	char *p = NULL;
+	cap_t cap = cap_init();
+	int i;
+
+	/* drop capabilities including CAP_IPC_LOCK */
+	if (cap_set_proc(cap))
+		return -1;
+
+	/* set mlock limits to 64k */
+	new.rlim_cur = 65536;
+	new.rlim_max = 65536;
+	setrlimit(RLIMIT_MEMLOCK, &new);
+
+	/* test VM_LOCK */
+	p = malloc(1024 * 64);
+	if (mlock(p, 1024 * 30)) {
+		printf("mlock() 30k return failure.\n");
+		return -1;
+	}
+	for (i = 0; i < 10; i++) {
+		if (mlock(p, 1024 * 40)) {
+			printf("mlock() #%d 40k returns failure.\n", i);
+			return -1;
+		}
+	}
+	for (i = 0; i < 10; i++) {
+		if (mlock2_(p, 1024 * 40, MLOCK_ONFAULT)) {
+			printf("mlock2_() #%d 40k returns failure.\n", i);
+			return -1;
+		}
+	}
+	free(p);
+
+	/* Test VM_LOCKONFAULT */
+	p = malloc(1024 * 64);
+	if (mlock2_(p, 1024 * 30, MLOCK_ONFAULT)) {
+		printf("mlock2_() 30k return failure.\n");
+		return -1;
+	}
+	for (i = 0; i < 10; i++) {
+		if (mlock2_(p, 1024 * 40, MLOCK_ONFAULT)) {
+			printf("mlock2_() #%d 40k returns failure.\n", i);
+			return -1;
+		}
+	}
+	for (i = 0; i < 10; i++) {
+		if (mlock(p, 1024 * 40)) {
+			printf("mlock() #%d 40k returns failure.\n", i);
+			return -1;
+		}
+	}
+	return 0;
+}
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1473960 — Re: [PATCH 4/4] selftests/vm: add test for mlock() when areas are intersected.

On Tue, 30 Aug 2016, wei.guo.simon@gmail.com wrote:

> From: Simon Guo <wei.guo.simon@gmail.com>
> 
> This patch adds mlock() test for multiple invocation on
> the same address area, and verify it doesn't mess the
> rlimit mlock limitation.
> 

Thanks for expanding mlock testing.  I'm wondering if you are interested 
in more elaborate testing that doesn't only check what you are fixing, 
such as mlock(p + x, 40k) where x is < 20k?

Would you also be willing to make sure that the rlimit is actually 
enforced when it's expected to?

[toc] | [prev] | [next] | [standalone]

#1474117 — Re: [PATCH 4/4] selftests/vm: add test for mlock() when areas are intersected.

Hi David,
On Wed, Aug 31, 2016 at 04:14:14PM -0700, David Rientjes wrote:
> On Tue, 30 Aug 2016, wei.guo.simon@gmail.com wrote:
> 
> > From: Simon Guo <wei.guo.simon@gmail.com>
> > 
> > This patch adds mlock() test for multiple invocation on
> > the same address area, and verify it doesn't mess the
> > rlimit mlock limitation.
> > 
> 
> Thanks for expanding mlock testing.  I'm wondering if you are interested 
> in more elaborate testing that doesn't only check what you are fixing, 
> such as mlock(p + x, 40k) where x is < 20k?
> 
> Would you also be willing to make sure that the rlimit is actually 
> enforced when it's expected to?
I'd like to do so. 
Let me think more for the comprehensive testing. If you have any other
test cases in mind, please let me know.

Thanks,
- Simon

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web