mm: rearrange madvise code to allow for reuse

PATCH Superseded Colin Cross 3 participants 6 messages Oct 1, 2013 – Oct 2, 2013

Subsystems (7)

Memory management Documentation MM core Proc filesystem +3 more subsystems

lore

Share & tools

lore

Colin Cross

author

2013-10-01 20:21 UTC #0 lore raw

PATCH v2 1/2 mm: rearrange madvise code to allow for reuse

This patch refactors the madvise syscall to allow for parts of it to be reused by a prctl syscall that affects vmas.

Move the code that walks vmas in a virtual address range into a function that takes a function pointer as a parameter. The only caller for now is sys_madvise, which uses it to call madvise_vma_behavior on each vma, but the next patch will add an additional caller.

Move handling all vma behaviors inside madvise_behavior, and rename it to madvise_vma_behavior.

Move the code that updates the flags on a vma, including splitting or merging the vma as necessary, into a new function called madvise_update_vma. The next patch will add support for updating a new anon_name field as well.

Signed-off-by Colin Cross <[email protected]>

---
 mm/madvise.c | 272 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 151 insertions(+), 121 deletions(-)

mm/madvise.c +151 ~~-121~~

index 7055883..b8820fd 100644
@@ -39,65 +39,20 @@ static int madvise_need_mmap_write(int behavior)
 }
 
 /*
- * We can potentially split a vm area into separate
- * areas, each area with its own behavior.
+ * Update the vm_flags on regiion of a vma, splitting it or merging it as
+ * necessary.  Must be called with mmap_sem held for writing;
  */
-static long madvise_behavior(struct vm_area_struct * vma,
-		     struct vm_area_struct **prev,
-		     unsigned long start, unsigned long end, int behavior)
+static int madvise_update_vma(struct vm_area_struct *vma,
+		     struct vm_area_struct **prev, unsigned long start,
+		     unsigned long end, unsigned long new_flags)
 {
 	struct mm_struct * mm = vma->vm_mm;
-	int error = 0;
 	pgoff_t pgoff;
-	unsigned long new_flags = vma->vm_flags;
-
-	switch (behavior) {
-	case MADV_NORMAL:
-		new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
-		break;
-	case MADV_SEQUENTIAL:
-		new_flags = (new_flags & ~VM_RAND_READ) | VM_SEQ_READ;
-		break;
-	case MADV_RANDOM:
-		new_flags = (new_flags & ~VM_SEQ_READ) | VM_RAND_READ;
-		break;
-	case MADV_DONTFORK:
-		new_flags |= VM_DONTCOPY;
-		break;
-	case MADV_DOFORK:
-		if (vma->vm_flags & VM_IO) {
-			error = -EINVAL;
-			goto out;
-		}
-		new_flags &= ~VM_DONTCOPY;
-		break;
-	case MADV_DONTDUMP:
-		new_flags |= VM_DONTDUMP;
-		break;
-	case MADV_DODUMP:
-		if (new_flags & VM_SPECIAL) {
-			error = -EINVAL;
-			goto out;
-		}
-		new_flags &= ~VM_DONTDUMP;
-		break;
-	case MADV_MERGEABLE:
-	case MADV_UNMERGEABLE:
-		error = ksm_madvise(vma, start, end, behavior, &new_flags);
-		if (error)
-			goto out;
-		break;
-	case MADV_HUGEPAGE:
-	case MADV_NOHUGEPAGE:
-		error = hugepage_madvise(vma, &new_flags, behavior);
-		if (error)
-			goto out;
-		break;
-	}
+	int error;
 
 	if (new_flags == vma->vm_flags) {
 		*prev = vma;
-		goto out;
+		return 0;
 	}
 
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);

@@ -113,13 +68,13 @@ static long madvise_behavior(struct vm_area_struct * vma,
 	if (start != vma->vm_start) {
 		error = split_vma(mm, vma, start, 1);
 		if (error)
-			goto out;
+			return error;
 	}
 
 	if (end != vma->vm_end) {
 		error = split_vma(mm, vma, end, 0);
 		if (error)
-			goto out;
+			return error;
 	}
 
 success:

@@ -128,10 +83,7 @@ success:
 	 */
 	vma->vm_flags = new_flags;
 
-out:
-	if (error == -ENOMEM)
-		error = -EAGAIN;
-	return error;
+	return 0;
 }
 
 #ifdef CONFIG_SWAP

@@ -337,6 +289,77 @@ static long madvise_remove(struct vm_area_struct *vma,
 	return error;
 }
 
