[PATCH 0/1] ipc/shm: fix the historical/wrong mm->start_stack check

[PATCH 0/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/23, Oleg Nesterov wrote:
>
> But for what? Ignoring the (I think buggy) check in do_shmat() ->start_stack
> is simply unused, we only report it via /proc/.

Really, I think do_shmat() should not abuse ->start_stack. Not only it is
wrong and unneeded afaics, do_shmat() is the only user of ->start_stack,
this adds more confusion.

Oleg.

[PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

The ->start_stack check in do_shmat() looks ugly and simply wrong.

1. ->start_stack is only valid right after exec(), the application
   can switch to another stack and even unmap this area.

2. The reason for this check is not clear at all. The application
   should know what it does. And why 4 pages? And why in fact it
   requires 5 pages?

3. This wrongly assumes that the stack can only grown down.

Personally I think we should simply kill this check, but I did not
dare to do this. So the patch only fixes the 1st problem (mostly to
avoid the usage of mm->start_stack) and ignores VM_GROWSUP.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 ipc/shm.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 7fc9f9f..5c35fd4 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1166,19 +1166,24 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 
 	down_write(&current->mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
-		err = -EINVAL;
-		if (addr + size < addr)
-			goto invalid;
+		unsigned long end = addr + size;
+		struct vm_area_struct *vma;
 
-		if (find_vma_intersection(current->mm, addr, addr + size))
+		err = -EINVAL;
+		if (end < addr)
 			goto invalid;
 		/*
-		 * If shm segment goes below stack, make sure there is some
-		 * space left for the stack to grow (at least 4 pages).
+		 * Ensure this segment doesn't overlap with the next vma.
+		 * If it is stack, make sure there is some space left for
+		 * the stack to grow, at least 4 pages. (Why?)
 		 */
-		if (addr < current->mm->start_stack &&
-		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
-			goto invalid;
+		vma = find_vma(current->mm, addr);
+		if (vma) {
+			if (vma->vm_flags & VM_GROWSDOWN)
+				end += PAGE_SIZE * 4; /* can't overflow */
+			if (end > vma->vm_start)
+				goto invalid;
+		}
 	}
 
 	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
-- 
1.5.5.1

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Cyrill Gorcunov]

On Sat, Aug 23, 2014 at 04:43:27PM +0200, Oleg Nesterov wrote:
> The ->start_stack check in do_shmat() looks ugly and simply wrong.
> 
> 1. ->start_stack is only valid right after exec(), the application
>    can switch to another stack and even unmap this area.
> 
> 2. The reason for this check is not clear at all. The application
>    should know what it does. And why 4 pages? And why in fact it
>    requires 5 pages?
> 
> 3. This wrongly assumes that the stack can only grown down.
> 
> Personally I think we should simply kill this check, but I did not
> dare to do this. So the patch only fixes the 1st problem (mostly to
> avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

I don't understand this check either, the comment above it says nothing
but only commits what code is doing not explaining why.

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/23, Cyrill Gorcunov wrote:
>
> On Sat, Aug 23, 2014 at 04:43:27PM +0200, Oleg Nesterov wrote:
> > The ->start_stack check in do_shmat() looks ugly and simply wrong.
> >
> > 1. ->start_stack is only valid right after exec(), the application
> >    can switch to another stack and even unmap this area.
> >
> > 2. The reason for this check is not clear at all. The application
> >    should know what it does. And why 4 pages? And why in fact it
> >    requires 5 pages?
> >
> > 3. This wrongly assumes that the stack can only grown down.
> >
> > Personally I think we should simply kill this check, but I did not
> > dare to do this. So the patch only fixes the 1st problem (mostly to
> > avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Thanks!

> I don't understand this check either, the comment above it says nothing
> but only commits what code is doing not explaining why.

Yes, and this check predates the git history. I even looked into
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git but this
change was added by the huge "v2.5.0.7 -> v2.5.0.8" update in 2002,
and obviously without any explanation (apart from "fix up proper shmat
semantics", but this connects SHM_REMAP itself).

Oleg.

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Manfred Spraul]

On 08/23/2014 04:43 PM, Oleg Nesterov wrote:
> The ->start_stack check in do_shmat() looks ugly and simply wrong.
>
> 1. ->start_stack is only valid right after exec(), the application
>     can switch to another stack and even unmap this area.
>
> 2. The reason for this check is not clear at all. The application
>     should know what it does. And why 4 pages? And why in fact it
>     requires 5 pages?
>
> 3. This wrongly assumes that the stack can only grown down.
>
> Personally I think we should simply kill this check, but I did not
> dare to do this. So the patch only fixes the 1st problem (mostly to
> avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Manfred Spraul <manfred@colorfullife.com>
> [snip]

> +		if (vma) {
> +			if (vma->vm_flags & VM_GROWSDOWN)
> +				end += PAGE_SIZE * 4; /* can't overflow */
Why is an overflow impossible?
> +			if (end > vma->vm_start)
> +				goto invalid;
> +		}
>   	}
>   
>   	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
--
     Manfred

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Hugh Dickins]

On Sat, 23 Aug 2014, Oleg Nesterov wrote:
> On 08/23, Cyrill Gorcunov wrote:
> >
> > On Sat, Aug 23, 2014 at 04:43:27PM +0200, Oleg Nesterov wrote:
> > > The ->start_stack check in do_shmat() looks ugly and simply wrong.
> > >
> > > 1. ->start_stack is only valid right after exec(), the application
> > >    can switch to another stack and even unmap this area.
> > >
> > > 2. The reason for this check is not clear at all. The application
> > >    should know what it does. And why 4 pages? And why in fact it
> > >    requires 5 pages?
> > >
> > > 3. This wrongly assumes that the stack can only grown down.
> > >
> > > Personally I think we should simply kill this check, but I did not
> > > dare to do this. So the patch only fixes the 1st problem (mostly to
> > > avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Yes, much better to use find_vma than have this strange stray use
of unreliable start_stack.

Acked-by: Hugh Dickins <hughd@google.com>

though like Manfred I didn't quite see how overflow was impossible
on unfamiliar architectures.

> 
> Thanks!
> 
> > I don't understand this check either, the comment above it says nothing
> > but only commits what code is doing not explaining why.
> 
> Yes, and this check predates the git history. I even looked into
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git but this
> change was added by the huge "v2.5.0.7 -> v2.5.0.8" update in 2002,
> and obviously without any explanation (apart from "fix up proper shmat
> semantics", but this connects SHM_REMAP itself).

I'd say it comes earlier, from Christoph Rohland's 2.4.17-pre7's
"Add missing checks on shmat()", though I didn't find more than that.

We can all understand wanting to leave a gap below the growsdown stack,
but of course could argue about growsup and 1 or 4 or 5 or whatever:
okay that we're all more interested in just removing that start_stack.

Hugh

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/24, Manfred Spraul wrote:
>
> On 08/23/2014 04:43 PM, Oleg Nesterov wrote:
>> The ->start_stack check in do_shmat() looks ugly and simply wrong.
>>
>> 1. ->start_stack is only valid right after exec(), the application
>>     can switch to another stack and even unmap this area.
>>
>> 2. The reason for this check is not clear at all. The application
>>     should know what it does. And why 4 pages? And why in fact it
>>     requires 5 pages?
>>
>> 3. This wrongly assumes that the stack can only grown down.
>>
>> Personally I think we should simply kill this check, but I did not
>> dare to do this. So the patch only fixes the 1st problem (mostly to
>> avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
>>
>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Manfred Spraul <manfred@colorfullife.com>

Thanks!

>> +		if (vma) {
>> +			if (vma->vm_flags & VM_GROWSDOWN)
>> +				end += PAGE_SIZE * 4; /* can't overflow */
> Why is an overflow impossible?

OOPS. I swear it was not possible until I simplified this patch ;)

In fact we do not really care because do_mmap_pgoff() will fail, but
this should be fixed anyway. Either we should not check the overflows
at all, or these checks should be consistent.

I'll send v2, thanks Manfred.

Oleg.

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/24, Hugh Dickins wrote:
>
> On Sat, 23 Aug 2014, Oleg Nesterov wrote:
> > On 08/23, Cyrill Gorcunov wrote:
> > >
> > > On Sat, Aug 23, 2014 at 04:43:27PM +0200, Oleg Nesterov wrote:
> > > > The ->start_stack check in do_shmat() looks ugly and simply wrong.
> > > >
> > > > 1. ->start_stack is only valid right after exec(), the application
> > > >    can switch to another stack and even unmap this area.
> > > >
> > > > 2. The reason for this check is not clear at all. The application
> > > >    should know what it does. And why 4 pages? And why in fact it
> > > >    requires 5 pages?
> > > >
> > > > 3. This wrongly assumes that the stack can only grown down.
> > > >
> > > > Personally I think we should simply kill this check, but I did not
> > > > dare to do this. So the patch only fixes the 1st problem (mostly to
> > > > avoid the usage of mm->start_stack) and ignores VM_GROWSUP.
> > > >
> > > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
>
> Yes, much better to use find_vma than have this strange stray use
> of unreliable start_stack.
>
> Acked-by: Hugh Dickins <hughd@google.com>

Thanks!

> though like Manfred I didn't quite see how overflow was impossible
> on unfamiliar architectures.

And you can't see, because the comment is simply wrong, I'll send v2.

> > > I don't understand this check either, the comment above it says nothing
> > > but only commits what code is doing not explaining why.
> >
> > Yes, and this check predates the git history. I even looked into
> > git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git but this
> > change was added by the huge "v2.5.0.7 -> v2.5.0.8" update in 2002,
> > and obviously without any explanation (apart from "fix up proper shmat
> > semantics", but this connects SHM_REMAP itself).
>
> I'd say it comes earlier, from Christoph Rohland's 2.4.17-pre7's
> "Add missing checks on shmat()", though I didn't find more than that.
>
> We can all understand wanting to leave a gap below the growsdown stack,
> but of course could argue about growsup and 1 or 4 or 5 or whatever:

And it is not clear to me why the kernel should care at all,

> okay that we're all more interested in just removing that start_stack.

so perhaps v2 should simply remove it? Or do you think it would be safer
to not do this?

Oleg.

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Hugh Dickins]

On Mon, 25 Aug 2014, Oleg Nesterov wrote:
> On 08/24, Hugh Dickins wrote:
> >
> > I'd say it comes earlier, from Christoph Rohland's 2.4.17-pre7's
> > "Add missing checks on shmat()", though I didn't find more than that.
> >
> > We can all understand wanting to leave a gap below the growsdown stack,
> > but of course could argue about growsup and 1 or 4 or 5 or whatever:
> 
> And it is not clear to me why the kernel should care at all,

Care about what exactly?  Leaving a gap between shm and stack?

The man page says that (unless SHM_REMAP) shmat() will fail with
EINVAL if a mapping already exists there, and I think it's fair
to regard the vm_start of a VM_GROWSDOWN somewhat elastically.

It may be that Linus's check_stack_guard_page() work in 2.6.36
changed the importance of this shmat() check, but I'd still feel
safer to leave it as is (while turning a blind eye to the
VM_GROWSUP omission).

> 
> > okay that we're all more interested in just removing that start_stack.
> 
> so perhaps v2 should simply remove it? Or do you think it would be safer
> to not do this?

It would be safer to leave it, but replace the start_stack use as you did.
And I think I'll let Linus's guard page justify your 4 (to match comment)
in place of the original's mysterious 5.

Hugh

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/25, Hugh Dickins wrote:
>
> On Mon, 25 Aug 2014, Oleg Nesterov wrote:
> > On 08/24, Hugh Dickins wrote:
> > >
> > > I'd say it comes earlier, from Christoph Rohland's 2.4.17-pre7's
> > > "Add missing checks on shmat()", though I didn't find more than that.
> > >
> > > We can all understand wanting to leave a gap below the growsdown stack,
> > > but of course could argue about growsup and 1 or 4 or 5 or whatever:
> >
> > And it is not clear to me why the kernel should care at all,
>
> Care about what exactly?  Leaving a gap between shm and stack?
>
> The man page says that (unless SHM_REMAP) shmat() will fail with
> EINVAL if a mapping already exists there, and I think it's fair
> to regard the vm_start of a VM_GROWSDOWN somewhat elastically.
>
> It may be that Linus's check_stack_guard_page() work in 2.6.36
> changed the importance of this shmat() check, but I'd still feel
> safer to leave it as is (while turning a blind eye to the
> VM_GROWSUP omission).
>
> >
> > > okay that we're all more interested in just removing that start_stack.
> >
> > so perhaps v2 should simply remove it? Or do you think it would be safer
> > to not do this?
>
> It would be safer to leave it, but replace the start_stack use as you did.

OK, thanks.

> And I think I'll let Linus's guard page justify your 4 (to match comment)
> in place of the original's mysterious 5.

Ah, thanks again. Yes, if we want to guarantee 4 pages we should check 5.

Although obviously this doesn't explain the original's 5, this was
written before check_stack_guard_page().

OK.

Oleg.

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Hugh Dickins]

On Mon, 25 Aug 2014, Oleg Nesterov wrote:
> On 08/25, Hugh Dickins wrote:
> 
> > And I think I'll let Linus's guard page justify your 4 (to match comment)
> > in place of the original's mysterious 5.
> 
> Ah, thanks again. Yes, if we want to guarantee 4 pages we should check 5.
> 
> Although obviously this doesn't explain the original's 5, this was
> written before check_stack_guard_page().

I meant, you changed the coded 5 to 4, to match the comment above it;
and if I were insistent on maximizing back-compatible-safety, I would
say that 4 should go back to 5; but since Linus added the guard page
gap at next->vm_start in between, I don't mind if you stick with 4
there to match the comment.

Disclaimer: I may be mistaken in each clause of that paragraph,
and you may prefer to ignore everything I wrote there as worthless!
But I didn't understand your "to guarantee 4 we should check 5".

Hugh

Re: [PATCH 1/1] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/25, Hugh Dickins wrote:
>
> On Mon, 25 Aug 2014, Oleg Nesterov wrote:
> > On 08/25, Hugh Dickins wrote:
> >
> > > And I think I'll let Linus's guard page justify your 4 (to match comment)
> > > in place of the original's mysterious 5.
> >
> > Ah, thanks again. Yes, if we want to guarantee 4 pages we should check 5.
> >
> > Although obviously this doesn't explain the original's 5, this was
> > written before check_stack_guard_page().
>
> I meant, you changed the coded 5 to 4, to match the comment above it;
> and if I were insistent on maximizing back-compatible-safety, I would
> say that 4 should go back to 5;

I already did this ;) I'll send v2 patch in a minute.

> But I didn't understand your "to guarantee 4 we should check 5".

I just tried to say that I fully agree with your point.

I didn't dare to preserve your acks although the patch is almost the
same, I'll appreciate it if you can ack v2 too.

Oleg.

[PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

The ->start_stack check in do_shmat() looks ugly and simply wrong.

1. ->start_stack is only valid right after exec(), the application
   can switch to another stack and even unmap this area. Or a stack
   can simply grow, ->start_stack won't even notice this.

2. The reason for this check is not clear at all. The application
   should know what it does. And why 4 pages? And why in fact it
   requires 5 pages? Plus "start_stack - size - PAGE_SIZE * 5" can
   underflow although this is minor.

   As Hugh pointed out, we actually need to require the additional
   guard page, but this code was written before linux had it.

3. This wrongly assumes that the stack can only grown down.

Personally I think we should simply kill this check, but I did not
dare to do this. So the patch only fixes the 1st problem (mostly to
avoid the usage of mm->start_stack) and ignores the VM_GROWSUP case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 ipc/shm.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 7fc9f9f..9a322f5 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1166,19 +1166,25 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 
 	down_write(&current->mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
-		err = -EINVAL;
-		if (addr + size < addr)
-			goto invalid;
+		struct vm_area_struct *vma;
 
-		if (find_vma_intersection(current->mm, addr, addr + size))
-			goto invalid;
+		err = -EINVAL;
 		/*
-		 * If shm segment goes below stack, make sure there is some
-		 * space left for the stack to grow (at least 4 pages).
+		 * Ensure this segment doesn't overlap with the next vma.
+		 * If it is stack, make sure there is some space left for
+		 * the stack to grow, at least 4 pages plus a guard page
+		 * enforced by check_stack_guard_page(). (Why?)
 		 */
-		if (addr < current->mm->start_stack &&
-		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
-			goto invalid;
+		vma = find_vma(current->mm, addr);
+		if (vma) {
+			unsigned long end = addr + size;
+
+			if (vma->vm_flags & VM_GROWSDOWN)
+				end += PAGE_SIZE * 5;
+
+			if (end < addr || end > vma->vm_start)
+				goto invalid;
+		}
 	}
 
 	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
-- 
1.5.5.1

Re: [PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

[Cyrill Gorcunov]

On Mon, Aug 25, 2014 at 09:12:07PM +0200, Oleg Nesterov wrote:
> +		vma = find_vma(current->mm, addr);
> +		if (vma) {
> +			unsigned long end = addr + size;
> +
> +			if (vma->vm_flags & VM_GROWSDOWN)
> +				end += PAGE_SIZE * 5;
> +
> +			if (end < addr || end > vma->vm_start)
> +				goto invalid;

Looks good to me, but I somehow missed in first version of the patch
too the following aspect -- end > vma->vm_start? Maybe end >= vma->vm_end?

Re: [PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

[Hugh Dickins]

On Mon, 25 Aug 2014, Oleg Nesterov wrote:

> The ->start_stack check in do_shmat() looks ugly and simply wrong.
> 
> 1. ->start_stack is only valid right after exec(), the application
>    can switch to another stack and even unmap this area. Or a stack
>    can simply grow, ->start_stack won't even notice this.
> 
> 2. The reason for this check is not clear at all. The application
>    should know what it does. And why 4 pages? And why in fact it
>    requires 5 pages? Plus "start_stack - size - PAGE_SIZE * 5" can
>    underflow although this is minor.
> 
>    As Hugh pointed out, we actually need to require the additional
>    guard page, but this code was written before linux had it.
> 
> 3. This wrongly assumes that the stack can only grown down.
> 
> Personally I think we should simply kill this check, but I did not
> dare to do this. So the patch only fixes the 1st problem (mostly to
> avoid the usage of mm->start_stack) and ignores the VM_GROWSUP case.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Sorry, I cannot ack this, because your comment below "at least 4 pages
plus a guard page enforced by check_stack_guard_page()" makes no sense
to me as an explanation for the 5.  The guard page (gap) enforced by
check_stack_guard_page() is already at vma->vm_start (and I just
verified that, though wasted some time on it not behaving as I had
expected, until I found show_map_vma()'s start += PAGE_SIZE hides it).

But in the course of trying to understand what I saw in /proc/<pid>/maps,
I did come across 2.6.34's 128k stack_expand inherited from 2.6.11's
20 page EXTRA_STACK_VM_PAGES.  With Linus's guard page enforcing a
page gap since 2.6.36.

This shmat() 4 or 5 page gap dates from long before all of those
were added.  That, together with your preference, and our difficulty
in communicating a sensible way of updating and describing the test,
now drives me to agree with you.  Please just rip out the start_stack
test and the comment defending it.

Hugh

> ---
>  ipc/shm.c |   26 ++++++++++++++++----------
>  1 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7fc9f9f..9a322f5 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1166,19 +1166,25 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  
>  	down_write(&current->mm->mmap_sem);
>  	if (addr && !(shmflg & SHM_REMAP)) {
> -		err = -EINVAL;
> -		if (addr + size < addr)
> -			goto invalid;
> +		struct vm_area_struct *vma;
>  
> -		if (find_vma_intersection(current->mm, addr, addr + size))
> -			goto invalid;
> +		err = -EINVAL;
>  		/*
> -		 * If shm segment goes below stack, make sure there is some
> -		 * space left for the stack to grow (at least 4 pages).
> +		 * Ensure this segment doesn't overlap with the next vma.
> +		 * If it is stack, make sure there is some space left for
> +		 * the stack to grow, at least 4 pages plus a guard page
> +		 * enforced by check_stack_guard_page(). (Why?)
>  		 */
> -		if (addr < current->mm->start_stack &&
> -		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> -			goto invalid;
> +		vma = find_vma(current->mm, addr);
> +		if (vma) {
> +			unsigned long end = addr + size;
> +
> +			if (vma->vm_flags & VM_GROWSDOWN)
> +				end += PAGE_SIZE * 5;
> +
> +			if (end < addr || end > vma->vm_start)
> +				goto invalid;
> +		}
>  	}
>  
>  	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
> -- 
> 1.5.5.1

Re: [PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/25, Hugh Dickins wrote:
>
> On Mon, 25 Aug 2014, Oleg Nesterov wrote:
>
> >    As Hugh pointed out, we actually need to require the additional
> >    guard page, but this code was written before linux had it.
> >
> > 3. This wrongly assumes that the stack can only grown down.
> >
> > Personally I think we should simply kill this check, but I did not
> > dare to do this. So the patch only fixes the 1st problem (mostly to
> > avoid the usage of mm->start_stack) and ignores the VM_GROWSUP case.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Sorry, I cannot ack this,

Hugh, I appreciate you nack even more. Thanks!

> because your comment below "at least 4 pages
> plus a guard page enforced by check_stack_guard_page()" makes no sense
> to me as an explanation for the 5.  The guard page (gap) enforced by
> check_stack_guard_page() is already at vma->vm_start
                              ^^^^^^^^^^^^^^^^^^^^^^^^
Ahh. Yes I misunderstood this logic. And yes, you tried to explain it
twice but I was too stupid.

> I did come across 2.6.34's 128k stack_expand inherited from 2.6.11's
> 20 page EXTRA_STACK_VM_PAGES.  With Linus's guard page enforcing a
> page gap since 2.6.36.

OK, and setup_arg_pages() still does expand_stack() although stack_expand
depends on RLIMIT_STACK. But I think this doesn't matter. The room was
already reserved, in general mm->start_stack points into the middle of
the stack.

So that check in do_shmat() can only help if the stack was not expanded
due to the low RLIMIT_STACK, then this application raises RLIMIT_STACK,
then does do_shmat(). But in this case it should likely crash when exec
returns to usermode.

> and our difficulty
> in communicating a sensible way of updating and describing the test,

I like your polite interpretation of my ignorance and inability to listen ;)

> now drives me to agree with you.  Please just rip out the start_stack
> test and the comment defending it.

Great. Will do.

Oleg.

[PATCH v3] ipc/shm: kill the historical/wrong mm->start_stack check

[Oleg Nesterov]

do_shmat() is the only user of ->start_stack (proc just reports its
value), and this check looks ugly and wrong.

The reason for this check is not clear at all, and it wrongly assumes
that the stack can only grow down.

But the main problem is that in general mm->start_stack has nothing
to do with stack_vma->vm_start. Not only the application can switch
to another stack and even unmap this area, setup_arg_pages() expands
the stack without updating mm->start_stack during exec(). This means
that in the likely case "addr > start_stack - size - PAGE_SIZE * 5"
is simply impossible after find_vma_intersection() == F, or the stack
can't grow anyway because of RLIMIT_STACK.

Many thanks to Hugh for his explanations.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 ipc/shm.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 7fc9f9f..0145479 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1172,13 +1172,6 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 
 		if (find_vma_intersection(current->mm, addr, addr + size))
 			goto invalid;
-		/*
-		 * If shm segment goes below stack, make sure there is some
-		 * space left for the stack to grow (at least 4 pages).
-		 */
-		if (addr < current->mm->start_stack &&
-		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
-			goto invalid;
 	}
 
 	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
-- 
1.5.5.1

Re: [PATCH v2] ipc/shm: fix the historical/wrong mm->start_stack check

[Oleg Nesterov]

On 08/26, Cyrill Gorcunov wrote:
>
> On Mon, Aug 25, 2014 at 09:12:07PM +0200, Oleg Nesterov wrote:
> > +		vma = find_vma(current->mm, addr);
> > +		if (vma) {
> > +			unsigned long end = addr + size;
> > +
> > +			if (vma->vm_flags & VM_GROWSDOWN)
> > +				end += PAGE_SIZE * 5;
> > +
> > +			if (end < addr || end > vma->vm_start)
> > +				goto invalid;
>
> Looks good to me, but I somehow missed in first version of the patch
> too the following aspect -- end > vma->vm_start? Maybe end >= vma->vm_end?

I think ">" is correct, the last byte is "addr + size - 1". Otherwise,
say, find_vma_intersection() is buggy. But this doesn't matter, please
see v3.

Oleg.

Re: [PATCH v3] ipc/shm: kill the historical/wrong mm->start_stack check

[Hugh Dickins]

On Tue, 26 Aug 2014, Oleg Nesterov wrote:

> do_shmat() is the only user of ->start_stack (proc just reports its
> value), and this check looks ugly and wrong.
> 
> The reason for this check is not clear at all, and it wrongly assumes
> that the stack can only grow down.
> 
> But the main problem is that in general mm->start_stack has nothing
> to do with stack_vma->vm_start. Not only the application can switch
> to another stack and even unmap this area, setup_arg_pages() expands
> the stack without updating mm->start_stack during exec(). This means
> that in the likely case "addr > start_stack - size - PAGE_SIZE * 5"
> is simply impossible after find_vma_intersection() == F, or the stack
> can't grow anyway because of RLIMIT_STACK.
> 
> Many thanks to Hugh for his explanations.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Hugh Dickins <hughd@google.com>

But you're much too generous to me: I never even noticed how exec's
expand_stack() comes *after* setting start_stack, so that the shmat()
check is nowadays utterly irrelevant.  Thank you for persisting and
observing that and finally nailing down this coffin!

> ---
>  ipc/shm.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7fc9f9f..0145479 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1172,13 +1172,6 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  
>  		if (find_vma_intersection(current->mm, addr, addr + size))
>  			goto invalid;
> -		/*
> -		 * If shm segment goes below stack, make sure there is some
> -		 * space left for the stack to grow (at least 4 pages).
> -		 */
> -		if (addr < current->mm->start_stack &&
> -		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> -			goto invalid;
>  	}
>  
>  	addr = do_mmap_pgoff(file, addr, size, prot, flags, 0, &populate);
> -- 
> 1.5.5.1