ipc/shm: fix the historical/wrong mm->start_stack check
cover ipc/shm: fix the historical/wrong mm->start_stack check
Quoted 3 lines · Oleg Nesterov
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
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.
--- ipc/shm.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-)
ipc/shm.c +14 -9
@@ -1166,19 +1166,24 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,down_write (¤t->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);
Signature · 1 line
1.5.5.1
Quoted 9 lines · 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?
Quoted 7 lines
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 ignoresVM_GROWSUP.Signed-off-by Oleg Nesterov <[email protected]>
I don't understand this check either, the comment above it says nothing but only commits what code is doing not explaining why.
Quoted 11 lines · Cyrill Gorcunov
Quoted 9 lines · 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?
Quoted 8 lines
Quoted 7 lines
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 ignoresVM_GROWSUP.Signed-off-by Oleg Nesterov <[email protected]>Reviewed-by Cyrill Gorcunov <[email protected]>
Thanks!
Quoted 2 lines
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 intogit://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.
Hugh Dickins
Acked-byQuoted 12 lines · Oleg Nesterov
Quoted 11 lines · Cyrill Gorcunov
Quoted 9 lines · 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?
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
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 ignoresVM_GROWSUP.Signed-off-by Oleg Nesterov <[email protected]>Reviewed-by Cyrill Gorcunov <[email protected]>
Yes, much better to use find_vma than have this strange stray use of unreliable start_stack.
though like Manfred I didn't quite see how overflow was impossible on unfamiliar architectures.
Quoted 11 lines
Thanks!
Quoted 2 lines
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.gitbut 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 connectsSHM_REMAPitself).
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
Quoted 20 lines · Hugh Dickins
Quoted 18 lines · Oleg Nesterov
Quoted 17 lines · Cyrill Gorcunov
Quoted 15 lines · 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 ignoresVM_GROWSUP.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>Reviewed-by Cyrill Gorcunov <[email protected]>Yes, much better to use
find_vmathan have this strange stray use of unreliablestart_stack.Acked-by Hugh Dickins <[email protected]>
Thanks!
Quoted 2 lines
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.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
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.gitbut 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 connectsSHM_REMAPitself).
Quoted 5 lines
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,
Quoted 1 line
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.
Quoted 9 lines · Oleg Nesterov
Quoted 6 lines · Hugh Dickins
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).
Quoted 5 lines
Quoted 2 lines
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
Quoted 24 lines · Hugh Dickins
Quoted 9 lines · Oleg Nesterov
Quoted 6 lines · Hugh Dickins
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 thevm_startof aVM_GROWSDOWNsomewhat elastically.It may be that Linus's
check_stack_guard_page()work in 2.6.36 changed the importance of thisshmat()check, but I'd still feel safer to leave it as is (while turning a blind eye to theVM_GROWSUPomission).Quoted 1 line
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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_stackuse as you did.
OK, thanks.
Quoted 2 lines
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.
Quoted 9 lines · Oleg Nesterov
Quoted 2 lines · Hugh Dickins
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
Quoted 7 lines · Hugh Dickins
Quoted 5 lines · Oleg Nesterov
Quoted 2 lines · Hugh Dickins
And I think I'll let Linus's guard page justify your 4 (to match comment) in place of the original's mysterious 5.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 9 lines · 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?
Quoted 7 lines
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 ignoresVM_GROWSUP.Signed-off-by Oleg Nesterov <[email protected]>
ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1183
Quoted 5 lines
[snip]
ipc/shm.c +3
-0+if (vma) { +if (vma->vm_flags &VM_GROWSDOWN ) + end +=PAGE_SIZE *4 ;/* can't overflow */
Why is an overflow impossible?
ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1189
Quoted 6 lines
ipc/shm.c +3
-0+if (end > vma->vm_start) + goto invalid; + } } addr =do_mmap_pgoff (file, addr, size, prot, flags,0 , &populate);
--
Manfred
Quoted 11 lines · Manfred Spraul
Quoted 9 lines · 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?
Quoted 8 lines
Quoted 7 lines
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 ignoresVM_GROWSUP.Signed-off-by Oleg Nesterov <[email protected]>Acked-by Manfred Spraul <[email protected]>
Thanks!
Quoted 4 lines
ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1183
Quoted 3 lines
ipc/shm.c +3
-0+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.
PATCH v2 1/1 ipc/shm: fix the historical/wrong mm->start_stack check
In reply to Oleg NesterovThe ->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.
--- ipc/shm.c | 26 ++++++++++++++++---------- 1 files changed, 16 insertions(+), 10 deletions(-)
ipc/shm.c +16 -10
@@ -1166,19 +1166,25 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,down_write (¤t->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);
Signature · 1 line
1.5.5.1
ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1186
Quoted 9 lines · Oleg Nesterov
ipc/shm.c +9
-0+ 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?
Quoted 14 lines · Cyrill Gorcunov
ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1186
Quoted 9 lines · Oleg Nesterov
ipc/shm.c +9
-0+ 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.
Quoted 14 lines · 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.
Quoted 7 lines
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 theVM_GROWSUPcase.Signed-off-by Oleg Nesterov <[email protected]>
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
Quoted 46 lines
--- ipc/shm.c | 26 ++++++++++++++++---------- 1 files changed, 16 insertions(+), 10 deletions(-)ipc/shm.c +16
-10@@ -1166,19 +1166,25 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,down_write (¤t->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);Signature · 1 line
1.5.5.1
Quoted 8 lines · Hugh Dickins
Quoted 5 lines · Oleg Nesterov
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.
Quoted 7 lines
Quoted 5 lines
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 theVM_GROWSUPcase.Signed-off-by Oleg Nesterov <[email protected]>Sorry, I cannot ack this,
Hugh, I appreciate you nack even more. Thanks!
Quoted 4 lines
because your comment below "at least 4 pages
plus a guard page enforced bycheck_stack_guard_page()" makes no sense to me as an explanation for the 5. The guard page (gap) enforced bycheck_stack_guard_page()is already atvma->vm_start
^^^^^^^^^^^^^^^^^^^^^^^^
Ahh. Yes I misunderstood this logic. And yes, you tried to explain it twice but I was too stupid.
Quoted 3 lines
I did come across 2.6.34's 128k
stack_expandinherited from 2.6.11's 20 pageEXTRA_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.
Quoted 2 lines
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 ;)
Quoted 2 lines
now drives me to agree with you. Please just rip out the
start_stacktest and the comment defending it.
Great. Will do.
Oleg.
PATCH v3 1/1 ipc/shm: kill the historical/wrong mm->start_stack check
In reply to Oleg Nesterovdo_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.
--- ipc/shm.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)
ipc/shm.c +0 -7
@@ -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);
Signature · 1 line
1.5.5.1
Re: [PATCH v3] ipc/shm: kill the historical/wrong mm->start_stack check
Quoted 14 lines · 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_stackhas nothing to do withstack_vma->vm_start. Not only the application can switch to another stack and even unmap this area,setup_arg_pages()expands the stack without updatingmm->start_stackduringexec(). This means that in the likely case "addr >start_stack- size -PAGE_SIZE* 5" is simply impossible afterfind_vma_intersection()== F, or the stack can't grow anyway because ofRLIMIT_STACK.
Quoted 3 lines
Many thanks to Hugh for his explanations.
Signed-off-by Oleg Nesterov <[email protected]>
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!
Quoted 24 lines
--- ipc/shm.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)ipc/shm.c +0
-7@@ -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);Signature · 1 line
1.5.5.1