+/*
+ * Apply an madvise behavior to a region of a vma.  madvise_update_vma
+ * will handle splitting a vm area into separate areas, each area with its own
+ * behavior.
+ */
+static int madvise_vma_behavior(struct vm_area_struct *vma,
+		     struct vm_area_struct **prev,
+		     unsigned long start, unsigned long end,
+		     unsigned long behavior)
+{
+	int error = 0;
+	unsigned long new_flags = vma->vm_flags;
+
+	switch (behavior) {
+	case MADV_REMOVE:
+		return madvise_remove(vma, prev, start, end);
+	case MADV_WILLNEED:
+		return madvise_willneed(vma, prev, start, end);
+	case MADV_DONTNEED:
+		return madvise_dontneed(vma, prev, start, end);
+	case MADV_NORMAL:
+		new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
+		break;
+	case MADV_SEQUENTIAL:
+		new_flags = (new_flags & ~VM_RAND_READ) | VM_SEQ_READ;
+		break;
+	case MADV_RANDOM:
+		new_flags = (new_flags & ~VM_SEQ_READ) | VM_RAND_READ;
+		break;
+	case MADV_DONTFORK:
+		new_flags |= VM_DONTCOPY;
+		break;
+	case MADV_DOFORK:
+		if (vma->vm_flags & VM_IO) {
+			error = -EINVAL;
+			goto out;
+		}
+		new_flags &= ~VM_DONTCOPY;
+		break;
+	case MADV_DONTDUMP:
+		new_flags |= VM_DONTDUMP;
+		break;
+	case MADV_DODUMP:
+		if (new_flags & VM_SPECIAL) {
+			error = -EINVAL;
+			goto out;
+		}
+		new_flags &= ~VM_DONTDUMP;
+		break;
+	case MADV_MERGEABLE:
+	case MADV_UNMERGEABLE:
+		error = ksm_madvise(vma, start, end, behavior, &new_flags);
+		if (error)
+			goto out;
+		break;
+	case MADV_HUGEPAGE:
+	case MADV_NOHUGEPAGE:
+		error = hugepage_madvise(vma, &new_flags, behavior);
+		if (error)
+			goto out;
+		break;
+	}
+
+	error = madvise_update_vma(vma, prev, start, end, new_flags);
+
+out:
+	if (error == -ENOMEM)
+		error = -EAGAIN;
+	return error;
+}
+
 #ifdef CONFIG_MEMORY_FAILURE
 /*
  * Error injection support for memory error handling.

@@ -369,22 +392,6 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
 }
 #endif
 
-static long
-madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
-		unsigned long start, unsigned long end, int behavior)
-{
-	switch (behavior) {
-	case MADV_REMOVE:
-		return madvise_remove(vma, prev, start, end);
-	case MADV_WILLNEED:
-		return madvise_willneed(vma, prev, start, end);
-	case MADV_DONTNEED:
-		return madvise_dontneed(vma, prev, start, end);
-	default:
-		return madvise_behavior(vma, prev, start, end, behavior);
-	}
-}
-
 static int
 madvise_behavior_valid(int behavior)
 {

@@ -415,6 +422,73 @@ madvise_behavior_valid(int behavior)
 }
 
 /*
+ * Walk the vmas in range [start,end), and call the visit function on each one.
+ * The visit function will get start and end parameters that cover the overlap
+ * between the current vma and the original range.  Any unmapped regions in the
+ * original range will result in this function returning -ENOMEM while still
+ * calling the visit function on all of the existing vmas in the range.
+ * Must be called with the mmap_sem held for reading or writing.
+ */
+static
+int madvise_walk_vmas(unsigned long start, unsigned long end,
+		unsigned long arg,
+		int (*visit)(struct vm_area_struct *vma,
+			struct vm_area_struct **prev, unsigned long start,
+			unsigned long end, unsigned long arg))
+{
+	struct vm_area_struct *vma;
+	struct vm_area_struct *prev;
+	unsigned long tmp;
+	int unmapped_error = 0;
+
+	/*
+	 * If the interval [start,end) covers some unmapped address
+	 * ranges, just ignore them, but return -ENOMEM at the end.
+	 * - different from the way of handling in mlock etc.
+	 */
+	vma = find_vma_prev(current->mm, start, &prev);
+	if (vma && start > vma->vm_start)
+		prev = vma;
+
+	for (;;) {
+		int error;
+
+		/* Still start < end. */
+		if (!vma)
+			return -ENOMEM;
+
+		/* Here start < (end|vma->vm_end). */
+		if (start < vma->vm_start) {
+			unmapped_error = -ENOMEM;
+			start = vma->vm_start;
+			if (start >= end)
+				break;
+		}
+
+		/* Here vma->vm_start <= start < (end|vma->vm_end) */
+		tmp = vma->vm_end;
+		if (end < tmp)
+			tmp = end;
+
+		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
+		error = visit(vma, &prev, start, tmp, arg);
+		if (error)
+			return error;
+		start = tmp;
+		if (prev && start < prev->vm_end)
+			start = prev->vm_end;
+		if (start >= end)
+			break;
+		if (prev)
+			vma = prev->vm_next;
+		else	/* madvise_remove dropped mmap_sem */
+			vma = find_vma(current->mm, start);
+	}
+
+	return unmapped_error;
+}
+
+/*
  * The madvise(2) system call.
  *
  * Applications can use madvise() to advise the kernel how it should

@@ -458,9 +532,7 @@ madvise_behavior_valid(int behavior)
  */
 SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 {
-	unsigned long end, tmp;
-	struct vm_area_struct * vma, *prev;
-	int unmapped_error = 0;
+	unsigned long end;
 	int error = -EINVAL;
 	int write;
 	size_t len;

@@ -495,52 +567,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	else
 		down_read(&current->mm->mmap_sem);
 
-	/*
-	 * If the interval [start,end) covers some unmapped address
-	 * ranges, just ignore them, but return -ENOMEM at the end.
-	 * - different from the way of handling in mlock etc.
-	 */
-	vma = find_vma_prev(current->mm, start, &prev);
-	if (vma && start > vma->vm_start)
-		prev = vma;
-
 	blk_start_plug(&plug);
-	for (;;) {
-		/* Still start < end. */
-		error = -ENOMEM;
-		if (!vma)
-			goto out;
-
-		/* Here start < (end|vma->vm_end). */
-		if (start < vma->vm_start) {
-			unmapped_error = -ENOMEM;
-			start = vma->vm_start;
-			if (start >= end)
-				goto out;
-		}
-
-		/* Here vma->vm_start <= start < (end|vma->vm_end) */
-		tmp = vma->vm_end;
-		if (end < tmp)
-			tmp = end;
-
-		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
-		error = madvise_vma(vma, &prev, start, tmp, behavior);
-		if (error)
-			goto out;
-		start = tmp;
-		if (prev && start < prev->vm_end)
-			start = prev->vm_end;
-		error = unmapped_error;
-		if (start >= end)
-			goto out;
-		if (prev)
-			vma = prev->vm_next;
-		else	/* madvise_remove dropped mmap_sem */
-			vma = find_vma(current->mm, start);
-	}
-out:
+	error = madvise_walk_vmas(start, end, behavior, madvise_vma_behavior);
 	blk_finish_plug(&plug);
+
 	if (write)
 		up_write(&current->mm->mmap_sem);
 	else

Signature · 7 lines

1.8.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Colin Cross

author

2013-10-01 20:22 UTC #1 lore raw

PATCH v2 2/2 mm: add a field to store names for private anonymous memory

In many userspace applications, and especially in VM based applications like Android uses heavily, there are multiple different allocators in use. At a minimum there is libc malloc and the stack, and in many cases there are libc malloc, the stack, direct syscalls to mmap anonymous memory, and multiple VM heaps (one for small objects, one for big objects, etc.). Each of these layers usually has its own tools to inspect its usage; malloc by compiling a debug version, the VM through heap inspection tools, and for direct syscalls there is usually no way to track them.

On Android we heavily use a set of tools that use an extended version of the logic covered in Documentation/vm/pagemap.txt to walk all pages mapped in userspace and slice their usage by process, shared (COW) vs. unique mappings, backing, etc. This can account for real physical memory usage even in cases like fork without exec (which Android uses heavily to share as many private COW pages as possible between processes), Kernel SamePage Merging, and clean zero pages. It produces a measurement of the pages that only exist in that process (USS, for unique), and a measurement of the physical memory usage of that process with the cost of shared pages being evenly split between processes that share them (PSS).

If all anonymous memory is indistinguishable then figuring out the real physical memory usage (PSS) of each heap requires either a pagemap walking tool that can understand the heap debugging of every layer, or for every layer's heap debugging tools to implement the pagemap walking logic, in which case it is hard to get a consistent view of memory across the whole system.

This patch adds a field to /proc/pid/maps and /proc/pid/smaps to show a userspace-provided name for anonymous vmas. The names of named anonymous vmas are shown in /proc/pid/maps as [anon:<name>] and in /proc/pid/smaps in a new "Name" field that is only present for named anonymous vmas. If the userspace pointer is no longer valid all or part of the name will be replaced with "<fault>".

Userspace can set the name for a region of memory by calling prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, start, len, (unsigned long)name); Setting the name to NULL clears it.

