lkml.org 
[lkml]   [2023]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Bisected stability regression in 6.6
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!!

\
 
 \ /
  Last update: 2023-11-20 13:57    [W:0.047 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site