diff mbox

[Lucid,Maverick] SRU: Fix (hopefully) the last stack guard page effect

Message ID 1284480244-5648-1-git-send-email-stefan.bader@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Stefan Bader Sept. 14, 2010, 4:04 p.m. UTC 
SRU Justification:

Impact: When introducing the stack guard page, a follow up patch tried to
minimize the effects user-space sees from this change. Unfortunately a
lot of the checks did not take into account that user-space may mlock a
portion of the stack, but not all of it. This causes the stack vma to
be split and a lot of assumptions to go down the drain. Most places have
been fixed by now but looking at /proc/<pid>/maps when a stack vma has
been split still hides a page on every area and not only on head of it
as it was intended.

Fix: The following upstream change makes one inline available to other
code and uses that when checking for the guard page in the proc output.

Testcase: A program that mlocks an area of the stack and then dumps its
maps will see holes before the patch and none after.

Note: When providing incremental patches to Linus, the cc for stable got
lost, though I pinged Greg, but have not received an answer yet. In the
end it hopefully comes from stable. I have not yet created a bug report
for it as I am not really sure how urgent this is to fix.

-Stefan

---

From 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Tue, 31 Aug 2010 15:52:27 +0200
Subject: [PATCH] mm: (pre-stable) Move vma_stack_continue into mm.h

So it can be used by all that need to check for that.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry-picked from commit 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 upstream)
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 fs/proc/task_mmu.c |    3 ++-
 include/linux/mm.h |    6 ++++++
 mm/mlock.c         |    6 ------
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

Leann Ogasawara Sept. 14, 2010, 10:33 p.m. UTC | #1 
Applied to Maverick linux master.  Feel free to add my Ack for the Lucid
SRU.

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

Thanks,
Leann

