Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Naresh Kamboju]

On Fri, 30 Jun 2023 at 00:18, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> This is the start of the stable review cycle for the 6.4.1 release.
> There are 28 patches in this series, all will be posted as a response
> to this one.  If anyone has any issues with these being applied, please
> let me know.
>
> Responses should be made by Sat, 01 Jul 2023 18:41:39 +0000.
> Anything received after that time might be too late.
>
> The whole patch series can be found in one patch at:
>         https://www.kernel.org/pub/linux/kernel/v6.x/stable-review/patch-6.4.1-rc1.gz
> or in the git tree and branch at:
>         git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-6.4.y
> and the diffstat can be found below.
>
> thanks,
>
> greg k-h

Results from Linaro’s test farm.

Following build regression noticed on Linux stable-rc 6.4 and also noticed on
Linux mainline master.

Regressions found on Parisc and Sparc build failed:
 - build/gcc-11-defconfig

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

Parisc Build log:
=============
arch/parisc/mm/fault.c: In function 'do_page_fault':
arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in
this function)
  292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
      |                      ^~~~
arch/parisc/mm/fault.c:292:22: note: each undeclared identifier is
reported only once for each function it appears in


sparc Build log:
===========
<stdin>:1519:2: warning: #warning syscall clone3 not implemented [-Wcpp]
arch/sparc/mm/fault_32.c: In function 'force_user_fault':
arch/sparc/mm/fault_32.c:315:49: error: 'regs' undeclared (first use
in this function)
  315 |         vma = lock_mm_and_find_vma(mm, address, regs);
      |                                                 ^~~~
arch/sparc/mm/fault_32.c:315:49: note: each undeclared identifier is
reported only once for each function it appears in


Links:
 - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959811/suite/build/test/gcc-11-defconfig/details/
 - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959811/suite/build/test/gcc-11-defconfig/log

 - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959890/suite/build/test/gcc-11-defconfig/details/
 - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959890/suite/build/test/gcc-11-defconfig/log


Both build failures noticed on mainline and sparc build have been
fixed yesterday.
 - https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.4-8542-g82a2a5105589/testrun/17963192/suite/build/test/gcc-11-defconfig/history/


Following patch that got fixed
---
From 0b26eadbf200abf6c97c6d870286c73219cdac65 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 29 Jun 2023 20:41:24 -0700
Subject: sparc32: fix lock_mm_and_find_vma() conversion

The sparc32 conversion to lock_mm_and_find_vma() in commit a050ba1e7422
("mm/fault: convert remaining simple cases to lock_mm_and_find_vma()")
missed the fact that we didn't actually have a 'regs' pointer available
in the 'force_user_fault()' case.

It's there in the regular page fault path ("do_sparc_fault()"), but not
the window underflow/overflow paths.

Which is all fine - we can just pass in a NULL pointer.  The register
state is only used to avoid deadlock with kernel faults, which is not
the case for any of these register window faults.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Fixes: a050ba1e7422 ("mm/fault: convert remaining simple cases to
lock_mm_and_find_vma()")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

--
Linaro LKFT
https://lkft.linaro.org

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Greg Kroah-Hartman]

On Fri, Jun 30, 2023 at 11:00:51AM +0530, Naresh Kamboju wrote:
> On Fri, 30 Jun 2023 at 00:18, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > This is the start of the stable review cycle for the 6.4.1 release.
> > There are 28 patches in this series, all will be posted as a response
> > to this one.  If anyone has any issues with these being applied, please
> > let me know.
> >
> > Responses should be made by Sat, 01 Jul 2023 18:41:39 +0000.
> > Anything received after that time might be too late.
> >
> > The whole patch series can be found in one patch at:
> >         https://www.kernel.org/pub/linux/kernel/v6.x/stable-review/patch-6.4.1-rc1.gz
> > or in the git tree and branch at:
> >         git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-6.4.y
> > and the diffstat can be found below.
> >
> > thanks,
> >
> > greg k-h
> 
> Results from Linaro’s test farm.
> 
> Following build regression noticed on Linux stable-rc 6.4 and also noticed on
> Linux mainline master.
> 
> Regressions found on Parisc and Sparc build failed:
>  - build/gcc-11-defconfig
> 
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> 
> Parisc Build log:
> =============
> arch/parisc/mm/fault.c: In function 'do_page_fault':
> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in
> this function)
>   292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
>       |                      ^~~~
> arch/parisc/mm/fault.c:292:22: note: each undeclared identifier is
> reported only once for each function it appears in
> 
> 
> sparc Build log:
> ===========
> <stdin>:1519:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> arch/sparc/mm/fault_32.c: In function 'force_user_fault':
> arch/sparc/mm/fault_32.c:315:49: error: 'regs' undeclared (first use
> in this function)
>   315 |         vma = lock_mm_and_find_vma(mm, address, regs);
>       |                                                 ^~~~
> arch/sparc/mm/fault_32.c:315:49: note: each undeclared identifier is
> reported only once for each function it appears in
> 
> 
> Links:
>  - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959811/suite/build/test/gcc-11-defconfig/details/
>  - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959811/suite/build/test/gcc-11-defconfig/log
> 
>  - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959890/suite/build/test/gcc-11-defconfig/details/
>  - https://qa-reports.linaro.org/lkft/linux-stable-rc-linux-6.4.y/build/v6.4-29-g8e5ddb853f08/testrun/17959890/suite/build/test/gcc-11-defconfig/log
> 
> 
> Both build failures noticed on mainline and sparc build have been
> fixed yesterday.
>  - https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.4-8542-g82a2a5105589/testrun/17963192/suite/build/test/gcc-11-defconfig/history/
> 
> 
> Following patch that got fixed
> ---
> >From 0b26eadbf200abf6c97c6d870286c73219cdac65 Mon Sep 17 00:00:00 2001
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Thu, 29 Jun 2023 20:41:24 -0700
> Subject: sparc32: fix lock_mm_and_find_vma() conversion
> 
> The sparc32 conversion to lock_mm_and_find_vma() in commit a050ba1e7422
> ("mm/fault: convert remaining simple cases to lock_mm_and_find_vma()")
> missed the fact that we didn't actually have a 'regs' pointer available
> in the 'force_user_fault()' case.
> 
> It's there in the regular page fault path ("do_sparc_fault()"), but not
> the window underflow/overflow paths.
> 
> Which is all fine - we can just pass in a NULL pointer.  The register
> state is only used to avoid deadlock with kernel faults, which is not
> the case for any of these register window faults.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Fixes: a050ba1e7422 ("mm/fault: convert remaining simple cases to
> lock_mm_and_find_vma()")
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Thanks!  That saves me having to dig.  I'll go push out updates with
this in them...

greg k-h

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>
> arch/parisc/mm/fault.c: In function 'do_page_fault':
> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
>   292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))