The name is stored in a user pointer in the shared union in vm_area_struct that points to a null terminated string inside the user process. vmas that point to the same address and are otherwise mergeable will be merged, but vmas that point to equivalent strings at different addresses will not be merged.

The idea to store a userspace pointer to reduce the complexity within mm (at the expense of the complexity of reading /proc/pid/mem) came from Dave Hansen. This results in no runtime overhead in the mm subsystem other than comparing the anon_name pointers when considering vma merging. The pointer is stored in a union with fields that are only used on file-backed mappings, so it does not increase memory usage.

Signed-off-by Colin Cross <[email protected]>

---

v2: updates the commit message to explain in more detail why the
patch is useful.

 Documentation/filesystems/proc.txt |  6 ++++
 fs/proc/task_mmu.c                 | 62 ++++++++++++++++++++++++++++++++++++++
 include/linux/mm.h                 |  5 ++-
 include/linux/mm_types.h           | 15 +++++++++
 include/uapi/linux/prctl.h         |  3 ++
 kernel/sys.c                       | 24 +++++++++++++++
 mm/madvise.c                       | 56 +++++++++++++++++++++++++++++++---
 mm/mempolicy.c                     |  2 +-
 mm/mlock.c                         |  3 +-
 mm/mmap.c                          | 44 ++++++++++++++++-----------
 mm/mprotect.c                      |  3 +-
 11 files changed, 197 insertions(+), 26 deletions(-)

