Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676942 > unrolled thread

[PATCH 0/1] expand_downwards: don't require the gap if !vm_prev

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Oleg Nesterov <oleg@redhat.com> - 2017-06-28 20:00 +0200
    Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 01:30 +0200
      Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Oleg Nesterov <oleg@redhat.com> - 2017-06-29 17:20 +0200
        Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 20:30 +0200
          Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Oleg Nesterov <oleg@redhat.com> - 2017-06-29 21:00 +0200
            Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 21:10 +0200
      Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Michal Hocko <mhocko@kernel.org> - 2017-06-30 15:30 +0200
        Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-30 19:10 +0200
          Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Michal Hocko <mhocko@kernel.org> - 2017-06-30 19:30 +0200
            Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-30 19:50 +0200
              Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Michal Hocko <mhocko@kernel.org> - 2017-07-03 17:50 +0200
                Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-03 18:40 +0200
                  Re: [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev Michal Hocko <mhocko@kernel.org> - 2017-07-03 19:00 +0200

#1676942 — [PATCH 0/1] expand_downwards: don't require the gap if !vm_prev

See the patch, but actually I have another question...

Now that the stack-guard-page has gone, why do we need to allow to grow
into the previous VM_GROWSDOWN vma? IOW, why we can not simply remove
the VM_GROWSDOWN check in expand_downwards() ?

Yes, this is what the kernel did before the recent changes. But afaics
only because the kernel could not know if the vma->vm_start page is
actually guard or not.

IOW, iiuc before the recent change it was not simple to _disallow_ this,
and that is why it worked. Just for example, suppose an application does

	addr = mmap(MAP_GROWSDOWN);
	mprotect(addr, PAGE_SIZE, PROT_NONE);
	*(addr + PAGE_SIZE) = 0;

and of course this should not fail.

But the the kernel could not know if vm_start == addr + PAGE_SIZE is the
"valid" address, or this vma was expanded before and vm_start is the stack
guard.

Yes, we can probably check anon_vma's as the comment suggests, but imo we
we can just remove the VM_GROWSDOWN case unconditionally.

Oleg.

[toc] | [next] | [standalone]

#1677273

On Wed, Jun 28, 2017 at 10:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Now that the stack-guard-page has gone, why do we need to allow to grow
> into the previous VM_GROWSDOWN vma? IOW, why we can not simply remove
> the VM_GROWSDOWN check in expand_downwards() ?

Because the "prev" vma may actually be the original vma.

I think I described it in an earlier thread, but what happened at
least once was:

 - program has some part that uses a lot of stack for part of the
execution for some temp buffer or deep recursion or whatever

 - somebody noticed this, and decided to free up the no-longer-used
pages by doing a "munmap()" after the program was done with that part
of the stack

 - but the "munmap()" wasn't complete (maybe it only accounted for the
explicitly used buffer, whatever), so the munmap actually didn't just
remove the no-longer used bottom of the stack, it actually split the
stack segment into two (with a small remaining stack turd that was the
*real* bottom of the deep stack that used to exist)

 - you do want to be able to grow the stack back to what it was, and
it needs to be able to meet that old turd without triggering the
"oops, guard area!" logic.

As to your patch: I would prefer to actually keep the new failure
behavior of unconditionally breaking a big stack expansion), unless
there's an actual thing it breaks.

In fact, I'd even be quite open to adding a kernel warning about badly
behaved binaries that grow their stack by a big amount in one go. Not
only is it bad taste (and we really should encourage compilers to do
probing every page when growing the stack), but it migth be a sign of
an attempted attack.

                Linus

[toc] | [prev] | [next] | [standalone]

#1677855