Bah. "prev" should be "prev_vma" here.

I've pushed out the fix. Greg, apologies. It's

   ea3f8272876f parisc: fix expand_stack() conversion

and Naresh already pointed to the similarly silly sparc32 fix.

             Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Greg Kroah-Hartman]

On Fri, Jun 30, 2023 at 11:00:51AM +0530, Naresh Kamboju wrote:
> On Fri, 30 Jun 2023 at 00:18, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > This is the start of the stable review cycle for the 6.4.1 release.
> > There are 28 patches in this series, all will be posted as a response
> > to this one.  If anyone has any issues with these being applied, please
> > let me know.
> >
> > Responses should be made by Sat, 01 Jul 2023 18:41:39 +0000.
> > Anything received after that time might be too late.
> >
> > The whole patch series can be found in one patch at:
> >         https://www.kernel.org/pub/linux/kernel/v6.x/stable-review/patch-6.4.1-rc1.gz
> > or in the git tree and branch at:
> >         git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-6.4.y
> > and the diffstat can be found below.
> >
> > thanks,
> >
> > greg k-h
> 
> Results from Linaro’s test farm.
> 
> Following build regression noticed on Linux stable-rc 6.4 and also noticed on
> Linux mainline master.
> 
> Regressions found on Parisc and Sparc build failed:
>  - build/gcc-11-defconfig
> 
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> 
> Parisc Build log:
> =============
> arch/parisc/mm/fault.c: In function 'do_page_fault':
> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in
> this function)
>   292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
>       |                      ^~~~
> arch/parisc/mm/fault.c:292:22: note: each undeclared identifier is
> reported only once for each function it appears in

This is now fixed in Linus's tree with ea3f8272876f ("parisc: fix
expand_stack() conversion"), so I'll queue it up and push out
yet-another-rc...

thanks,

greg k-h

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Greg Kroah-Hartman]

On Thu, Jun 29, 2023 at 11:16:21PM -0700, Linus Torvalds wrote:
> On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
> >
> > arch/parisc/mm/fault.c: In function 'do_page_fault':
> > arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
> >   292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
> 
> Bah. "prev" should be "prev_vma" here.
> 
> I've pushed out the fix. Greg, apologies. It's
> 
>    ea3f8272876f parisc: fix expand_stack() conversion
> 
> and Naresh already pointed to the similarly silly sparc32 fix.

Ah, I saw it hit your repo before your email here, sorry about that.
Now picked up.

greg k-h

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Guenter Roeck]

On 6/29/23 23:16, Linus Torvalds wrote:
> On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>>
>> arch/parisc/mm/fault.c: In function 'do_page_fault':
>> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
>>    292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
> 
> Bah. "prev" should be "prev_vma" here.
> 
> I've pushed out the fix. Greg, apologies. It's
> 
>     ea3f8272876f parisc: fix expand_stack() conversion
> 
> and Naresh already pointed to the similarly silly sparc32 fix.
> 
>               Linus

Did you see that one (in mainline) ?

Building csky:defconfig ... failed
--------------
Error log:
arch/csky/mm/fault.c: In function 'do_page_fault':
arch/csky/mm/fault.c:240:40: error: 'address' undeclared (first use in this function); did you mean 'addr'?
   240 |         vma = lock_mm_and_find_vma(mm, address, regs);

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Guenter Roeck]

