Messages in this thread | | | Date | Mon, 3 Jul 2023 09:08:37 +0200 | Subject | Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long | From | Helge Deller <> |
| |
Hi Linus,
On 7/3/23 08:20, Linus Torvalds wrote: > On Sun, 2 Jul 2023 at 22:33, Guenter Roeck <linux@roeck-us.net> wrote: >> >> Here you are: >> >> [ 31.188688] stack expand failed: ffeff000-fff00000 (ffefeff2) > > Ahhah! > > I think the problem is actually ridiculously simple. > > The thing is, the parisc stack expands upwards. That's obvious. I've > mentioned it several times in just this thread as being the thing that > makes parisc special. > > But it's *so* obvious that I didn't even think about what it really implies. > > And part of all the changes was this part in expand_downwards(): > > if (!(vma->vm_flags & VM_GROWSDOWN)) > return -EFAULT; > > and that will *always* fail on parisc, because - as said multiple > times - the parisc stack expands upwards. It doesn't have VM_GROWSDOWN > set. > > What a dum-dum I am. > > And I did it that way because the *normal* stack expansion obviously > wants it that way and putting the check there not only made sense, but > simplified other code. > > But fs/execve.c is special - and only special for parisc - in that it > really wants to expand a normally upwards-growing stack downwards > unconditionally. > > Anyway, I think that new check in expand_downwards() is the right > thing to do, and the real fix here is to simply make vm_flags reflect > reality. > > Because during execve, that stack that will _eventually_ grow upwards, > does in fact grow downwards. Let's make it reflect that. > > We already do magical extra setup for the stack flags during setup > (VM_STACK_INCOMPLETE_SETUP), so extending that logic to contain > VM_GROWSDOWN seems sane and the right thing to do. > > IOW, I think a patch like the attached will fix the problem for real. > > It needs a good commit log and maybe a code comment or two, but before > I bother to do that, let's verify that yes, it does actually fix > things. > > In the meantime, I will actually go to bed, but I'm pretty sure this is it.
Great, that patch fixes it!
I wonder if you want to #define VM_STACK_EARLY VM_GROWSDOWN even for the case where the stack grows down too (instead of 0), just to make clear that in both cases the stack goes downwards initially.
Helge
| |