On 06/28, Linus Torvalds wrote:
>
> On Wed, Jun 28, 2017 at 10:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Now that the stack-guard-page has gone, why do we need to allow to grow
> > into the previous VM_GROWSDOWN vma? IOW, why we can not simply remove
> > the VM_GROWSDOWN check in expand_downwards() ?
>
> Because the "prev" vma may actually be the original vma.
>
> I think I described it in an earlier thread, but what happened at
> least once was:
>
>  - program has some part that uses a lot of stack for part of the
> execution for some temp buffer or deep recursion or whatever
>
>  - somebody noticed this, and decided to free up the no-longer-used
> pages by doing a "munmap()" after the program was done with that part
> of the stack
>
>  - but the "munmap()" wasn't complete (maybe it only accounted for the
> explicitly used buffer, whatever), so the munmap actually didn't just
> remove the no-longer used bottom of the stack, it actually split the
> stack segment into two (with a small remaining stack turd that was the
> *real* bottom of the deep stack that used to exist)

Ah, OK, thanks...



> As to your patch: I would prefer to actually keep the new failure
> behavior of unconditionally breaking a big stack expansion), unless
> there's an actual thing it breaks.

Hmm. May be you misread this patch? Or I misunderstood.

> In fact, I'd even be quite open to adding a kernel warning about badly
> behaved binaries that grow their stack by a big amount in one go.

Yes, but this is another story.

Currently expand_downwards(address) does

	if (address < stack_guard_gap)
		return -ENOMEM;

This has nothing to do with "by how much it needs to grow", this simply
forbids the bottom of stack below stack_guard_gap. Why?

I don't think this patch can make any difference in practice, it just
tries to make this logic more consistent/understandable.

For example. Suppose that stack_guard_gap = 1M (default). Now,

	addr = 512K;  // any addr <= stack_guard_gap;
	char *stack = mmap(addr, MAP_FIXED|MAP_GROWSDOWN, PAGE_SIZE);

	*stack = 0;
	stack -= PAGE_SIZE;
	*stack = 0;

The first store will always succeed, the 2nd one will always fail even
if (likely) there is no another vma below. This looks strange to me.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1678043

On Thu, Jun 29, 2017 at 8:19 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Hmm. May be you misread this patch?

Ahh, yes. I'm ok with your patch.

That said, you did remove something extra: the comment about

    /* Check that both stack segments have the same anon_vma? */

is actually still relevant wrt that VM_GROWSDOWN test. The issue is
that we could actually limit the VM_GROWSDOWN thing to only be ok with
merging with a previous vma only if it *really* was the same segment.

And I think we could do that by checking the anon-vma (or maybe the vm_offset?)

                  Linus

[toc] | [prev] | [next] | [standalone]

#1678069

On 06/29, Linus Torvalds wrote:
>
> On Thu, Jun 29, 2017 at 8:19 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Hmm. May be you misread this patch?
>
> Ahh, yes. I'm ok with your patch.
>
> That said, you did remove something extra: the comment about
>
>     /* Check that both stack segments have the same anon_vma? */

I didn't ;) I moved it up, right above VM_GROWSDOWN check.


> is actually still relevant wrt that VM_GROWSDOWN test. The issue is
> that we could actually limit the VM_GROWSDOWN thing to only be ok with
> merging with a previous vma only if it *really* was the same segment.

Yes, yes, this is clear. This comment motivated me to ask that question,
I thought that we probably do not need to reconcile the stacks even in
this case.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1678094

On Thu, Jun 29, 2017 at 11:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> I didn't ;) I moved it up, right above VM_GROWSDOWN check.

I think I will just need to take a long nap, I'm clearly not tracking
the patches very well.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1678825

On Wed 28-06-17 16:26:33, Linus Torvalds wrote:
[...]
> In fact, I'd even be quite open to adding a kernel warning about badly
> behaved binaries that grow their stack by a big amount in one go. Not
> only is it bad taste (and we really should encourage compilers to do
> probing every page when growing the stack), but it migth be a sign of
> an attempted attack.

FWIW our gcc guys shown an interest in having something to tell the
kernel how much the stack can grow at once. They want it for testing of
the new stack probing alloca implementation. I have something
preliminary with /proc/<pid>/stack_expand_limit for the internal testing
purpose but maybe there will be more interest for this. I didn't plan to
post it public because it basically duplicates the stack_gap but it is
also true that it can help some applications which won't use the proposed
__save_alloca (or whatever it will be) without increasing stack_gap too
much.

What do you think?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1679002