On Tue, 2010-09-14 at 18:04 +0200, Stefan Bader wrote:
> SRU Justification:
> 
> Impact: When introducing the stack guard page, a follow up patch tried to
> minimize the effects user-space sees from this change. Unfortunately a
> lot of the checks did not take into account that user-space may mlock a
> portion of the stack, but not all of it. This causes the stack vma to
> be split and a lot of assumptions to go down the drain. Most places have
> been fixed by now but looking at /proc/<pid>/maps when a stack vma has
> been split still hides a page on every area and not only on head of it
> as it was intended.
> 
> Fix: The following upstream change makes one inline available to other
> code and uses that when checking for the guard page in the proc output.
> 
> Testcase: A program that mlocks an area of the stack and then dumps its
> maps will see holes before the patch and none after.
> 
> Note: When providing incremental patches to Linus, the cc for stable got
> lost, though I pinged Greg, but have not received an answer yet. In the
> end it hopefully comes from stable. I have not yet created a bug report
> for it as I am not really sure how urgent this is to fix.
> 
> -Stefan
> 
> ---
> 
> From 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Tue, 31 Aug 2010 15:52:27 +0200
> Subject: [PATCH] mm: (pre-stable) Move vma_stack_continue into mm.h
> 
> So it can be used by all that need to check for that.
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry-picked from commit 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 upstream)
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  fs/proc/task_mmu.c |    3 ++-
>  include/linux/mm.h |    6 ++++++
>  mm/mlock.c         |    6 ------
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 439fc1f..271afc4 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -224,7 +224,8 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>  	/* We don't show the stack guard page in /proc/maps */
>  	start = vma->vm_start;
>  	if (vma->vm_flags & VM_GROWSDOWN)
> -		start += PAGE_SIZE;
> +		if (!vma_stack_continue(vma->vm_prev, vma->vm_start))
> +			start += PAGE_SIZE;
>  
>  	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
>  			start,
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e6b1210..74949fb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -864,6 +864,12 @@ int set_page_dirty(struct page *page);
>  int set_page_dirty_lock(struct page *page);
>  int clear_page_dirty_for_io(struct page *page);
>  
> +/* Is the vma a continuation of the stack vma above it? */
> +static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> +{
> +	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
> +}
> +
>  extern unsigned long move_page_tables(struct vm_area_struct *vma,
>  		unsigned long old_addr, struct vm_area_struct *new_vma,
>  		unsigned long new_addr, unsigned long len);
> diff --git a/mm/mlock.c b/mm/mlock.c
> index cbae7c5..b70919c 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -135,12 +135,6 @@ void munlock_vma_page(struct page *page)
>  	}
>  }
>  
> -/* Is the vma a continuation of the stack vma above it? */
> -static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> -{
> -	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
> -}
> -
>  static inline int stack_guard_page(struct vm_area_struct *vma, unsigned long addr)
>  {
>  	return (vma->vm_flags & VM_GROWSDOWN) &&
> -- 
> 1.7.0.4
> 
>
Brad Figg Sept. 16, 2010, 1:14 p.m. UTC | #2 
On 09/14/2010 09:04 AM, Stefan Bader wrote:
> SRU Justification:
>
> Impact: When introducing the stack guard page, a follow up patch tried to
> minimize the effects user-space sees from this change. Unfortunately a
> lot of the checks did not take into account that user-space may mlock a
> portion of the stack, but not all of it. This causes the stack vma to
> be split and a lot of assumptions to go down the drain. Most places have
> been fixed by now but looking at /proc/<pid>/maps when a stack vma has
> been split still hides a page on every area and not only on head of it
> as it was intended.
>
> Fix: The following upstream change makes one inline available to other
> code and uses that when checking for the guard page in the proc output.
>
> Testcase: A program that mlocks an area of the stack and then dumps its
> maps will see holes before the patch and none after.
>
> Note: When providing incremental patches to Linus, the cc for stable got
> lost, though I pinged Greg, but have not received an answer yet. In the
> end it hopefully comes from stable. I have not yet created a bug report
> for it as I am not really sure how urgent this is to fix.
>
> -Stefan
>
> ---
>
>  From 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 Mon Sep 17 00:00:00 2001
> From: Stefan Bader<stefan.bader@canonical.com>
> Date: Tue, 31 Aug 2010 15:52:27 +0200
> Subject: [PATCH] mm: (pre-stable) Move vma_stack_continue into mm.h
>
> So it can be used by all that need to check for that.
>
> Signed-off-by: Stefan Bader<stefan.bader@canonical.com>
> Signed-off-by: Linus Torvalds<torvalds@linux-foundation.org>
> (cherry-picked from commit 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 upstream)
> Signed-off-by: Stefan Bader<stefan.bader@canonical.com>
> ---
>   fs/proc/task_mmu.c |    3 ++-
>   include/linux/mm.h |    6 ++++++
>   mm/mlock.c         |    6 ------
>   3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 439fc1f..271afc4 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -224,7 +224,8 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
>   	/* We don't show the stack guard page in /proc/maps */
>   	start = vma->vm_start;
>   	if (vma->vm_flags&  VM_GROWSDOWN)
> -		start += PAGE_SIZE;
> +		if (!vma_stack_continue(vma->vm_prev, vma->vm_start))
> +			start += PAGE_SIZE;
>
>   	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
>   			start,
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e6b1210..74949fb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -864,6 +864,12 @@ int set_page_dirty(struct page *page);
>   int set_page_dirty_lock(struct page *page);
>   int clear_page_dirty_for_io(struct page *page);
>
> +/* Is the vma a continuation of the stack vma above it? */
> +static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> +{
> +	return vma&&  (vma->vm_end == addr)&&  (vma->vm_flags&  VM_GROWSDOWN);
> +}
> +
>   extern unsigned long move_page_tables(struct vm_area_struct *vma,
>   		unsigned long old_addr, struct vm_area_struct *new_vma,
>   		unsigned long new_addr, unsigned long len);
> diff --git a/mm/mlock.c b/mm/mlock.c
> index cbae7c5..b70919c 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -135,12 +135,6 @@ void munlock_vma_page(struct page *page)
>   	}
>   }
>
> -/* Is the vma a continuation of the stack vma above it? */
> -static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> -{
> -	return vma&&  (vma->vm_end == addr)&&  (vma->vm_flags&  VM_GROWSDOWN);
> -}
> -
>   static inline int stack_guard_page(struct vm_area_struct *vma, unsigned long addr)
>   {
>   	return (vma->vm_flags&  VM_GROWSDOWN)&&


Acked-by: Brad Figg <brad.figg@canonical.com>
Steve Conklin Sept. 17, 2010, 2:51 p.m. UTC | #3 
On Thu, 2010-09-16 at 06:14 -0700, Brad Figg wrote:
> On 09/14/2010 09:04 AM, Stefan Bader wrote:
> > SRU Justification:
> >
> > Impact: When introducing the stack guard page, a follow up patch tried to
> > minimize the effects user-space sees from this change. Unfortunately a
> > lot of the checks did not take into account that user-space may mlock a
> > portion of the stack, but not all of it. This causes the stack vma to
> > be split and a lot of assumptions to go down the drain. Most places have
> > been fixed by now but looking at /proc/<pid>/maps when a stack vma has
> > been split still hides a page on every area and not only on head of it
> > as it was intended.
> >
> > Fix: The following upstream change makes one inline available to other
> > code and uses that when checking for the guard page in the proc output.
> >
> > Testcase: A program that mlocks an area of the stack and then dumps its
> > maps will see holes before the patch and none after.
> >
> > Note: When providing incremental patches to Linus, the cc for stable got
> > lost, though I pinged Greg, but have not received an answer yet. In the
> > end it hopefully comes from stable. I have not yet created a bug report
> > for it as I am not really sure how urgent this is to fix.
> >
> > -Stefan
> >
> > ---
> >
> >  From 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 Mon Sep 17 00:00:00 2001
> > From: Stefan Bader<stefan.bader@canonical.com>
> > Date: Tue, 31 Aug 2010 15:52:27 +0200
> > Subject: [PATCH] mm: (pre-stable) Move vma_stack_continue into mm.h
> >
> > So it can be used by all that need to check for that.
> >
> > Signed-off-by: Stefan Bader<stefan.bader@canonical.com>
> > Signed-off-by: Linus Torvalds<torvalds@linux-foundation.org>
> > (cherry-picked from commit 39aa3cb3e8250db9188a6f1e3fb62ffa1a717678 upstream)
> > Signed-off-by: Stefan Bader<stefan.bader@canonical.com>
> > ---
> >   fs/proc/task_mmu.c |    3 ++-
> >   include/linux/mm.h |    6 ++++++
> >   mm/mlock.c         |    6 ------
> >   3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 439fc1f..271afc4 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -224,7 +224,8 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
> >   	/* We don't show the stack guard page in /proc/maps */
> >   	start = vma->vm_start;
> >   	if (vma->vm_flags&  VM_GROWSDOWN)
> > -		start += PAGE_SIZE;
> > +		if (!vma_stack_continue(vma->vm_prev, vma->vm_start))
> > +			start += PAGE_SIZE;
> >
> >   	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
> >   			start,
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index e6b1210..74949fb 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -864,6 +864,12 @@ int set_page_dirty(struct page *page);
> >   int set_page_dirty_lock(struct page *page);
> >   int clear_page_dirty_for_io(struct page *page);
> >
> > +/* Is the vma a continuation of the stack vma above it? */
> > +static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> > +{
> > +	return vma&&  (vma->vm_end == addr)&&  (vma->vm_flags&  VM_GROWSDOWN);
> > +}
> > +
> >   extern unsigned long move_page_tables(struct vm_area_struct *vma,
> >   		unsigned long old_addr, struct vm_area_struct *new_vma,
> >   		unsigned long new_addr, unsigned long len);
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index cbae7c5..b70919c 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -135,12 +135,6 @@ void munlock_vma_page(struct page *page)
> >   	}
> >   }
> >
> > -/* Is the vma a continuation of the stack vma above it? */
> > -static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
> > -{
> > -	return vma&&  (vma->vm_end == addr)&&  (vma->vm_flags&  VM_GROWSDOWN);
> > -}
> > -
> >   static inline int stack_guard_page(struct vm_area_struct *vma, unsigned long addr)
> >   {
> >   	return (vma->vm_flags&  VM_GROWSDOWN)&&
> 
> 
> Acked-by: Brad Figg <brad.figg@canonical.com>
> 
> -- 
> Brad Figg brad.figg@canonical.com http://www.canonical.com
> 
Acked-by: Steve Conklin <sconklin@canonical.com>
diff mbox

Patch

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 439fc1f..271afc4 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -224,7 +224,8 @@  static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 	/* We don't show the stack guard page in /proc/maps */
 	start = vma->vm_start;
 	if (vma->vm_flags & VM_GROWSDOWN)
-		start += PAGE_SIZE;
+		if (!vma_stack_continue(vma->vm_prev, vma->vm_start))
+			start += PAGE_SIZE;
 
 	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
 			start,
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e6b1210..74949fb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -864,6 +864,12 @@  int set_page_dirty(struct page *page);
 int set_page_dirty_lock(struct page *page);
 int clear_page_dirty_for_io(struct page *page);
 
+/* Is the vma a continuation of the stack vma above it? */
+static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
+{
+	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
+}
+
 extern unsigned long move_page_tables(struct vm_area_struct *vma,
 		unsigned long old_addr, struct vm_area_struct *new_vma,
 		unsigned long new_addr, unsigned long len);
diff --git a/mm/mlock.c b/mm/mlock.c
index cbae7c5..b70919c 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -135,12 +135,6 @@  void munlock_vma_page(struct page *page)
 	}
 }
 
-/* Is the vma a continuation of the stack vma above it? */
-static inline int vma_stack_continue(struct vm_area_struct *vma, unsigned long addr)
-{
-	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
-}
-
 static inline int stack_guard_page(struct vm_area_struct *vma, unsigned long addr)
 {
 	return (vma->vm_flags & VM_GROWSDOWN) &&