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

#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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web