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 <o...@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(¤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);
> --
> 1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
