[PATCH] Mark thread stack correctly in proc/<pid>/maps

[PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

Memory mmaped by glibc for a thread stack currently shows up as a simple
anonymous map, which makes it difficult to differentiate between memory
usage of the thread on stack and other dynamic allocation. Since glibc
already uses MAP_STACK to request this mapping, the attached patch
uses this flag to add additional VM_STACK_FLAGS to the resulting vma
so that the mapping is treated as a stack and not any regular
anonymous mapping. Also, one may use vm_flags to decide if a vma is a
stack.

There is an additional complication with posix threads where the stack
guard for a thread stack may be larger than a page, unlike the case
for process stack where the stack guard is a page long. glibc
implements these guards by calling mprotect on the beginning page(s)
to remove all permissions. I have used this to remove vmas that have
the thread stack guard, from the /proc/maps output.

If accepted, this should also reflect in the man page for mmap since
MAP_STACK will no longer be a noop.

Signed-off-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
---
 fs/proc/task_mmu.c |    8 +++++---
 include/linux/mm.h |   17 +++++++++++++++++
 mm/mmap.c          |    3 +++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e418c5a..98b5275 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -227,7 +227,10 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 		pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
 	}
 
-	/* We don't show the stack guard page in /proc/maps */
+	/* We don't show the stack guard pages in /proc/maps */
+	if (thread_stack_guard(vma))
+		return;
+
 	start = vma->vm_start;
 	if (stack_guard_page_start(vma, start))
 		start += PAGE_SIZE;
@@ -259,8 +262,7 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 				if (vma->vm_start <= mm->brk &&
 						vma->vm_end >= mm->start_brk) {
 					name = "[heap]";
-				} else if (vma->vm_start <= mm->start_stack &&
-					   vma->vm_end >= mm->start_stack) {
+				} else if (vma_is_stack(vma)) {
 					name = "[stack]";
 				}
 			} else {
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 17b27cd..9871e10 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1018,6 +1018,23 @@ static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long addr)
 	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
 }
 
+static inline int vma_is_stack(struct vm_area_struct *vma)
+{
+	return vma && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN));
+}
+
+/*
+ * POSIX thread stack guards may be more than a page long and access to it
+ * should return an error (possibly a SIGSEGV). The glibc implementation does
+ * an mprotect(..., ..., PROT_NONE), so our guard vma has no permissions.
+ */
+static inline int thread_stack_guard(struct vm_area_struct *vma)
+{
+	return vma_is_stack(vma) &&
+		((vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_MAYSHARE)) == 0) &&
+		vma_is_stack((vma->vm_flags & VM_GROWSDOWN)?vma->vm_next:vma->vm_prev);
+}
+
 static inline int stack_guard_page_start(struct vm_area_struct *vma,
 					     unsigned long addr)
 {
diff --git a/mm/mmap.c b/mm/mmap.c
index 3f758c7..2f9f540 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -992,6 +992,9 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
 			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
+	if (flags & MAP_STACK)
+		vm_flags |= VM_STACK_FLAGS;
+
 	if (flags & MAP_LOCKED)
 		if (!can_do_mlock())
 			return -EPERM;
-- 
1.7.7.4

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Mark thread stack correctly in proc/<pid>/maps

[Jamie Lokier]

Siddhesh Poyarekar wrote:
> Memory mmaped by glibc for a thread stack currently shows up as a simple
> anonymous map, which makes it difficult to differentiate between memory
> usage of the thread on stack and other dynamic allocation. Since glibc
> already uses MAP_STACK to request this mapping, the attached patch
> uses this flag to add additional VM_STACK_FLAGS to the resulting vma
> so that the mapping is treated as a stack and not any regular
> anonymous mapping. Also, one may use vm_flags to decide if a vma is a
> stack.

I think this is fine.

> There is an additional complication with posix threads where the stack
> guard for a thread stack may be larger than a page, unlike the case
> for process stack where the stack guard is a page long. glibc
> implements these guards by calling mprotect on the beginning page(s)
> to remove all permissions. I have used this to remove vmas that have
> the thread stack guard, from the /proc/maps output.

> -	/* We don't show the stack guard page in /proc/maps */
> +	/* We don't show the stack guard pages in /proc/maps */
> +	if (thread_stack_guard(vma))
> +		return;
> +
>  	start = vma->vm_start;
>  	if (stack_guard_page_start(vma, start))
>  		start += PAGE_SIZE;

Hmm, I see why you did this.  The current code already hides one guard
page, which is already dubious for programs that do things like read
/proc/pid/maps to decide if MAP_FIXED would be not clobber an existing
mapping.  At least those programs _could_ know about the stack guard
page address

I wonder if it's a potential security hole: You've just allowed
programs to use two MAP_GROWSUP/DOWN|PROT_NONE to hide vmas from the
user.  Sure, the memory isn't accessible, but it can still store data
and be ephemerally made visible using mprotect() then hidden again.

I would prefer a label like "[stack guard]" or just "[guard]",
both for the thread stacks and the process stack.

With a label like "[guard]" it needn't be limited to stacks; heap
guard pages used by some programs would also be labelled.

> +static inline int vma_is_stack(struct vm_area_struct *vma)
> +{
> +	return vma && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN));
> +}
> +
> +/*
> + * POSIX thread stack guards may be more than a page long and access to it
> + * should return an error (possibly a SIGSEGV). The glibc implementation does
> + * an mprotect(..., ..., PROT_NONE), so our guard vma has no permissions.
> + */
> +static inline int thread_stack_guard(struct vm_area_struct *vma)