On 6/29/23 23:29, Guenter Roeck wrote:
> On 6/29/23 23:16, Linus Torvalds wrote:
>> On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>>>
>>> arch/parisc/mm/fault.c: In function 'do_page_fault':
>>> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
>>>    292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
>>
>> Bah. "prev" should be "prev_vma" here.
>>
>> I've pushed out the fix. Greg, apologies. It's
>>
>>     ea3f8272876f parisc: fix expand_stack() conversion
>>
>> and Naresh already pointed to the similarly silly sparc32 fix.
>>
>>               Linus
> 
> Did you see that one (in mainline) ?
> 
> Building csky:defconfig ... failed
> --------------
> Error log:
> arch/csky/mm/fault.c: In function 'do_page_fault':
> arch/csky/mm/fault.c:240:40: error: 'address' undeclared (first use in this function); did you mean 'addr'?
>    240 |         vma = lock_mm_and_find_vma(mm, address, regs);
> 

This is also in {6.1,6.3,6.4}-rc unless I am missing something.

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

On Thu, 29 Jun 2023 at 23:29, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Did you see that one (in mainline) ?
>
> Building csky:defconfig ... failed

Nope. Thanks. Obvious fix: 'address' is called 'addr' here.

I knew we had all these tiny little mazes that looked the same but
were just _subtly_ different, but I still ended up doing too much
cut-and-paste.

And I only ended up cross-compiling the fairly small set that I had
existing cross-build environments for. Which was less than half our
~24 different architectures.

Oh well.  We'll get them all. Eventually. Let me go fix up that csky case.

              Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

On Thu, 29 Jun 2023 at 23:33, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Oh well.  We'll get them all. Eventually. Let me go fix up that csky case.

It's commit e55e5df193d2 ("csky: fix up lock_mm_and_find_vma() conversion").

Let's hope all the problems are these kinds of silly - but obvious -
naming differences between different architectures.

Because as long as they cause build errors, they may be embarrassing,
but easy to find and notice.

I may not have cared enough about some of these architectures, and it
shows. sparc32. parisc. csky...

             Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Greg Kroah-Hartman]

On Thu, Jun 29, 2023 at 11:33:45PM -0700, Linus Torvalds wrote:
> On Thu, 29 Jun 2023 at 23:29, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Did you see that one (in mainline) ?
> >
> > Building csky:defconfig ... failed
> 
> Nope. Thanks. Obvious fix: 'address' is called 'addr' here.
> 
> I knew we had all these tiny little mazes that looked the same but
> were just _subtly_ different, but I still ended up doing too much
> cut-and-paste.
> 
> And I only ended up cross-compiling the fairly small set that I had
> existing cross-build environments for. Which was less than half our
> ~24 different architectures.
> 
> Oh well.  We'll get them all. Eventually. Let me go fix up that csky case.

Thanks, I've picked that up now as well.

greg k-h

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Helge Deller]

On 6/30/23 08:29, Greg Kroah-Hartman wrote:
> On Thu, Jun 29, 2023 at 11:16:21PM -0700, Linus Torvalds wrote:
>> On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>>>
>>> arch/parisc/mm/fault.c: In function 'do_page_fault':
>>> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
>>>    292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
>>
>> Bah. "prev" should be "prev_vma" here.
>>
>> I've pushed out the fix. Greg, apologies. It's
>>
>>     ea3f8272876f parisc: fix expand_stack() conversion
>>
>> and Naresh already pointed to the similarly silly sparc32 fix.
>
> Ah, I saw it hit your repo before your email here, sorry about that.
> Now picked up.

I've just cherry-picked ea3f8272876f on top of -rc2, built and run-tested it,
and everything is OK on parisc.

Thanks!
Helge

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Guenter Roeck]

On 6/29/23 23:47, Linus Torvalds wrote:
> On Thu, 29 Jun 2023 at 23:33, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Oh well.  We'll get them all. Eventually. Let me go fix up that csky case.
> 
> It's commit e55e5df193d2 ("csky: fix up lock_mm_and_find_vma() conversion").
> 
> Let's hope all the problems are these kinds of silly - but obvious -
> naming differences between different architectures.
> 
> Because as long as they cause build errors, they may be embarrassing,
> but easy to find and notice.
> 
> I may not have cared enough about some of these architectures, and it
> shows. sparc32. parisc. csky...
> 

There is one more, unfortunately.

Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... failed
------------
Error log:
arch/xtensa/mm/fault.c: In function ‘do_page_fault’:
arch/xtensa/mm/fault.c:133:8: error: implicit declaration of function ‘lock_mm_and_find_vma’

This affects all stable release candidates as well as mainline.
mmu builds are fine, and indeed lock_mm_and_find_vma() is only declared
for CONFIG_MMU. I don't know if this needs a dummy or some other fix
for the nommu case.

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On Fri, 30 Jun 2023 at 15:51, Guenter Roeck <linux@roeck-us.net> wrote:
>
> There is one more, unfortunately.
>
> Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... failed

Heh. I didn't even realize that anybody would ever do
lock_mm_and_find_vma() code on a nommu platform.