Documentation/filesystems/proc.txt +6 -0

index fcc22c9..77269c6 100644
@@ -369,6 +369,8 @@ is not associated with a file:
  [stack:1001]             = the stack of the thread with tid 1001
  [vdso]                   = the "virtual dynamic shared object",
                             the kernel system call handler
+ [anon:<name>]            = an anonymous mapping that has been
+                            named by userspace
 
  or if empty, the mapping is anonymous.

@@ -419,6 +421,7 @@ KernelPageSize:        4 kB
 MMUPageSize:           4 kB
 Locked:              374 kB
 VmFlags: rd ex mr mw me de
+Name:           name from userspace
 
 the first of these lines shows the same information as is displayed for the
 mapping in /proc/PID/maps.  The remaining lines show the size of the mapping

@@ -469,6 +472,9 @@ Note that there is no guarantee that every flag and associated mnemonic will
 be present in all further kernel releases. Things get changed, the flags may
 be vanished or the reverse -- new added.
 
+The "Name" field will only be present on a mapping that has been named by
+userspace, and will show the name passed in by userspace.
+
 This file is only present if the CONFIG_MMU kernel configuration option is
 enabled.

fs/proc/task_mmu.c +62 -0

80 lines not on this page · raw message

include/linux/mm.h +4 -1

index f022460..4ccb9e4 100644
@@ -1499,7 +1499,7 @@ extern int vma_adjust(struct vm_area_struct *vma, unsigned long start,
 extern struct vm_area_struct *vma_merge(struct mm_struct *,
 	struct vm_area_struct *prev, unsigned long addr, unsigned long end,
 	unsigned long vm_flags, struct anon_vma *, struct file *, pgoff_t,
-	struct mempolicy *);
+	struct mempolicy *, const char __user *);
 extern struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *);
 extern int split_vma(struct mm_struct *,
 	struct vm_area_struct *, unsigned long addr, int new_below);

@@ -1842,5 +1842,8 @@ void __init setup_nr_node_ids(void);
 static inline void setup_nr_node_ids(void) {}
 #endif
 
+int madvise_set_anon_name(unsigned long start, unsigned long len_in,
+				unsigned long name_addr);
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */

include/linux/mm_types.h +15 -0