Is there a reason the names aren't consistent - i.e. not vma_is_stack_guard()?

> +{
> +	return vma_is_stack(vma) &&
> +		((vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_MAYSHARE)) == 0) &&
> +		vma_is_stack((vma->vm_flags & VM_GROWSDOWN)?vma->vm_next:vma->vm_prev);
> +}
> +

That doesn't check if ->vm_next/prev is adjacent in address space.

You can't assume the program is using Glibc, or that MAP_STACK
mappings are all from Glibc, or that they are in the pattern you expect.

How about simply calling it vma_is_guard(), return 1 if it's PROT_NONE
without checking vma_is_stack() or ->vm_next/prev, and annotate the
maps output like this:

   is_stack              => "[stack]"
   is_guard & is_stack   => "[stack guard]"
   is_guard & !is_stack  => "[guard]"

What do you think?

-- Jamie

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Mon, Jan 16, 2012 at 4:58 PM, Jamie Lokier <jamie@shareable.org> wrote:
> Is there a reason the names aren't consistent - i.e. not vma_is_stack_guard()?

Ah, that was an error on my part; I did not notice the naming convention.

> How about simply calling it vma_is_guard(), return 1 if it's PROT_NONE
> without checking vma_is_stack() or ->vm_next/prev, and annotate the
> maps output like this:
>
>   is_stack              => "[stack]"
>   is_guard & is_stack   => "[stack guard]"
>   is_guard & !is_stack  => "[guard]"
>
> What do you think?

Thanks for the review. We're already marking permissions in the maps
output to convey protection, so isn't marking those vmas as [guard]
redundant?

Following that, we could just mark the thread stack guard as [stack]
without any permissions. The process stack guard page probably
deserves the [stack guard] label since it is marked differently from
the thread stack guard and will otherwise have the permissions that
the process stack has. Will that be good?

-- 
Siddhesh Poyarekar

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Mark thread stack correctly in proc/<pid>/maps

[Jamie Lokier]

Siddhesh Poyarekar wrote:
> On Mon, Jan 16, 2012 at 4:58 PM, Jamie Lokier <jamie@shareable.org> wrote:
> > Is there a reason the names aren't consistent - i.e. not vma_is_stack_guard()?
> 
> Ah, that was an error on my part; I did not notice the naming convention.
> 
> > How about simply calling it vma_is_guard(), return 1 if it's PROT_NONE
> > without checking vma_is_stack() or ->vm_next/prev, and annotate the
> > maps output like this:
> >
> >   is_stack              => "[stack]"
> >   is_guard & is_stack   => "[stack guard]"
> >   is_guard & !is_stack  => "[guard]"
> >
> > What do you think?
> 
> Thanks for the review. We're already marking permissions in the maps
> output to convey protection, so isn't marking those vmas as [guard]
> redundant?