On Fri, Jun 30, 2017 at 6:24 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> FWIW our gcc guys shown an interest in having something to tell the
> kernel how much the stack can grow at once. They want it for testing of
> the new stack probing alloca implementation.

Here, I made this just for them:

   #define STACK_GROWTH_SIZE (4096)

isn't that beautiful? A new kernel interface without some stupid sysfs
file for it.

And the added advantage is that it compiles to nice dense code too.

> I have something
> preliminary with /proc/<pid>/stack_expand_limit for the internal testing
> purpose but maybe there will be more interest for this. I

NO NO NO.

I absolutely refuse to see the stack gap as some kind of "this is how
much you can grow the stack without probing".

That's complete and utter garbage.

Tell them that they can grow the stack by 4kB. That's it. If they want
to get all fancy, and they say that they really want an
architecture-specific value, tell them to use getpagesize().

The stack gap is there due to the ABI being broken. If we're fixing
the ABI, then the stack gap has *nothing* to add.

Don't encourage shit.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1679018

On Fri 30-06-17 10:08:03, Linus Torvalds wrote:
> On Fri, Jun 30, 2017 at 6:24 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > FWIW our gcc guys shown an interest in having something to tell the
> > kernel how much the stack can grow at once. They want it for testing of
> > the new stack probing alloca implementation.
> 
> Here, I made this just for them:
> 
>    #define STACK_GROWTH_SIZE (4096)
> 
> isn't that beautiful? A new kernel interface without some stupid sysfs
> file for it.
> 
> And the added advantage is that it compiles to nice dense code too.
> 
> > I have something
> > preliminary with /proc/<pid>/stack_expand_limit for the internal testing
> > purpose but maybe there will be more interest for this. I
> 
> NO NO NO.
> 
> I absolutely refuse to see the stack gap as some kind of "this is how
> much you can grow the stack without probing".
> 
> That's complete and utter garbage.
> 
> Tell them that they can grow the stack by 4kB. That's it. If they want
> to get all fancy, and they say that they really want an
> architecture-specific value, tell them to use getpagesize().

Ohh, you misunderstood I guess. They wanted that only for internal
testing (e.g. make sure that everything that matters blows up if it is
doing something wrong). Absolutely nothing to base any compilator
decistion on.

> The stack gap is there due to the ABI being broken. If we're fixing
> the ABI, then the stack gap has *nothing* to add.

Yeah I know. The only usecase why I thought this might be interesting is
when you run the code you haven't compiled with a compiler which does
the proper thing. Think of all the 3rd party stuff that you eventually
_need_ to run. Then you have also a hard guess to tune your gap for. If
you can blow on/warn about unexpectedly large stack expansions then you
can protect that particular piece of SW.

But as I've said, this is not something I planned to post upstream but
you mentioning a warning made me think that I could just mention it.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1679036

On Fri, Jun 30, 2017 at 10:26 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> Ohh, you misunderstood I guess. They wanted that only for internal
> testing (e.g. make sure that everything that matters blows up if it is
> doing something wrong). Absolutely nothing to base any compilator
> decistion on.

Oh, good.

If that's the case, I really think we should try to add some code that
checks that the stack grows strictly one page at a time, and have a
way to enable SIGSEGV if that is ever not the case.

That should be trivial to add in expand_downwards/expand_upwards.

We could make a "warn once" thing unconditional for distro testing,
but since compiler people would presumably want to test this before
the rest of the distro is clean, they'd need some rlimit or something
like that to enable it for particular processes.

Would that be ok for them?

Some prctl to get/set that "max I'm allowed to extend the stack"?

                Linus

[toc] | [prev] | [next] | [standalone]

#1680460

On Fri 30-06-17 10:48:15, Linus Torvalds wrote:
> On Fri, Jun 30, 2017 at 10:26 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > Ohh, you misunderstood I guess. They wanted that only for internal
> > testing (e.g. make sure that everything that matters blows up if it is
> > doing something wrong). Absolutely nothing to base any compilator
> > decistion on.
> 
> Oh, good.
> 
> If that's the case, I really think we should try to add some code that
> checks that the stack grows strictly one page at a time, and have a
> way to enable SIGSEGV if that is ever not the case.
> 
> That should be trivial to add in expand_downwards/expand_upwards.