index faf4b7c..16de160 100644
@@ -255,6 +255,10 @@ struct vm_area_struct {
 	 * For areas with an address space and backing store,
 	 * linkage into the address_space->i_mmap interval tree, or
 	 * linkage of vma in the address_space->i_mmap_nonlinear list.
+	 *
+	 * For private anonymous mappings, a pointer to a null terminated string
+	 * in the user process containing the name given to the vma, or NULL
+	 * if unnamed.
 	 */
 	union {
 		struct {

@@ -262,6 +266,7 @@ struct vm_area_struct {
 			unsigned long rb_subtree_last;
 		} linear;
 		struct list_head nonlinear;
+		const char __user *anon_name;
 	} shared;
 
 	/*

@@ -454,4 +459,14 @@ static inline cpumask_t *mm_cpumask(struct mm_struct *mm)
 	return mm->cpu_vm_mask_var;
 }
 
+
+/* Return the name for an anonymous mapping or NULL for a file-backed mapping */
+static inline const char __user *vma_get_anon_name(struct vm_area_struct *vma)
+{
+	if (vma->vm_file)
+		return NULL;
+
+	return vma->shared.anon_name;
+}
+
 #endif /* _LINUX_MM_TYPES_H */

include/uapi/linux/prctl.h +3 -0

index 289760f..063bf75 100644
@@ -149,4 +149,7 @@
 
 #define PR_GET_TID_ADDRESS	40
 
+#define PR_SET_VMA		41
+# define PR_SET_VMA_ANON_NAME		0
+
 #endif /* _LINUX_PRCTL_H */

kernel/sys.c +24 -0

index 771129b..5e0948c 100644
@@ -1836,6 +1836,27 @@ static int prctl_get_tid_address(struct task_struct *me, int __user **tid_addr)
 }
 #endif
 
+static int prctl_set_vma(unsigned long opt, unsigned long addr,
+		unsigned long len, unsigned long arg)
+{
+	struct mm_struct *mm = current->mm;
+	int error;
+
+	down_write(&mm->mmap_sem);
+
+	switch (opt) {
+	case PR_SET_VMA_ANON_NAME:
+		error = madvise_set_anon_name(addr, len, arg);
+		break;
+	default:
+		error = -EINVAL;
+	}
+
+	up_write(&mm->mmap_sem);
+
+	return error;
+}
+
 SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		unsigned long, arg4, unsigned long, arg5)
 {

@@ -1999,6 +2020,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		if (arg2 || arg3 || arg4 || arg5)
 			return -EINVAL;
 		return current->no_new_privs ? 1 : 0;
+	case PR_SET_VMA:
+		error = prctl_set_vma(arg2, arg3, arg4, arg5);
+		break;
 	default:
 		error = -EINVAL;
 		break;

mm/madvise.c +52 -4

95 lines not on this page · raw message

mm/mempolicy.c +1 -1

index 4baf12e..550ea39 100644
@@ -728,7 +728,7 @@ static int mbind_range(struct mm_struct *mm, unsigned long start,
 			((vmstart - vma->vm_start) >> PAGE_SHIFT);
 		prev = vma_merge(mm, prev, vmstart, vmend, vma->vm_flags,
 				  vma->anon_vma, vma->vm_file, pgoff,
-				  new_pol);
+				  new_pol, vma->vm_name);
 		if (prev) {
 			vma = prev;
 			next = vma->vm_next;

mm/mlock.c +2 -1

index 79b7cf7..33861c7 100644
@@ -287,7 +287,8 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev,
 
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
 	*prev = vma_merge(mm, *prev, start, end, newflags, vma->anon_vma,
-			  vma->vm_file, pgoff, vma_policy(vma));
+			  vma->vm_file, pgoff, vma_policy(vma),
+			  vma_get_anon_name(vma));
 	if (*prev) {
 		vma = *prev;
 		goto success;

mm/mmap.c +26 ~~-18~~

117 lines not on this page · raw message

mm/mprotect.c +2 -1

index 94722a4..94d50b7 100644
@@ -271,7 +271,8 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
 	 */
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
 	*pprev = vma_merge(mm, *pprev, start, end, newflags,
-			vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma));
+			vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
+			vma_get_anon_name(vma));
 	if (*pprev) {
 		vma = *pprev;
 		goto success;

Signature · 7 lines

1.8.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dave Hansen

2013-10-01 21:13 UTC #2 lore raw

Re: [PATCHv2 2/2] mm: add a field to store names for private anonymous memory

Quoted 39 lines · Colin Cross

fs/proc/task_mmu.c +39 -0

+static void seq_print_vma_name(struct seq_file *m, struct vm_area_struct *vma)
+{
+	const char __user *name = vma_get_anon_name(vma);
+	struct mm_struct *mm = vma->vm_mm;
+
+	unsigned long page_start_vaddr;
+	unsigned long page_offset;
+	unsigned long num_pages;
+	unsigned long max_len = NAME_MAX;
+	int i;
+
+	page_start_vaddr = (unsigned long)name & PAGE_MASK;
+	page_offset = (unsigned long)name - page_start_vaddr;
+	num_pages = DIV_ROUND_UP(page_offset + max_len, PAGE_SIZE);
+
+	seq_puts(m, "[anon:");
+
+	for (i = 0; i < num_pages; i++) {
+		int len;
+		int write_len;
+		const char *kaddr;
+		long pages_pinned;
+		struct page *page;
+
+		pages_pinned = get_user_pages(current, mm, page_start_vaddr,
+				1, 0, 0, &page, NULL);
+		if (pages_pinned < 1) {
+			seq_puts(m, "<fault>]");
+			return;
+		}
+
+		kaddr = (const char *)kmap(page);
+		len = min(max_len, PAGE_SIZE - page_offset);
+		write_len = strnlen(kaddr + page_offset, len);
+		seq_write(m, kaddr + page_offset, write_len);
+		kunmap(page);
+		put_page(page);
+
+		/* if strnlen hit a null terminator then we're done */

fs/proc/task_mmu.c static void pad_len_spaces(struct seq_file *m, int len) · line 142

Quoted 10 lines

fs/proc/task_mmu.c +10 -0

+		if (write_len != len)
+			break;
+
+		max_len -= len;
+		page_offset = 0;
+		page_start_vaddr += PAGE_SIZE;
+	}
+
+	seq_putc(m, ']');
+}

Is there a reason you can't use access_process_vm(), or share some code with proc_pid_cmdline()? It seems to be doing a bunch of the same stuff that you are. Also, considering that this roll-your-own code, and it's digging around in user-supplied addresses, it seems like the kind of thing that's prone to introducing security problems. Could you share some of your logic around how misuse of this mechanism is prevented?

If the range this is going after spans two pages, and the second is bogus, you'll end up with :

	[anon: foo<fault>]

I guess that's OK, but I find it a wee bit funky.

Quoted 17 lines

#ifdef CONFIG_NUMA
/*
* These functions are for numa_maps but called in generic **maps seq_file

fs/proc/task_mmu.c +6 -0

@@ -336,6 +386,12 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 				pad_len_spaces(m, len);
 				seq_printf(m, "[stack:%d]", tid);
 			}
+			goto done;
+		}
+
+		if (vma_get_anon_name(vma)) {
+			pad_len_spaces(m, len);
+			seq_print_vma_name(m, vma);
 		}
 	}

fs/proc/task_mmu.c static int show_smap(struct seq_file *m, void *v, int is_pid) · line 698

Quoted 8 lines

fs/proc/task_mmu.c +5 -0

@@ -635,6 +691,12 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
 	show_smap_vma_flags(m, vma);
 
+	if (vma_get_anon_name(vma)) {
+		seq_puts(m, "Name:           ");
+		seq_print_vma_name(m, vma);
+		seq_putc(m, '\n');
+	}

FWIW, I'm not a fan of using "get" in function names unless it's taking some kind of reference. I'd probably call it "vma_user_anon_ptr()" or something.

I dug through the implementation a bit, and don't see any showstoppers, but it does churn around the VMA merging code enough to make me a bit nervous. I hope you tested it well. :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Colin Cross

author

2013-10-01 21:59 UTC #3 lore raw

Re: [PATCHv2 2/2] mm: add a field to store names for private anonymous memory

Quoted 49 lines · Dave Hansen

Quoted 48 lines · Colin Cross

fs/proc/task_mmu.c +48 -0

+static void seq_print_vma_name(struct seq_file *m, struct vm_area_struct *vma)
+{
+     const char __user *name = vma_get_anon_name(vma);
+     struct mm_struct *mm = vma->vm_mm;
+
+     unsigned long page_start_vaddr;
+     unsigned long page_offset;
+     unsigned long num_pages;
+     unsigned long max_len = NAME_MAX;
+     int i;
+
+     page_start_vaddr = (unsigned long)name & PAGE_MASK;
+     page_offset = (unsigned long)name - page_start_vaddr;
+     num_pages = DIV_ROUND_UP(page_offset + max_len, PAGE_SIZE);
+
+     seq_puts(m, "[anon:");
+
+     for (i = 0; i < num_pages; i++) {
+             int len;
+             int write_len;
+             const char *kaddr;
+             long pages_pinned;
+             struct page *page;
+
+             pages_pinned = get_user_pages(current, mm, page_start_vaddr,
+                             1, 0, 0, &page, NULL);
+             if (pages_pinned < 1) {
+                     seq_puts(m, "<fault>]");
+                     return;
+             }
+
+             kaddr = (const char *)kmap(page);
+             len = min(max_len, PAGE_SIZE - page_offset);
+             write_len = strnlen(kaddr + page_offset, len);
+             seq_write(m, kaddr + page_offset, write_len);
+             kunmap(page);
+             put_page(page);
+
+             /* if strnlen hit a null terminator then we're done */
+             if (write_len != len)
+                     break;
+
+             max_len -= len;
+             page_offset = 0;
+             page_start_vaddr += PAGE_SIZE;
+     }
+
+     seq_putc(m, ']');