Yes it's redundant, I just think it's a bit clearer at showing the
intent.  After all that's also the reason for "[stack]", "[heap]" etc.
It's not important though.

> Following that, we could just mark the thread stack guard as [stack]
> without any permissions. The process stack guard page probably
> deserves the [stack guard] label since it is marked differently from
> the thread stack guard and will otherwise have the permissions that
> the process stack has. Will that be good?

I don't have any strong opinions about what the test looks like; mainly I
was pointing out the ->vm_next/prev check seemed dubious, that Glibc's
layout shouldn't be assumed, and that hiding ranges from /maps may
mislead some programs.

Aesthetically I think if the main process stack has "[stack guard]",
it makes sense for the thread stack guards to be labelled the same.

One more technical thing: Now that you're using VM_STACK to change the
text, why not set that flag for the process stack vma as well, when
the stack is set up by exec, and get rid of the special case for
process stack in printing?

All the best,
-- Jamie

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Mon, Jan 16, 2012 at 10:01 PM, Jamie Lokier <jamie@shareable.org> wrote:
> Aesthetically I think if the main process stack has "[stack guard]",
> it makes sense for the thread stack guards to be labelled the same.

Right, I'll mark both stack guards alike.

> One more technical thing: Now that you're using VM_STACK to change the
> text, why not set that flag for the process stack vma as well, when
> the stack is set up by exec, and get rid of the special case for
> process stack in printing?

I think the flag is already set:

static int __bprm_mm_init(struct linux_binprm *bprm)
{
...
        /*
         * Place the stack at the largest stack address the architecture
         * supports. Later, we'll move this to an appropriate place. We don't
         * use STACK_TOP because that can depend on attributes which aren't
         * configured yet.
         */
        BUILD_BUG_ON(VM_STACK_FLAGS & VM_STACK_INCOMPLETE_SETUP);
        vma->vm_end = STACK_TOP_MAX;
        vma->vm_start = vma->vm_end - PAGE_SIZE;
        vma->vm_flags = VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
        vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
        INIT_LIST_HEAD(&vma->anon_vma_chain);
...
}

The only special case in the printing code for the process stack is
the skipping of the guard page. I'll modify that to mark and display
the stack guard instead.

I'll post an updated patch with these changes.

Thanks!

-- 
Siddhesh Poyarekar

[PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

[Take 2]

Memory mmaped by glibc for a thread stack currently shows up as a simple
anonymous map, which makes it difficult to differentiate between memory
usage of the thread on stack and other dynamic allocation. Since glibc
already uses MAP_STACK to request this mapping, the attached patch
uses this flag to add additional VM_STACK_FLAGS to the resulting vma
so that the mapping is treated as a stack and not any regular
anonymous mapping. Also, one may use vm_flags to decide if a vma is a
stack.

This patch also changes the maps output to annotate stack guards for
both the process stack as well as the thread stacks. Thus is born the
[stack guard] annotation, which should be exactly a page long for the
process stack and can be longer than a page (configurable in
userspace) for POSIX compliant thread stacks. A thread stack guard is
simply page(s) with PROT_NONE.

If accepted, this should also reflect in the man page for mmap since
MAP_STACK will no longer be a noop.

Signed-off-by: Siddhesh Poyarekar <siddhesh.poyarekar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/proc/task_mmu.c |   41 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mm.h |   19 +++++++++++++++++--
 mm/mmap.c          |    3 +++
 3 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e418c5a..650330c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -227,13 +227,42 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 		pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
 	}
 
-	/* We don't show the stack guard page in /proc/maps */
+	/*
+	 * Mark the process stack guard, which is just one page at the
+	 * beginning of the stack within the stack vma.
+	 */
 	start = vma->vm_start;