With nommu, handle_mm_fault() will just BUG(), so it's kind of
pointless to do any of this at all, and I didn't expect anybody to
have this page faulting path that just causes that BUG() for any
faults.

But it turns out xtensa has a notion of protection faults even for
NOMMU configs:

    config PFAULT
        bool "Handle protection faults" if EXPERT && !MMU
        default y
        help
          Handle protection faults. MMU configurations must enable it.
          noMMU configurations may disable it if used memory map never
          generates protection faults or faults are always fatal.

          If unsure, say Y.

which is why it violated my expectations so badly.

I'm not sure if that protection fault handling really ever gets quite
this far (it certainly should *not* make it to the BUG() in
handle_mm_fault()), but I think the attached patch is likely the right
thing to do.

Can you check if it fixes that xtensa case? It looks
ObviouslyCorrect(tm) to me, but considering that I clearly missed this
case existing AT ALL, it might be best to double-check.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1876 bytes --]

 include/linux/mm.h |  5 +++--
 mm/nommu.c         | 11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 39aa409e84d5..4f2c33c273eb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2323,6 +2323,9 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to);
 void truncate_pagecache_range(struct inode *inode, loff_t offset, loff_t end);
 int generic_error_remove_page(struct address_space *mapping, struct page *page);
 
+struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
+		unsigned long address, struct pt_regs *regs);
+
 #ifdef CONFIG_MMU
 extern vm_fault_t handle_mm_fault(struct vm_area_struct *vma,
 				  unsigned long address, unsigned int flags,
@@ -2334,8 +2337,6 @@ void unmap_mapping_pages(struct address_space *mapping,
 		pgoff_t start, pgoff_t nr, bool even_cows);
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows);
-struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
-		unsigned long address, struct pt_regs *regs);
 #else
 static inline vm_fault_t handle_mm_fault(struct vm_area_struct *vma,
 					 unsigned long address, unsigned int flags,
diff --git a/mm/nommu.c b/mm/nommu.c
index 37d0b03143f1..fdc392735ec6 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -630,6 +630,17 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 }
 EXPORT_SYMBOL(find_vma);
 