Quoted 8 lines

Quoted 1 line
+}
Is there a reason you can't use access_process_vm(), or share some code with proc_pid_cmdline()? It seems to be doing a bunch of the same stuff that you are. Also, considering that this roll-your-own code, and it's digging around in user-supplied addresses, it seems like the kind of thing that's prone to introducing security problems. Could you share some of your logic around how misuse of this mechanism is prevented?

The key difference between access_process_vm/proc_pid_cmdline and this is that I don't have a length here. It reads up to NAME_MAX bytes until it finds a null terminator. There is also a secondary optimization that avoids memcpy, it maps the page into the kernel and then calls seq_write on it directly, but that is minor.

I could change this to access_process_vm, which would result in copying 1024 bytes for every named mapping in /proc/pid/maps.

By passing a pointer to the kernel a process is allowing any other process that can read /proc/pid/maps to see memory from the pointer to either the first following null terminator, or 1024 bytes. If the memory were to get freed and reused without a null terminator the process could leak up to 1024 bytes of its memory to other processes that can read /proc/pid/maps. I don't see this as a security issue because /proc/pid/maps is protected by the same permissions as /proc/pid/mem, so anything that could read the leaked data could already read it directly.

Quoted 6 lines

If the range this is going after spans two pages, and the second is bogus, you'll end up with :
        [anon: foo<fault>]