-	if (stack_guard_page_start(vma, start))
+	if (stack_guard_page_start(vma, start)) {
+		seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+				start,
+				start + PAGE_SIZE,
+				flags & VM_READ ? 'r' : '-',
+				flags & VM_WRITE ? 'w' : '-',
+				flags & VM_EXEC ? 'x' : '-',
+				flags & VM_MAYSHARE ? 's' : 'p',
+				pgoff,
+				MAJOR(dev), MINOR(dev), ino, &len);
+
+		pad_len_spaces(m, len);
+		seq_puts(m, "[stack guard]\n");
 		start += PAGE_SIZE;
+	}
 	end = vma->vm_end;
-	if (stack_guard_page_end(vma, end))
+	if (stack_guard_page_end(vma, end)) {
+		seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+				end - PAGE_SIZE,
+				end,
+				flags & VM_READ ? 'r' : '-',
+				flags & VM_WRITE ? 'w' : '-',
+				flags & VM_EXEC ? 'x' : '-',
+				flags & VM_MAYSHARE ? 's' : 'p',
+				pgoff,
+				MAJOR(dev), MINOR(dev), ino, &len);
+
+		pad_len_spaces(m, len);
+		seq_puts(m, "[stack guard]\n");
 		end -= PAGE_SIZE;
+	}
 
 	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
 			start,
@@ -259,8 +288,10 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 				if (vma->vm_start <= mm->brk &&
 						vma->vm_end >= mm->start_brk) {
 					name = "[heap]";
-				} else if (vma->vm_start <= mm->start_stack &&
-					   vma->vm_end >= mm->start_stack) {
+				} else if (vma_is_stack(vma) &&
+					   vma_is_guard(vma)) {
+					name = "[stack guard]";
+				} else if (vma_is_stack(vma)) {
 					name = "[stack]";
 				}
 			} else {
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 17b27cd..4e57753 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1018,12 +1018,26 @@ static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long addr)
 	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
 }
 
+static inline int vma_is_stack(struct vm_area_struct *vma)
+{
+	return vma && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN));
+}
+
+/*
+ * Check guard set by userspace (PROT_NONE)
+ */
+static inline int vma_is_guard(struct vm_area_struct *vma)
+{
+	return (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) == 0;
+}
+
 static inline int stack_guard_page_start(struct vm_area_struct *vma,
 					     unsigned long addr)
 {
 	return (vma->vm_flags & VM_GROWSDOWN) &&
 		(vma->vm_start == addr) &&
-		!vma_growsdown(vma->vm_prev, addr);
+		!vma_growsdown(vma->vm_prev, addr) &&
+		!vma_is_guard(vma);
 }
 
 /* Is the vma a continuation of the stack vma below it? */