yes, but I would be worried to have this hardcoded. People very often do
run 3rd party code compiled with a non-default distribution compiler. I
also expect there are sensible usecases where probing on the stack could
lead to a performance degradation so people might want to explicitely
disable it.

> We could make a "warn once" thing unconditional for distro testing,
> but since compiler people would presumably want to test this before
> the rest of the distro is clean, they'd need some rlimit or something
> like that to enable it for particular processes.

yes, WARN_ONCE is not very practical to identify offenders..
 
> Would that be ok for them?
> 
> Some prctl to get/set that "max I'm allowed to extend the stack"?

prctl would be less code than proc interface I've had and quite
straightforward as well. Below is what I have ended up with for them. I
doesn't warn by default because I thought it would be too noisy without
stack probing used more widely.

If you think this is worth pursuing in upstream, just let me know and I
can polish it, add a patch for the man page and other things.
---
diff --git a/fs/exec.c b/fs/exec.c
index 904199086490..7589fb701fca 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -320,6 +320,13 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 	arch_bprm_mm_init(mm, vma);
 	up_write(&mm->mmap_sem);
 	bprm->p = vma->vm_end - sizeof(void *);
+
+	/*
+	 * We cannot set the stack expand limit now because we will do manual
+	 * stack expansions which might be larger. See setup_arg_pages
+	 */
+	if (current->mm)
+		bprm->expand_stack_limit = current->mm->expand_stack_limit;
 	return 0;
 err:
 	up_write(&mm->mmap_sem);
