Re: Bisected stability regression in 6.6

[Helge Deller <deller@gmx.de>]

On 11/12/23 02:22, Dr. David Alan Gilbert wrote:
> * matoro (matoro_mailinglist_kernel@matoro.tk) wrote:
>> On 2023-11-11 16:27, Sam James wrote:
>>> Helge Deller <deller@gmx.de> writes:
>>>
>>>> On 11/11/23 07:31, matoro wrote:
>>>>> Hi Helge, I have bisected a regression in 6.6 which is causing
>>>>> userspace segfaults at a significantly increased rate in kernel 6.6.
>>>>> There seems to be a pathological case triggered by the ninja build
>>>>> tool.  The test case I have been using is cmake with ninja backend to
>>>>> attempt to build the nghttp2 package.  In 6.6, this segfaults, not at
>>>>> the same location every time, but with enough reliability that I was
>>>>> able to use it as a bisection regression case, including immediately
>>>>> after a reboot.  In the kernel log, these show up as "trap #15: Data
>>>>> TLB miss fault" messages.  Now these messages can and do show up in
>>>>> 6.5 causing segfaults, but never immediately after a reboot and
>>>>> infrequently enough that the system is stable.  With kernel 6.6 I am
>>>>> completely unable to build nghttp2 under any circumstances.
>>>>>
>>>>> I have bisected this down to the following commit:
>>>>>
>>>>> $ git bisect good
>>>>> 3033cd4307681c60db6d08f398a64484b36e0b0f is the first bad commit
>>>>> commit 3033cd4307681c60db6d08f398a64484b36e0b0f
>>>>> Author: Helge Deller <deller@gmx.de>
>>>>> Date:   Sat Aug 19 00:53:28 2023 +0200
>>>>>
>>>>>       parisc: Use generic mmap top-down layout and brk randomization
>>>>>
>>>>>       parisc uses a top-down layout by default that exactly fits
>>>>> the generic
>>>>>       functions, so get rid of arch specific code and use the
>>>>> generic version
>>>>>       by selecting ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.
>>>>>
>>>>>       Note that on parisc the stack always grows up and a "unlimited stack"
>>>>>       simply means that the value as defined in
>>>>> CONFIG_STACK_MAX_DEFAULT_SIZE_MB
>>>>>       should be used. So RLIM_INFINITY is not an indicator to use
>>>>> the legacy
>>>>>       memory layout.
>>>>>
>>>>>       Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>>>    arch/parisc/Kconfig             | 17 +++++++++++++
>>>>>    arch/parisc/kernel/process.c    | 14 -----------
>>>>>    arch/parisc/kernel/sys_parisc.c | 54
>>>>> +----------------------------------------
>>>>>    mm/util.c                       |  5 +++-
>>>>>    4 files changed, 22 insertions(+), 68 deletions(-)
>>>>
>>>> Thanks for your report!
>>>> I think it's quite unlikely that this patch introduces such a bad
>>>> regression.
>>>> I'd suspect some other bad commmit, but I'll try to reproduce.
>>>
>>> matoro, does a revert apply cleanly? Does it help?
>>
>> Yes, I just tested this and it cleanly reverts on linux-6.6.y and the revert
>> does fix the issue.
>
> Helge:
>    In that patch is:
>
> diff --git a/mm/util.c b/mm/util.c
> index dd12b9531ac4c..8810206444977 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -396,7 +396,10 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
>          if (current->personality & ADDR_COMPAT_LAYOUT)
>                  return 1;
>
> -       if (rlim_stack->rlim_cur == RLIM_INFINITY)
> +       /* On parisc the stack always grows up - so a unlimited stack should
> +        * not be an indicator to use the legacy memory layout. */
> +       if (rlim_stack->rlim_cur == RLIM_INFINITY &&
> +               !IS_ENABLED(CONFIG_STACK_GROWSUP))
>                  return 1;
>
>          return sysctl_legacy_va_layout;
>
> is that:
>     '!IS_ENABLED(CONFIG_STACK_GROWSUP))'
>
>   the right way around?
>
> That feels inverted to me;  non-parisc don't have that config
> set, so !IS_ENABLED... is true,  so they return 1 instead of checking
> the flag?

Right. For non-parisc the behaviour didn't change with my patch, and this
is intended. If rlim_stack->rlim_cur == RLIM_INFINITY, non-parisc return 1 as before.

Note that matoro reported a regression specifically on the parisc platform.

This change:
-       if (rlim_stack->rlim_cur == RLIM_INFINITY)
+       if (rlim_stack->rlim_cur == RLIM_INFINITY &&
+               !IS_ENABLED(CONFIG_STACK_GROWSUP))
just changes the behaviour on parisc.
On parisc rlim_stack->rlim_cur == RLIM_INFINITY" is always true, unless the user
changed the stack limit manually. If unchanged, mmap_is_legacy() should return
sysctl_legacy_va_layout, otherwise 1.

So, I think that part of the patch is OK.

Helge