@@ -1037,7 +1051,8 @@ static inline int stack_guard_page_end(struct vm_area_struct *vma,
 {
 	return (vma->vm_flags & VM_GROWSUP) &&
 		(vma->vm_end == addr) &&
-		!vma_growsup(vma->vm_next, addr);
+		!vma_growsup(vma->vm_next, addr) &&
+		!vma_is_guard(vma);
 }
 
 extern unsigned long move_page_tables(struct vm_area_struct *vma,
diff --git a/mm/mmap.c b/mm/mmap.c
index 3f758c7..2f9f540 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -992,6 +992,9 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
 			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
+	if (flags & MAP_STACK)
+		vm_flags |= VM_STACK_FLAGS;
+
 	if (flags & MAP_LOCKED)
 		if (!can_do_mlock())
 			return -EPERM;
-- 
1.7.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

Hi,

Resending since I did not get any feedback on the second take of the patch.

Thanks,
Siddhesh


---------- Forwarded message ----------
From: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
Date: Tue, Jan 17, 2012 at 10:24 AM
Subject: [PATCH] Mark thread stack correctly in proc/<pid>/maps
To: Jamie Lokier <jamie@shareable.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Alexander Viro
<viro@zeniv.linux.org.uk>, linux-fsdevel@vger.kernel.org, Michael
Kerrisk <mtk.manpages@gmail.com>, linux-man@vger.kernel.org, Siddhesh
Poyarekar <siddhesh.poyarekar@gmail.com>


[Take 2]

Memory mmaped by glibc for a thread stack currently shows up as a simple
anonymous map, which makes it difficult to differentiate between memory
usage of the thread on stack and other dynamic allocation. Since glibc
already uses MAP_STACK to request this mapping, the attached patch
uses this flag to add additional VM_STACK_FLAGS to the resulting vma
so that the mapping is treated as a stack and not any regular
anonymous mapping. Also, one may use vm_flags to decide if a vma is a
stack.

This patch also changes the maps output to annotate stack guards for
both the process stack as well as the thread stacks. Thus is born the
[stack guard] annotation, which should be exactly a page long for the
process stack and can be longer than a page (configurable in
userspace) for POSIX compliant thread stacks. A thread stack guard is
simply page(s) with PROT_NONE.

If accepted, this should also reflect in the man page for mmap since
MAP_STACK will no longer be a noop.

Signed-off-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
---
 fs/proc/task_mmu.c |   41 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mm.h |   19 +++++++++++++++++--
 mm/mmap.c          |    3 +++
 3 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e418c5a..650330c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -227,13 +227,42 @@ static void show_map_vma(struct seq_file *m,
struct vm_area_struct *vma)
               pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
       }

-       /* We don't show the stack guard page in /proc/maps */
+       /*
+        * Mark the process stack guard, which is just one page at the
+        * beginning of the stack within the stack vma.
+        */
       start = vma->vm_start;
-       if (stack_guard_page_start(vma, start))
+       if (stack_guard_page_start(vma, start)) {
+               seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+                               start,
+                               start + PAGE_SIZE,
+                               flags & VM_READ ? 'r' : '-',
+                               flags & VM_WRITE ? 'w' : '-',
+                               flags & VM_EXEC ? 'x' : '-',
+                               flags & VM_MAYSHARE ? 's' : 'p',
+                               pgoff,
+                               MAJOR(dev), MINOR(dev), ino, &len);
+
+               pad_len_spaces(m, len);
+               seq_puts(m, "[stack guard]\n");
               start += PAGE_SIZE;
+       }
       end = vma->vm_end;
-       if (stack_guard_page_end(vma, end))
+       if (stack_guard_page_end(vma, end)) {
+               seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+                               end - PAGE_SIZE,
+                               end,
+                               flags & VM_READ ? 'r' : '-',
+                               flags & VM_WRITE ? 'w' : '-',
+                               flags & VM_EXEC ? 'x' : '-',
+                               flags & VM_MAYSHARE ? 's' : 'p',
+                               pgoff,
+                               MAJOR(dev), MINOR(dev), ino, &len);
+
+               pad_len_spaces(m, len);
+               seq_puts(m, "[stack guard]\n");
               end -= PAGE_SIZE;
+       }

       seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
                       start,
@@ -259,8 +288,10 @@ static void show_map_vma(struct seq_file *m,
struct vm_area_struct *vma)
                               if (vma->vm_start <= mm->brk &&
                                               vma->vm_end >= mm->start_brk) {
                                       name = "[heap]";
-                               } else if (vma->vm_start <= mm->start_stack &&
-                                          vma->vm_end >= mm->start_stack) {
+                               } else if (vma_is_stack(vma) &&
+                                          vma_is_guard(vma)) {
+                                       name = "[stack guard]";
+                               } else if (vma_is_stack(vma)) {
                                       name = "[stack]";
                               }
                       } else {
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 17b27cd..4e57753 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1018,12 +1018,26 @@ static inline int vma_growsdown(struct
vm_area_struct *vma, unsigned long addr)
       return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
 }

+static inline int vma_is_stack(struct vm_area_struct *vma)
+{
+       return vma && (vma->vm_flags & (VM_GROWSUP | VM_GROWSDOWN));
+}
+
+/*
+ * Check guard set by userspace (PROT_NONE)
+ */
+static inline int vma_is_guard(struct vm_area_struct *vma)
+{
+       return (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC |
VM_SHARED)) == 0;
+}
+
 static inline int stack_guard_page_start(struct vm_area_struct *vma,
                                            unsigned long addr)
 {
       return (vma->vm_flags & VM_GROWSDOWN) &&
               (vma->vm_start == addr) &&
-               !vma_growsdown(vma->vm_prev, addr);
+               !vma_growsdown(vma->vm_prev, addr) &&
+               !vma_is_guard(vma);
 }

 /* Is the vma a continuation of the stack vma below it? */