@@ -786,6 +793,14 @@ int setup_arg_pages(struct linux_binprm *bprm,
 	if (ret)
 		ret = -EFAULT;
 
+	/*
+	 * Stack is finilized now and so all later expansions have
+	 * to comply with the inherited limit.
+	 *
+	 * TODO: Do we want to allow non-privileged task to set
+	 * the limit for privileged ones?
+	 */
+	vma->vm_mm->expand_stack_limit = bprm->expand_stack_limit;
 out_unlock:
 	up_write(&mm->mmap_sem);
 	return ret;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 05488da3aee9..2d3d7fff4811 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -17,6 +17,7 @@ struct linux_binprm {
 	char buf[BINPRM_BUF_SIZE];
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
+	unsigned long expand_stack_limit;
 	unsigned long vma_pages;
 #else
 # define MAX_ARG_PAGES	32
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 45cdb27791a3..24ee38a45a9e 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -425,6 +425,7 @@ struct mm_struct {
 	unsigned long start_brk, brk, start_stack;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
+	unsigned long expand_stack_limit;	/* how much we can grow stack at once */
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
 
 	/*
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index a8d0759a9e40..6f3c65530c71 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -197,4 +197,7 @@ struct prctl_mm_map {
 # define PR_CAP_AMBIENT_LOWER		3
 # define PR_CAP_AMBIENT_CLEAR_ALL	4
 
+#define PR_SET_STACK_EXPAND_LIMIT 48
+#define PR_GET_STACK_EXPAND_LIMIT 49
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 4111d584ff4a..05bdcdeda98f 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2290,6 +2290,17 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_GET_FP_MODE:
 		error = GET_FP_MODE(me);
 		break;
+	case PR_SET_STACK_EXPAND_LIMIT:
+		if (arg3 || arg4 || arg5)
+			return -EINVAL;
+		if (down_write_killable(&me->mm->mmap_sem))
+			return -EINTR;
+		me->mm->expand_stack_limit = arg2;
+		up_write(&me->mm->mmap_sem);
+		break;
+	case PR_GET_STACK_EXPAND_LIMIT:
+		error = me->mm->expand_stack_limit;
+		break;
 	default:
 		error = -EINVAL;
 		break;
diff --git a/mm/mmap.c b/mm/mmap.c
index 5a0ba9788cdd..ff2a5981ee92 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2266,7 +2266,16 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 		unsigned long size, grow;
 
 		size = address - vma->vm_start;
-		grow = (address - vma->vm_end) >> PAGE_SHIFT;
+		grow = address - vma->vm_end;
+
+		if (mm->expand_stack_limit && grow > mm->expand_stack_limit) {
+			pr_warn("%s[%d]: disallowed stack expansion %lu with limit %lu\n",
+					current->comm, task_pid_nr(current),
+					grow, mm->expand_stack_limit);
+			anon_vma_unlock_write(vma->anon_vma);
+			return -ENOMEM;
+		}
+		grow >>= PAGE_SHIFT;
 
 		error = -ENOMEM;
 		if (vma->vm_pgoff + (size >> PAGE_SHIFT) >= vma->vm_pgoff) {
@@ -2350,7 +2359,20 @@ int expand_downwards(struct vm_area_struct *vma,
 		unsigned long size, grow;
 
 		size = vma->vm_end - address;
-		grow = (vma->vm_start - address) >> PAGE_SHIFT;
+		grow = vma->vm_start - address;
+
+		/*
+		 * Make sure that a single stack expansion doesn't exceed the
+		 * configured limit.
+		 */
+		if (mm->expand_stack_limit && grow > mm->expand_stack_limit) {
+			pr_warn("%s[%d]: disallowed stack expansion %lu with limit %lu\n",
+					current->comm, task_pid_nr(current),
+					grow, mm->expand_stack_limit);
+			anon_vma_unlock_write(vma->anon_vma);
+			return -ENOMEM;
+		}
+		grow >>= PAGE_SHIFT;
 
 		error = -ENOMEM;
 		if (grow <= vma->vm_pgoff) {
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1680486

On Mon, Jul 3, 2017 at 8:49 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> If you think this is worth pursuing in upstream, just let me know and I
> can polish it, add a patch for the man page and other things.

Hmm. This doesn't look bad, except the bprm games there really look annoying.

Also, I'm wondering whether this should be per-thread - conceptually
"expand_stack()" really is a thread thing. All callers are using
"current", although it's not always obvious.

So I'm wondering if a slightly larger patch that simply made the
"limit" be an _argument_ to expand_stack() would clean up both of
these issues. The execve() use would simply pass in the stack limit,
and the fault users would pass in "current->expand_stack_limit".

Again, I'm not sure how many people really use multiple GROW_DOWN
stacks for threading, but it's conceptually the right thing to do, so
I think conceptually this should be per-thread. And the fact that it
might clean up the execve() thing makes me think it's the right thing
to do.

What do you think?

                 Linus

[toc] | [prev] | [next] | [standalone]

#1680495

On Mon 03-07-17 09:30:35, Linus Torvalds wrote:
> On Mon, Jul 3, 2017 at 8:49 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > If you think this is worth pursuing in upstream, just let me know and I
> > can polish it, add a patch for the man page and other things.
> 
> Hmm. This doesn't look bad, except the bprm games there really look annoying.
> 
> Also, I'm wondering whether this should be per-thread - conceptually
> "expand_stack()" really is a thread thing. All callers are using
> "current", although it's not always obvious.
> 
> So I'm wondering if a slightly larger patch that simply made the
> "limit" be an _argument_ to expand_stack() would clean up both of
> these issues. The execve() use would simply pass in the stack limit,
> and the fault users would pass in "current->expand_stack_limit".
> 
> Again, I'm not sure how many people really use multiple GROW_DOWN
> stacks for threading, but it's conceptually the right thing to do, so
> I think conceptually this should be per-thread. And the fact that it
> might clean up the execve() thing makes me think it's the right thing
> to do.
> 
> What do you think?

I am not sure about the per-thread vs. per mm part. If for nothing else,
MAP_GROWSDOWN can be something else than the main thread stack which can be
modified by all threads and then the semantic would be quite surprising
if different threads had a different idea about the expansion. No?

But an additional argument to expand_stack would surely clean things
up a bit and will get rid of the ugly bprm part as well. I will think
about it some more and then post the patch to linux-api to have a larger
audience.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web