I guess that's OK, but I find it a wee bit funky.

Yeah, it is funky, but I don't expect it to happen in practice since the string will likely be in the read-only .text section and won't get unmapped. It's a side-effect of avoiding the memcpy above. If I switched to access_process_vm it would go away.

Quoted 20 lines

#ifdef CONFIG_NUMA
/*
* These functions are for numa_maps but called in generic **maps seq_file

fs/proc/task_mmu.c +6 -0

@@ -336,6 +386,12 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
                              pad_len_spaces(m, len);
                              seq_printf(m, "[stack:%d]", tid);
                      }
+                     goto done;
+             }
+
+             if (vma_get_anon_name(vma)) {
+                     pad_len_spaces(m, len);
+                     seq_print_vma_name(m, vma);
              }
      }

@@ -635,6 +691,12 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
      show_smap_vma_flags(m, vma);

Quoted 9 lines

fs/proc/task_mmu.c static int show_smap(struct seq_file *m, void *v, int is_pid) · line 698
Quoted 5 lines
fs/proc/task_mmu.c +5 -0
+     if (vma_get_anon_name(vma)) {
+             seq_puts(m, "Name:           ");
+             seq_print_vma_name(m, vma);
+             seq_putc(m, '\n');
+     }
FWIW, I'm not a fan of using "get" in function names unless it's taking some kind of reference. I'd probably call it "vma_user_anon_ptr()" or something.

Sure

Quoted 3 lines

I dug through the implementation a bit, and don't see any showstoppers, but it does churn around the VMA merging code enough to make me a bit nervous. I hope you tested it well. :)

Most of the churn is plumbing through the name of an existing vma from all the callers to vma_merge. I considered refactoring vma_merge to take a struct describing the new vma, instead of a list of parameters that describe the new vma, with a helper function to create the struct from an existing vma. So a vma_merge caller would have:

Colin Cross

author

2013-10-01 22:05 UTC #4 lore raw

Re: [PATCHv2 2/2] mm: add a field to store names for private anonymous memory

Quoted 126 lines · Colin Cross

Quoted 49 lines · Dave Hansen
Quoted 48 lines · Colin Cross
fs/proc/task_mmu.c +48 -0
+static void seq_print_vma_name(struct seq_file *m, struct vm_area_struct *vma)
+{
+     const char __user *name = vma_get_anon_name(vma);
+     struct mm_struct *mm = vma->vm_mm;
+
+     unsigned long page_start_vaddr;
+     unsigned long page_offset;
+     unsigned long num_pages;
+     unsigned long max_len = NAME_MAX;
+     int i;
+
+     page_start_vaddr = (unsigned long)name & PAGE_MASK;
+     page_offset = (unsigned long)name - page_start_vaddr;
+     num_pages = DIV_ROUND_UP(page_offset + max_len, PAGE_SIZE);
+
+     seq_puts(m, "[anon:");
+
+     for (i = 0; i < num_pages; i++) {
+             int len;
+             int write_len;
+             const char *kaddr;
+             long pages_pinned;
+             struct page *page;
+
+             pages_pinned = get_user_pages(current, mm, page_start_vaddr,
+                             1, 0, 0, &page, NULL);
+             if (pages_pinned < 1) {
+                     seq_puts(m, "<fault>]");
+                     return;
+             }
+
+             kaddr = (const char *)kmap(page);
+             len = min(max_len, PAGE_SIZE - page_offset);
+             write_len = strnlen(kaddr + page_offset, len);
+             seq_write(m, kaddr + page_offset, write_len);
+             kunmap(page);
+             put_page(page);
+
+             /* if strnlen hit a null terminator then we're done */
+             if (write_len != len)
+                     break;
+
+             max_len -= len;
+             page_offset = 0;
+             page_start_vaddr += PAGE_SIZE;
+     }
+
+     seq_putc(m, ']');
Quoted 8 lines
Quoted 1 line
+}
Is there a reason you can't use access_process_vm(), or share some code with proc_pid_cmdline()? It seems to be doing a bunch of the same stuff that you are. Also, considering that this roll-your-own code, and it's digging around in user-supplied addresses, it seems like the kind of thing that's prone to introducing security problems. Could you share some of your logic around how misuse of this mechanism is prevented?
The key difference between access_process_vm/proc_pid_cmdline and this is that I don't have a length here. It reads up to NAME_MAX bytes until it finds a null terminator. There is also a secondary optimization that avoids memcpy, it maps the page into the kernel and then calls seq_write on it directly, but that is minor.
I could change this to access_process_vm, which would result in copying 1024 bytes for every named mapping in /proc/pid/maps.
By passing a pointer to the kernel a process is allowing any other process that can read /proc/pid/maps to see memory from the pointer to either the first following null terminator, or 1024 bytes. If the memory were to get freed and reused without a null terminator the process could leak up to 1024 bytes of its memory to other processes that can read /proc/pid/maps. I don't see this as a security issue because /proc/pid/maps is protected by the same permissions as /proc/pid/mem, so anything that could read the leaked data could already read it directly.
Quoted 6 lines
If the range this is going after spans two pages, and the second is bogus, you'll end up with :
        [anon: foo<fault>]
