[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <43096cfbf1d87eff4cbd81c8a13a66f0@matoro.tk>
Date: Sun, 12 Nov 2023 18:37:24 -0500
From: matoro <matoro_mailinglist_kernel@...oro.tk>
To: Helge Deller <deller@....de>
Cc: Sam James <sam@...too.org>, linux-parisc@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Bisected stability regression in 6.6
On 2023-11-12 15:22, Helge Deller wrote:
> * matoro <matoro_mailinglist_kernel@...oro.tk>:
>> On 2023-11-11 16:27, Sam James wrote:
>> > Helge Deller <deller@....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@....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@....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@....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@...oro.tk>
> Signed-off-by: Helge Deller <deller@....de>
> Fixes: 3033cd430768 ("parisc: Use generic mmap top-down layout and brk
> randomization")
> Cc: <stable@...r.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!!
Powered by blists - more mailing lists