@@ -1037,7 +1051,8 @@ static inline int stack_guard_page_end(struct
vm_area_struct *vma,
 {
       return (vma->vm_flags & VM_GROWSUP) &&
               (vma->vm_end == addr) &&
-               !vma_growsup(vma->vm_next, addr);
+               !vma_growsup(vma->vm_next, addr) &&
+               !vma_is_guard(vma);
 }

 extern unsigned long move_page_tables(struct vm_area_struct *vma,
diff --git a/mm/mmap.c b/mm/mmap.c
index 3f758c7..2f9f540 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -992,6 +992,9 @@ unsigned long do_mmap_pgoff(struct file *file,
unsigned long addr,
       vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
                       mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;

+       if (flags & MAP_STACK)
+               vm_flags |= VM_STACK_FLAGS;
+
       if (flags & MAP_LOCKED)
               if (!can_do_mlock())
                       return -EPERM;
--
1.7.7.4



-- 
Siddhesh Poyarekar
http://siddhesh.in

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[KOSAKI Motohiro]

>   extern unsigned long move_page_tables(struct vm_area_struct *vma,
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 3f758c7..2f9f540 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -992,6 +992,9 @@ unsigned long do_mmap_pgoff(struct file *file,
> unsigned long addr,
>         vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
>                         mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
>
> +       if (flags&  MAP_STACK)
> +               vm_flags |= VM_STACK_FLAGS;

??
MAP_STACK doesn't mean auto stack expansion. Why do you turn on VM_GROWSDOWN?
Seems incorrect.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Fri, Feb 3, 2012 at 3:10 AM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
>>  extern unsigned long move_page_tables(struct vm_area_struct *vma,
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 3f758c7..2f9f540 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -992,6 +992,9 @@ unsigned long do_mmap_pgoff(struct file *file,
>> unsigned long addr,
>>        vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
>>                        mm->def_flags | VM_MAYREAD | VM_MAYWRITE |
>> VM_MAYEXEC;
>>
>> +       if (flags&  MAP_STACK)
>> +               vm_flags |= VM_STACK_FLAGS;
>
>
> ??
> MAP_STACK doesn't mean auto stack expansion. Why do you turn on
> VM_GROWSDOWN?
> Seems incorrect.
>

Right now MAP_STACK does not mean anything since it is ignored. The
intention of this behaviour change is to make MAP_STACK mean that the
map is going to be used as a stack and hence, set it up like a stack
ought to be. I could not really think of a valid case for fixed size
stacks; it looks like a limitation in the pthread implementation in
glibc rather than a feature. So this patch will actually result in
uniform behaviour across threads when it comes to stacks.

This does change vm accounting since thread stacks were earlier
accounted as anon memory.

-- 
Siddhesh Poyarekar
http://siddhesh.in

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[KOSAKI Motohiro]

> Right now MAP_STACK does not mean anything since it is ignored. The
> intention of this behaviour change is to make MAP_STACK mean that the
> map is going to be used as a stack and hence, set it up like a stack
> ought to be. I could not really think of a valid case for fixed size
> stacks; it looks like a limitation in the pthread implementation in
> glibc rather than a feature. So this patch will actually result in
> uniform behaviour across threads when it comes to stacks.
>
> This does change vm accounting since thread stacks were earlier
> accounted as anon memory.

The fact is, now process stack and pthread stack clearly behave
different dance. libc don't expect pthread stack grow automatically.
So, your patch will break userland. Just only change display thing.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Fri, Feb 3, 2012 at 1:31 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> The fact is, now process stack and pthread stack clearly behave
> different dance. libc don't expect pthread stack grow automatically.
> So, your patch will break userland. Just only change display thing.

Thanks for your feedback. This attempt was to unify this behaviours,
but I guess you're right; I need to check if glibc really has a
problem with this than assuming that it should not. I will check with
glibc maintainers on this and update here. Since this flag is
specifically for glibc, it should not affect other applications or
libraries.

The proc changes won't make sense without the change to mark thread
stacks unless we create yet another vm flag to reflect MAP_STACK in
the vma and then use that for both process and its threads. I'll
submit a patch with this (if acceptable of course) if glibc strictly
requires fixed sized stacks.

-- 
Siddhesh Poyarekar
http://siddhesh.in

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1247 bytes --]

On Friday 03 February 2012 03:01:35 KOSAKI Motohiro wrote:
> > Right now MAP_STACK does not mean anything since it is ignored. The
> > intention of this behaviour change is to make MAP_STACK mean that the
> > map is going to be used as a stack and hence, set it up like a stack
> > ought to be. I could not really think of a valid case for fixed size
> > stacks; it looks like a limitation in the pthread implementation in
> > glibc rather than a feature. So this patch will actually result in
> > uniform behaviour across threads when it comes to stacks.
> > 
> > This does change vm accounting since thread stacks were earlier
> > accounted as anon memory.
> 
> The fact is, now process stack and pthread stack clearly behave
> different dance. libc don't expect pthread stack grow automatically.
> So, your patch will break userland. Just only change display thing.

does it though ?  glibc doesn't keep track of the unused address space ... 
that's what the kernel is for.  pthread_attr_setstacksize explicitly operates 
on the *minimum* stack size, not the *exact* size.

where exactly do you think userland would break ?

http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_setstacksize.html
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Fri, Feb 3, 2012 at 1:31 PM, KOSAKI Motohiro
<kosaki.motohiro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> The fact is, now process stack and pthread stack clearly behave
> different dance. libc don't expect pthread stack grow automatically.
> So, your patch will break userland. Just only change display thing.

The change should not make thread stacks (as implemented by glibc)
grow automatically in the general case since it has to implement guard
page(s) at the beginning of the mapped memory for stack using
mprotect(top, size, PROT_NONE). Due to this, the program will crash
before it ever has a chance to cause a page fault to make the stack
grow. This is of course assuming that the program doesn't purposely
jump over the guard page(s) by setting %sp beyond them and then
causing a page fault. So the only case in which a thread stack will
grow automatically is if the stackguard is set to 0:

http://pubs.opengroup.org/onlinepubs/007904975/functions/pthread_attr_setguardsize.html

I have also dropped an email on the libc-alpha list here to solicit
comments from libc maintainers on this:

http://sourceware.org/ml/libc-alpha/2012-02/msg00036.html


-- 
Siddhesh Poyarekar
http://siddhesh.in
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Sat, Feb 4, 2012 at 12:04 AM, Siddhesh Poyarekar
<siddhesh.poyarekar@gmail.com> wrote:
> On Fri, Feb 3, 2012 at 1:31 PM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com> wrote:
>> The fact is, now process stack and pthread stack clearly behave
>> different dance. libc don't expect pthread stack grow automatically.
>> So, your patch will break userland. Just only change display thing.
<snip>
> I have also dropped an email on the libc-alpha list here to solicit
> comments from libc maintainers on this:
>
> http://sourceware.org/ml/libc-alpha/2012-02/msg00036.html
>

Kosaki-san, your suggestion of adding an extra flag seems like the
right way to go about this based on the discussion on libc-alpha,
specifically, your point about pthread_getattr_np() -- it may not be a
standard, but it's a breakage anyway. However, looking at the vm_flags
options in mm.h, it looks like the entire 32-bit space has been
exhausted for the flag value. The vm_flags is an unsigned long, so it
ought to take 8 bytes on a 64-bit system, but 32-bit systems will be
left behind.

So there are two options for this:

1) make vm_flags 64-bit for all arches. This will cause ABI breakage
on 32-bit systems, so any external drivers will have to be rebuilt
2) Implement this patch for 64-bit only by defining the new flag only
for 64-bit. 32-bit systems behave as is

Which of these would be better? I prefer the latter because it looks
like the path of least breakage.

-- 
Siddhesh Poyarekar
http://siddhesh.in

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[KOSAKI Motohiro]

(2/7/12 11:00 PM), Siddhesh Poyarekar wrote:
> On Sat, Feb 4, 2012 at 12:04 AM, Siddhesh Poyarekar
> <siddhesh.poyarekar@gmail.com>  wrote:
>> On Fri, Feb 3, 2012 at 1:31 PM, KOSAKI Motohiro
>> <kosaki.motohiro@gmail.com>  wrote:
>>> The fact is, now process stack and pthread stack clearly behave
>>> different dance. libc don't expect pthread stack grow automatically.
>>> So, your patch will break userland. Just only change display thing.
> <snip>
>> I have also dropped an email on the libc-alpha list here to solicit
>> comments from libc maintainers on this:
>>
>> http://sourceware.org/ml/libc-alpha/2012-02/msg00036.html
>>
>
> Kosaki-san, your suggestion of adding an extra flag seems like the
> right way to go about this based on the discussion on libc-alpha,
> specifically, your point about pthread_getattr_np() -- it may not be a
> standard, but it's a breakage anyway. However, looking at the vm_flags
> options in mm.h, it looks like the entire 32-bit space has been
> exhausted for the flag value. The vm_flags is an unsigned long, so it
> ought to take 8 bytes on a 64-bit system, but 32-bit systems will be
> left behind.
>
> So there are two options for this:
>
> 1) make vm_flags 64-bit for all arches. This will cause ABI breakage
> on 32-bit systems, so any external drivers will have to be rebuilt

Several month ago, Linus NAKed this way.


> 2) Implement this patch for 64-bit only by defining the new flag only
> for 64-bit. 32-bit systems behave as is

No. That's bad than status quo. Enduser may get inconsistent and bad user
experience.

Now, we are using some bit saving hack. example,

1) use ifdef

#ifndef CONFIG_TRANSPARENT_HUGEPAGE
#define VM_MAPPED_COPY 0x01000000      /* T if mapped copy of data (nommu mmap) */
#else
#define VM_HUGEPAGE    0x01000000      /* MADV_HUGEPAGE marked this vma */
#endif

2) use bit combination