I guess that's OK, but I find it a wee bit funky.
Yeah, it is funky, but I don't expect it to happen in practice since the string will likely be in the read-only .text section and won't get unmapped. It's a side-effect of avoiding the memcpy above. If I switched to access_process_vm it would go away.
Quoted 20 lines
Quoted 20 lines
#ifdef CONFIG_NUMA
/*
* These functions are for numa_maps but called in generic **maps seq_file
fs/proc/task_mmu.c +6 -0
@@ -336,6 +386,12 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
                              pad_len_spaces(m, len);
                              seq_printf(m, "[stack:%d]", tid);
                      }
+                     goto done;
+             }
+
+             if (vma_get_anon_name(vma)) {
+                     pad_len_spaces(m, len);
+                     seq_print_vma_name(m, vma);
              }
      }
 
@@ -635,6 +691,12 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
      show_smap_vma_flags(m, vma);
 
Quoted 9 lines
fs/proc/task_mmu.c static int show_smap(struct seq_file *m, void *v, int is_pid) · line 698
Quoted 5 lines
fs/proc/task_mmu.c +5 -0
+     if (vma_get_anon_name(vma)) {
+             seq_puts(m, "Name:           ");
+             seq_print_vma_name(m, vma);
+             seq_putc(m, '\n');
+     }
FWIW, I'm not a fan of using "get" in function names unless it's taking some kind of reference. I'd probably call it "vma_user_anon_ptr()" or something.
Sure
Quoted 3 lines
I dug through the implementation a bit, and don't see any showstoppers, but it does churn around the VMA merging code enough to make me a bit nervous. I hope you tested it well. :)

Quoted 5 lines

Most of the churn is plumbing through the name of an existing vma from all the callers to vma_merge. I considered refactoring vma_merge to take a struct describing the new vma, instead of a list of parameters that describe the new vma, with a helper function to create the struct from an existing vma. So a vma_merge caller would have:

Premature send.

A vma_merge caller like mempolicy.c mbind_range would have:
struct vma_desc;
vma_desc_copy_from_vma(&vma_desc, vma); vma_desc.addr = vmstart;
vma_desc.end = vmend;
vma_desc.pgoff = pgoff;
vma_desc.mempolicy = new_pol;
prev = vma_merge(mm, prev, &vma_desc);

This seemed like too invasive a change for adding a single new comparison point to vma_merge, but might make sense if there are likely to be more fields to compare in vma_merge in the future.

KOSAKI Motohiro

2013-10-02 00:37 UTC #5 lore raw

Re: [PATCHv2 2/2] mm: add a field to store names for private anonymous memory

In reply to Colin Cross

Quoted 23 lines

fs/proc/task_mmu.c +23 -0

+static void seq_print_vma_name(struct seq_file *m, struct vm_area_struct *vma)
+{
+	const char __user *name = vma_get_anon_name(vma);
+	struct mm_struct *mm = vma->vm_mm;
+
+	unsigned long page_start_vaddr;
+	unsigned long page_offset;
+	unsigned long num_pages;
+	unsigned long max_len = NAME_MAX;
+	int i;
+
+	page_start_vaddr = (unsigned long)name & PAGE_MASK;
+	page_offset = (unsigned long)name - page_start_vaddr;
+	num_pages = DIV_ROUND_UP(page_offset + max_len, PAGE_SIZE);
+
+	seq_puts(m, "[anon:");
+
+	for (i = 0; i < num_pages; i++) {
+		int len;
+		int write_len;
+		const char *kaddr;
+		long pages_pinned;
+		struct page *page;

fs/proc/task_mmu.c static void pad_len_spaces(struct seq_file *m, int len) · line 126

Quoted 10 lines

fs/proc/task_mmu.c +10 -0

+
+		pages_pinned = get_user_pages(current, mm, page_start_vaddr,
+				1, 0, 0, &page, NULL);
+		if (pages_pinned < 1) {
+			seq_puts(m, "<fault>]");
+			return;
+		}
+
+		kaddr = (const char *)kmap(page);
+		len = min(max_len, PAGE_SIZE - page_offset);

You can't show the name if the name is placed in end of page.