+/*
+ * At least xtensa ends up having protection faults even with no
+ * MMU.. No stack expansion, at least.
+ */
+struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
+			unsigned long addr, struct pt_regs *regs)
+{
+	mmap_read_lock(mm);
+	return vma_lookup(mm, addr);
+}
+
 /*
  * expand a stack to a given address
  * - not supported under NOMMU conditions

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Guenter Roeck]

On Fri, Jun 30, 2023 at 06:24:49PM -0700, Linus Torvalds wrote:
> On Fri, 30 Jun 2023 at 15:51, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > There is one more, unfortunately.
> >
> > Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... failed
> 
> Heh. I didn't even realize that anybody would ever do
> lock_mm_and_find_vma() code on a nommu platform.
> 
> With nommu, handle_mm_fault() will just BUG(), so it's kind of
> pointless to do any of this at all, and I didn't expect anybody to
> have this page faulting path that just causes that BUG() for any
> faults.
> 
> But it turns out xtensa has a notion of protection faults even for
> NOMMU configs:
> 
>     config PFAULT
>         bool "Handle protection faults" if EXPERT && !MMU
>         default y
>         help
>           Handle protection faults. MMU configurations must enable it.
>           noMMU configurations may disable it if used memory map never
>           generates protection faults or faults are always fatal.
> 
>           If unsure, say Y.
> 
> which is why it violated my expectations so badly.
> 
> I'm not sure if that protection fault handling really ever gets quite
> this far (it certainly should *not* make it to the BUG() in
> handle_mm_fault()), but I think the attached patch is likely the right
> thing to do.
> 
> Can you check if it fixes that xtensa case? It looks
> ObviouslyCorrect(tm) to me, but considering that I clearly missed this
> case existing AT ALL, it might be best to double-check.
> 
>                Linus

Yes, the patch below fixes the problem.

Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... running ......... passed

Thanks,
Guenter

>  include/linux/mm.h |  5 +++--
>  mm/nommu.c         | 11 +++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 39aa409e84d5..4f2c33c273eb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2323,6 +2323,9 @@ void pagecache_isize_extended(struct inode *inode, loff_t from, loff_t to);
>  void truncate_pagecache_range(struct inode *inode, loff_t offset, loff_t end);
>  int generic_error_remove_page(struct address_space *mapping, struct page *page);
>  
> +struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
> +		unsigned long address, struct pt_regs *regs);
> +
>  #ifdef CONFIG_MMU
>  extern vm_fault_t handle_mm_fault(struct vm_area_struct *vma,
>  				  unsigned long address, unsigned int flags,
> @@ -2334,8 +2337,6 @@ void unmap_mapping_pages(struct address_space *mapping,
>  		pgoff_t start, pgoff_t nr, bool even_cows);
>  void unmap_mapping_range(struct address_space *mapping,
>  		loff_t const holebegin, loff_t const holelen, int even_cows);
> -struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
> -		unsigned long address, struct pt_regs *regs);
>  #else
>  static inline vm_fault_t handle_mm_fault(struct vm_area_struct *vma,
>  					 unsigned long address, unsigned int flags,
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 37d0b03143f1..fdc392735ec6 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -630,6 +630,17 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  }
>  EXPORT_SYMBOL(find_vma);
>  
> +/*
> + * At least xtensa ends up having protection faults even with no
> + * MMU.. No stack expansion, at least.
> + */
> +struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
> +			unsigned long addr, struct pt_regs *regs)
> +{
> +	mmap_read_lock(mm);
> +	return vma_lookup(mm, addr);
> +}
> +
>  /*
>   * expand a stack to a given address
>   * - not supported under NOMMU conditions

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

On Fri, 30 Jun 2023 at 19:50, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Yes, the patch below fixes the problem.
>
> Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... running ......... passed

Thanks. Committed as

  d85a143b69ab ("xtensa: fix NOMMU build with lock_mm_and_find_vma()
conversion")

and pushed out.

                    Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Greg Kroah-Hartman]

On Fri, Jun 30, 2023 at 09:22:45PM -0700, Linus Torvalds wrote:
> On Fri, 30 Jun 2023 at 19:50, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Yes, the patch below fixes the problem.
> >
> > Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... running ......... passed
> 
> Thanks. Committed as
> 
>   d85a143b69ab ("xtensa: fix NOMMU build with lock_mm_and_find_vma()
> conversion")
> 
> and pushed out.

Thanks, now queued up.

greg k-h

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Max Filippov]

Hi Linus,

On Fri, Jun 30, 2023 at 9:23 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, 30 Jun 2023 at 19:50, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Yes, the patch below fixes the problem.
> >
> > Building xtensa:de212:kc705-nommu:nommu_kc705_defconfig ... running ......... passed
>
> Thanks. Committed as
>
>   d85a143b69ab ("xtensa: fix NOMMU build with lock_mm_and_find_vma()
> conversion")
>
> and pushed out.

Thanks for the build fix. Unfortunately despite being obviously correct
it doesn't release the mm lock in case VMA is not found, so it results
in a runtime hang. I've posted a fix for that.

-- 
Thanks.
-- Max

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review

[Linus Torvalds]

On Sat, 1 Jul 2023 at 03:32, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> Thanks for the build fix. Unfortunately despite being obviously correct
> it doesn't release the mm lock in case VMA is not found, so it results
> in a runtime hang. I've posted a fix for that.

Heh. I woke up this morning to that feeling of "Duh!" about this, and
find you already had fixed it. Patch applied.

            Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Helge Deller]

Hi Linus,

On 6/30/23 08:56, Helge Deller wrote:
> On 6/30/23 08:29, Greg Kroah-Hartman wrote:
>> On Thu, Jun 29, 2023 at 11:16:21PM -0700, Linus Torvalds wrote:
>>> On Thu, 29 Jun 2023 at 22:31, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>>>>
>>>> arch/parisc/mm/fault.c: In function 'do_page_fault':
>>>> arch/parisc/mm/fault.c:292:22: error: 'prev' undeclared (first use in this function)
>>>>    292 |                 if (!prev || !(prev->vm_flags & VM_GROWSUP))
>>>
>>> Bah. "prev" should be "prev_vma" here.
>>>
>>> I've pushed out the fix. Greg, apologies. It's
>>>
>>>     ea3f8272876f parisc: fix expand_stack() conversion
>>>
>>> and Naresh already pointed to the similarly silly sparc32 fix.
>>
>> Ah, I saw it hit your repo before your email here, sorry about that.
>> Now picked up.
>
> I've just cherry-picked ea3f8272876f on top of -rc2, built and run-tested it,
> and everything is OK on parisc.

Actually, your changes seems to trigger...:

root@debian:~# /usr/bin/ls /usr/bin/*
-bash: /usr/bin/ls: Argument list too long

or with a long gcc argument list:
gcc: fatal error: cannot execute '/usr/lib/gcc/hppa-linux-gnu/12/cc1': execv: Argument list too long

I'm trying to understand what's missing, but maybe you have some idea?

Helge

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

On Sun, 2 Jul 2023 at 14:33, Helge Deller <deller@gmx.de> wrote:
>
> Actually, your changes seems to trigger...:
>
> root@debian:~# /usr/bin/ls /usr/bin/*
> -bash: /usr/bin/ls: Argument list too long

So this only happens with _fairly_ long argument lists, right? Maybe
your config has a 64kB page size, and normal programs never expand
beyond a single page?

I bet it is because of f313c51d26aa ("execve: expand new process stack
manually ahead of time"), but I don't see exactly why.

But pa-risc is the only architecture with CONFIG_STACK_GROWSUP, and
while I really thought that commit should do the exact same thing as
the old

  #ifdef CONFIG_STACK_GROWSUP

special case, I must clearly have been wrong.

Would you mind just verifying that yes, that commit on mainline is
broken for you, and the previous one works?

               Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On Sun, 2 Jul 2023 at 15:45, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Would you mind just verifying that yes, that commit on mainline is
> broken for you, and the previous one works?

Also, while I looked at it again, and still didn't understand why
parisc would be different here, I *did* realize that because parisc
has a stack that grows up, the debug warning I added for GUP won't
trigger.

So if I got that execve() logic wrong for STACK_GROWSUP (which I
clearly must have), then exactly because it's grows-up, a GUP failure
wouldn't warn about not expanding the stack.

IOW, would you mind applying something like this on top of the current
kernel, and let me know if it warns?

.. and here I thought ia64 would be the pain-point. Silly me.

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 773 bytes --]

 mm/gup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index ef29641671c7..66520194006b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1168,11 +1168,15 @@ static long __get_user_pages(struct mm_struct *mm,
 
 		/* first iteration or cross vma bound */
 		if (!vma || start >= vma->vm_end) {
-			vma = find_vma(mm, start);
+			struct vm_area_struct *prev = NULL;
+			vma = find_vma_prev(mm, start, &prev);
 			if (vma && (start < vma->vm_start)) {
 				WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
 				vma = NULL;
 			}
+			if (!vma && prev && start >= prev->vm_end)
+				WARN_ON_ONCE(prev->vm_flags & VM_GROWSUP);
+
 			if (!vma && in_gate_area(mm, start)) {
 				ret = get_gate_page(mm, start & PAGE_MASK,
 						gup_flags, &vma,

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/2/23 16:30, Linus Torvalds wrote:
> On Sun, 2 Jul 2023 at 15:45, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Would you mind just verifying that yes, that commit on mainline is
>> broken for you, and the previous one works?
> 
> Also, while I looked at it again, and still didn't understand why
> parisc would be different here, I *did* realize that because parisc
> has a stack that grows up, the debug warning I added for GUP won't
> trigger.
> 
> So if I got that execve() logic wrong for STACK_GROWSUP (which I
> clearly must have), then exactly because it's grows-up, a GUP failure
> wouldn't warn about not expanding the stack.
> 
> IOW, would you mind applying something like this on top of the current
> kernel, and let me know if it warns?
> 

I can reproduce the problem in qemu. However, I do not see a warning
after applying your patch.

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On Sun, 2 Jul 2023 at 20:23, Guenter Roeck <linux@roeck-us.net> wrote:
>
> I can reproduce the problem in qemu. However, I do not see a warning
> after applying your patch.

Funky, funky.

I'm assuming it's the

                                page = get_arg_page(bprm, pos, 1);
                                if (!page) {
                                        ret = -E2BIG;
                                        goto out;
                                }

in copy_strings() that causes this. Or possibly, the version in
copy_string_kernel().

Does *this* get that "pr_warn()" printout (and a stack trace once,
just for good measure)?

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 773 bytes --]

 mm/gup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index ef29641671c7..66520194006b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1168,11 +1168,15 @@ static long __get_user_pages(struct mm_struct *mm,
 
 		/* first iteration or cross vma bound */
 		if (!vma || start >= vma->vm_end) {
-			vma = find_vma(mm, start);
+			struct vm_area_struct *prev = NULL;
+			vma = find_vma_prev(mm, start, &prev);
 			if (vma && (start < vma->vm_start)) {
 				WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
 				vma = NULL;
 			}
+			if (!vma && prev && start >= prev->vm_end)
+				WARN_ON_ONCE(prev->vm_flags & VM_GROWSUP);
+
 			if (!vma && in_gate_area(mm, start)) {
 				ret = get_gate_page(mm, start & PAGE_MASK,
 						gup_flags, &vma,

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/2/23 21:22, Linus Torvalds wrote:
> On Sun, 2 Jul 2023 at 20:23, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> I can reproduce the problem in qemu. However, I do not see a warning
>> after applying your patch.
> 
> Funky, funky.
> 
> I'm assuming it's the
> 
>                                  page = get_arg_page(bprm, pos, 1);
>                                  if (!page) {
>                                          ret = -E2BIG;
>                                          goto out;
>                                  }
> 
> in copy_strings() that causes this. Or possibly, the version in
> copy_string_kernel().
> 
> Does *this* get that "pr_warn()" printout (and a stack trace once,
> just for good measure)?
> 

Sorry, you lost me. Isn't that the same patch as before ? Or
is it just time for me to go to bed ?

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

On Sun, 2 Jul 2023 at 21:46, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Sorry, you lost me. Isn't that the same patch as before ? Or
> is it just time for me to go to bed ?

No, I think it's time for *me* to go to bed.

Let's get the right diff this time.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 932 bytes --]

 fs/exec.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index 1a827d55ba94..50462ee6085c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -212,6 +212,9 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		ret = expand_downwards(vma, pos);
 		if (unlikely(ret < 0)) {
 			mmap_write_unlock(mm);
+			pr_warn("stack expand failed: %lx-%lx (%lx)\n",
+				vma->vm_start, vma->vm_end, pos);
+			WARN_ON_ONCE(1);
 			return NULL;
 		}
 		mmap_write_downgrade(mm);
@@ -226,8 +229,12 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 			write ? FOLL_WRITE : 0,
 			&page, NULL);
 	mmap_read_unlock(mm);
-	if (ret <= 0)
+	if (ret <= 0) {
+		pr_warn("get_user_pages_remote failed: %lx-%lx (%lx)\n",
+			vma->vm_start, vma->vm_end, pos);
+		WARN_ON_ONCE(1);
 		return NULL;
+	}
 
 	if (write)
 		acct_arg_size(bprm, vma_pages(vma));

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/2/23 21:49, Linus Torvalds wrote:
> On Sun, 2 Jul 2023 at 21:46, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Sorry, you lost me. Isn't that the same patch as before ? Or
>> is it just time for me to go to bed ?
> 
> No, I think it's time for *me* to go to bed.
> 
> Let's get the right diff this time.
> 

Here you are:

[   31.188688] stack expand failed: ffeff000-fff00000 (ffefeff2)
[   31.189131] ------------[ cut here ]------------
[   31.189259] WARNING: CPU: 0 PID: 472 at fs/exec.c:217 get_arg_page+0x1e8/0x1f4
[   31.189827] Modules linked in:
[   31.190083] CPU: 0 PID: 472 Comm: sh Tainted: G                 N 6.4.0-32bit+ #1
[   31.190213] Hardware name: 9000/778/B160L
[   31.190347]
[   31.190407]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
[   31.190496] PSW: 00000000000001001011111100001111 Tainted: G                 N
[   31.190625] r00-03  0004bf0f 11026240 1034a3ec 12bb41c0
[   31.190741] r04-07  127ec400 00000001 12b725a4 12b72530
[   31.190821] r08-11  129e6708 ffefeff2 2ff9d000 ffeff000
[   31.190895] r12-15  127ec400 10e463f0 10e34348 12a4d1a0
[   31.190962] r16-19  00000002 00001000 ffefe000 12a4d1a0
[   31.191033] r20-23  0000000f 00001a46 013ae000 12bb4498
[   31.191103] r24-27  11542330 00000000 115430a0 10ed98d8
[   31.191173] r28-31  00000031 00000310 12bb4240 0000000f
[   31.191251] sr00-03  00000000 00000000 00000000 000000a0
[   31.191332] sr04-07  00000000 00000000 00000000 00000000
[   31.191407]
[   31.191443] IASQ: 00000000 00000000 IAOQ: 1034a3ec 1034a3f0
[   31.191522]  IIR: 03ffe01f    ISR: 00000000  IOR: 1065d424
[   31.191593]  CPU:        0   CR30: 12a4d1a0 CR31: 00000000
[   31.192675]  ORIG_R28: 12a4d1a0
[   31.192770]  IAOQ[0]: get_arg_page+0x1e8/0x1f4
[   31.192851]  IAOQ[1]: get_arg_page+0x1ec/0x1f4
[   31.192922]  RP(r2): get_arg_page+0x1e8/0x1f4
[   31.193007] Backtrace:
[   31.193085]  [<1034a9cc>] copy_strings+0x148/0x3d8
[   31.193214]  [<1034ad94>] do_execveat_common+0x138/0x21c
[   31.193302]  [<1034bcc4>] sys_execve+0x3c/0x54
[   31.193400]  [<101af1b4>] syscall_exit+0x0/0x10
[   31.193562]
[   31.193698] ---[ end trace 0000000000000000 ]---
[   31.200551] stack expand failed: ffeff000-fff00000 (ffefefee)
/bin/sh: ls: Argument list too long

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

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.

                     Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 948 bytes --]

 include/linux/mm.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 74f1be743ba2..2dd73e4f3d8e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -377,7 +377,7 @@ extern unsigned int kobjsize(const void *objp);
 #endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */
 
 /* Bits set in the VMA until the stack is in its final location */
-#define VM_STACK_INCOMPLETE_SETUP	(VM_RAND_READ | VM_SEQ_READ)
+#define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ | VM_STACK_EARLY)
 
 #define TASK_EXEC ((current->personality & READ_IMPLIES_EXEC) ? VM_EXEC : 0)
 
@@ -399,8 +399,10 @@ extern unsigned int kobjsize(const void *objp);
 
 #ifdef CONFIG_STACK_GROWSUP
 #define VM_STACK	VM_GROWSUP
+#define VM_STACK_EARLY	VM_GROWSDOWN
 #else
 #define VM_STACK	VM_GROWSDOWN
+#define VM_STACK_EARLY	0
 #endif
 
 #define VM_STACK_FLAGS	(VM_STACK | VM_STACK_DEFAULT_FLAGS | VM_ACCOUNT)

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[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

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/2/23 23: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.
> 

Yes, it does. I'll run a complete qemu test with it applied to be sure
there is no impact on other architectures (yes, I know, that should not
be the case, but better safe than sorry). I'll even apply
https://lore.kernel.org/all/20230609075528.9390-12-bhe@redhat.com/raw
to be able to test sh4.

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/3/23 05:59, Guenter Roeck wrote:
> On 7/2/23 23: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.
>>
> 
> Yes, it does. I'll run a complete qemu test with it applied to be sure
> there is no impact on other architectures (yes, I know, that should not
> be the case, but better safe than sorry). I'll even apply
> https://lore.kernel.org/all/20230609075528.9390-12-bhe@redhat.com/raw
> to be able to test sh4.
> 

Meh, should have figured. That fixes one problem with sh4 builds
and creates another. Should have figured.

Guenter

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

On Mon, 3 Jul 2023 at 00:08, Helge Deller <deller@gmx.de> wrote:
>
> Great, that patch fixes it!

Yeah, I was pretty sure this was it, but it's good to have it
confirmed. Committed.

> 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.

No, that wouldn't work for the simple reason that the special bits in
VM_STACK_INCOMPLETE_SETUP are always cleared after the stack setup is
done.

So if we added VM_GROWSDOWN to those early bits in general, the bit
would then be cleared even when that wasn't the intent.

Yes, yes, we could change the VM_STACK_INCOMPLETE_SETUP logic to only
clear some of the bits in the end, but the end result would be
practically the same: we'd still have to do different things for
grows-up vs grows-down cases, so the difference might as well be here
in the VM_STACK_EARLY bit.

                 Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Guenter Roeck]

On 7/3/23 09:49, Linus Torvalds wrote:
> On Mon, 3 Jul 2023 at 00:08, Helge Deller <deller@gmx.de> wrote:
>>
>> Great, that patch fixes it!
> 
> Yeah, I was pretty sure this was it, but it's good to have it
> confirmed. Committed.
> 

FWIW, my qemu boot tests didn't find any problems with other architectures.

Guenter

>> 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.
> 
> No, that wouldn't work for the simple reason that the special bits in
> VM_STACK_INCOMPLETE_SETUP are always cleared after the stack setup is
> done.
> 
> So if we added VM_GROWSDOWN to those early bits in general, the bit
> would then be cleared even when that wasn't the intent.
> 
> Yes, yes, we could change the VM_STACK_INCOMPLETE_SETUP logic to only
> clear some of the bits in the end, but the end result would be
> practically the same: we'd still have to do different things for
> grows-up vs grows-down cases, so the difference might as well be here
> in the VM_STACK_EARLY bit.
> 
>                   Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Linus Torvalds]

On Mon, 3 Jul 2023 at 10:19, Guenter Roeck <linux@roeck-us.net> wrote:
>
> FWIW, my qemu boot tests didn't find any problems with other architectures.

Thanks. This whole "let's get the stack expansion locking right"
wasn't exactly buttery smooth, but given all our crazy architectures
it was not entirely unexpected.

Let's hope it really is all done now,

            Linus

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Helge Deller]

On 7/3/23 18:49, Linus Torvalds wrote:
> On Mon, 3 Jul 2023 at 00:08, Helge Deller <deller@gmx.de> wrote:
>>
>> Great, that patch fixes it!
>
> Yeah, I was pretty sure this was it, but it's good to have it
> confirmed. Committed.

Thank you!

Nice to see that Greg picked up the patch for stable that fast as well!

>> 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.
>
> No, that wouldn't work for the simple reason that the special bits in
> VM_STACK_INCOMPLETE_SETUP are always cleared after the stack setup is
> done.
>
> So if we added VM_GROWSDOWN to those early bits in general, the bit
> would then be cleared even when that wasn't the intent.
>
> Yes, yes, we could change the VM_STACK_INCOMPLETE_SETUP logic to only
> clear some of the bits in the end, but the end result would be
> practically the same: we'd still have to do different things for
> grows-up vs grows-down cases, so the difference might as well be here
> in the VM_STACK_EARLY bit.

Ok, thanks for explainig!

Helge

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Sam James]

Helge Deller <deller@gmx.de> writes:

> On 7/3/23 18:49, Linus Torvalds wrote:
>> On Mon, 3 Jul 2023 at 00:08, Helge Deller <deller@gmx.de> wrote:
>>>
>>> Great, that patch fixes it!
>>
>> Yeah, I was pretty sure this was it, but it's good to have it
>> confirmed. Committed.
>
> Thank you!
>
> Nice to see that Greg picked up the patch for stable that fast as well!

Sorry, where? I was just about to check if it was marked for backporting
but I can't see it in Greg's trees yet.

We need it fo 6.1, 6.3, 6.4.

(Apologies if I'm missing it somewhere obvious.)

Re: [PATCH 6.4 00/28] 6.4.1-rc1 review - hppa argument list too long

[Sam James]

Sam James <sam@gentoo.org> writes:

> Helge Deller <deller@gmx.de> writes:
>
>> On 7/3/23 18:49, Linus Torvalds wrote:
>>> On Mon, 3 Jul 2023 at 00:08, Helge Deller <deller@gmx.de> wrote:
>>>>
>>>> Great, that patch fixes it!
>>>
>>> Yeah, I was pretty sure this was it, but it's good to have it
>>> confirmed. Committed.
>>
>> Thank you!
>>
>> Nice to see that Greg picked up the patch for stable that fast as well!
>
> Sorry, where? I was just about to check if it was marked for backporting
> but I can't see it in Greg's trees yet.
>
> We need it fo 6.1, 6.3, 6.4.
>
> (Apologies if I'm missing it somewhere obvious.)

.. and I was. I see it now, sorry!