#define VM_STACK_INCOMPLETE_SETUP      (VM_RAND_READ | VM_SEQ_READ)


Maybe you can take a similar way. And of course, you can ban some useless flag
bits.

thanks.

> Which of these would be better? I prefer the latter because it looks
> like the path of least breakage.
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH] Mark thread stack correctly in proc/<pid>/maps

[Siddhesh Poyarekar]

On Wed, Feb 8, 2012 at 11:27 PM, KOSAKI Motohiro
<kosaki.motohiro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Now, we are using some bit saving hack. example,
>
> 1) use ifdef
>
> #ifndef CONFIG_TRANSPARENT_HUGEPAGE
> #define VM_MAPPED_COPY 0x01000000      /* T if mapped copy of data (nommu
> mmap) */
> #else
> #define VM_HUGEPAGE    0x01000000      /* MADV_HUGEPAGE marked this vma */
> #endif
>
> 2) use bit combination
>
> #define VM_STACK_INCOMPLETE_SETUP      (VM_RAND_READ | VM_SEQ_READ)
>
>
> Maybe you can take a similar way. And of course, you can ban some useless
> flag
> bits.

I found the thread in which Linus rejected the idea of expanding vm_flags:

https://lkml.org/lkml/2011/11/10/522

and based on that, I don't think I can justify the need for a new flag
for this patch since it is purely for display purposes and has nothing
to do with the actual treatment of the vma. So I figured out another
way to identify a thread stack without changing the way the vma
properties (I should have done this in the first place I think) which
is by checking if the vma contains the stack pointer of the task.

With this change:

/proc/pid/task/tid/maps: will only mark the stack that the task uses

/proc/pid/maps: will mark all stacks. This path will be slower since
it will iterate through the entire thread group for each vma.

I'll test this and attach a new version of the patch.

Regards,
Siddhesh

-- 
Siddhesh Poyarekar
http://siddhesh.in
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html