Re: Bisected stability regression in 6.6

[matoro <matoro_mailinglist_kernel@matoro.tk>]

On 2023-11-12 15:22, Helge Deller wrote:
> * matoro <matoro_mailinglist_kernel@matoro.tk>:
>> 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 was wrong.
> Indeed, by switching to the generic implementation with this patch
> the calculation of mmap_base is wrong for parisc (because parisc
> is the only architecture left where the stack grows upwards).
> 
> Could you please test the patch below. It did fixed the crashes
> when building nghttp2 for me.
> 
> Helge
> 
> ---
> 
> From: Helge Deller <deller@gmx.de>
> Subject: [PATCH] parisc: Adjust ARCH_MMAP_RND_BITS* to previous values
> 
> Matoro reported various userspace crashes in kernel 6.6 and bisected it to
> commit 3033cd430768 ("parisc: Use generic mmap top-down layout and brk
> randomization").
> 
> The problem is, that mmap_base is calculated wrongly for the
> stack-grows-upwards case (as on parisc). On parisc, mmap_base is simply just
> below the stack start.
> 
> Reported-by: matoro <matoro_mailinglist_kernel@matoro.tk>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Fixes: 3033cd430768 ("parisc: Use generic mmap top-down layout and brk 
> randomization")
> Cc:  <stable@vger.kernel.org> # v6.6+
> 
> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
> index a15ab147af2e..68cbe666510a 100644
> --- a/arch/parisc/Kconfig
> +++ b/arch/parisc/Kconfig
> @@ -138,11 +138,11 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>  	default 8
> 
>  config ARCH_MMAP_RND_BITS_MAX
> -	default 24 if 64BIT
> -	default 17
> +	default 18 if 64BIT
> +	default 13
> 
>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
> -	default 17
> +	default 13
> 
>  # unless you want to implement ACPI on PA-RISC ... ;-)
>  config PM
> diff --git a/arch/parisc/include/asm/elf.h b/arch/parisc/include/asm/elf.h
> index 140eaa97bf21..2d73d3c3cd37 100644
> --- a/arch/parisc/include/asm/elf.h
> +++ b/arch/parisc/include/asm/elf.h
> @@ -349,15 +349,7 @@ struct pt_regs;	/* forward declaration... */
> 
>  #define ELF_HWCAP	0
> 
> -/* Masks for stack and mmap randomization */
> -#define BRK_RND_MASK	(is_32bit_task() ? 0x07ffUL : 0x3ffffUL)
> -#define MMAP_RND_MASK	(is_32bit_task() ? 0x1fffUL : 0x3ffffUL)
> -#define STACK_RND_MASK	MMAP_RND_MASK
> -
> -struct mm_struct;
> -extern unsigned long arch_randomize_brk(struct mm_struct *);
> -#define arch_randomize_brk arch_randomize_brk
> -
> +#define STACK_RND_MASK	0x7ff	/* 8MB of VA */
> 
>  #define ARCH_HAS_SETUP_ADDITIONAL_PAGES 1
>  struct linux_binprm;
> diff --git a/arch/parisc/include/asm/processor.h 
> b/arch/parisc/include/asm/processor.h
> index ff6cbdb6903b..ece4b3046515 100644
> --- a/arch/parisc/include/asm/processor.h
> +++ b/arch/parisc/include/asm/processor.h
> @@ -47,6 +47,8 @@
> 
>  #ifndef __ASSEMBLY__
> 
> +struct rlimit;
> +unsigned long mmap_upper_limit(struct rlimit *rlim_stack);
>  unsigned long calc_max_stack_size(unsigned long stack_max);
> 
>  /*
> diff --git a/arch/parisc/kernel/sys_parisc.c 
> b/arch/parisc/kernel/sys_parisc.c
> index ab896eff7a1d..98af719d5f85 100644
> --- a/arch/parisc/kernel/sys_parisc.c
> +++ b/arch/parisc/kernel/sys_parisc.c
> @@ -77,7 +77,7 @@ unsigned long calc_max_stack_size(unsigned long stack_max)
>   * indicating that "current" should be used instead of a passed-in
>   * value from the exec bprm as done with arch_pick_mmap_layout().
>   */
> -static unsigned long mmap_upper_limit(struct rlimit *rlim_stack)
> +unsigned long mmap_upper_limit(struct rlimit *rlim_stack)
>  {
>  	unsigned long stack_base;
> 
> diff --git a/mm/util.c b/mm/util.c
> index 8cbbfd3a3d59..0b7e715a71f2 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -414,6 +414,15 @@ static int mmap_is_legacy(struct rlimit *rlim_stack)
> 
>  static unsigned long mmap_base(unsigned long rnd, struct rlimit 
> *rlim_stack)
>  {
> +#ifdef CONFIG_STACK_GROWSUP
> +	/*
> +	 * For an upwards growing stack the calculation is much simpler.
> +	 * Memory for the maximum stack size is reserved at the top of the
> +	 * task. mmap_base starts directly below the stack and grows
> +	 * downwards.
> +	 */
> +	return PAGE_ALIGN(mmap_upper_limit(rlim_stack) - rnd);
> +#else
>  	unsigned long gap = rlim_stack->rlim_cur;
>  	unsigned long pad = stack_guard_gap;
> 
> @@ -431,6 +440,7 @@ static unsigned long mmap_base(unsigned long rnd, struct 
> rlimit *rlim_stack)
>  		gap = MAX_GAP;
> 
>  	return PAGE_ALIGN(STACK_TOP - gap - rnd);
> +#endif
>  }
> 
>  void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)

Works here!  Thanks Helge!!