Re: [PATCH] arm64/sve: Lower the maximum allocation for the SVE ptrace regset

From: Dave Martin <Dave.Martin@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Doug Anderson <dianders@chromium.org>
Subject: Re: [PATCH] arm64/sve: Lower the maximum allocation for the SVE ptrace regset
Date: Mon, 12 Feb 2024 16:50:49 +0000	[thread overview]
Message-ID: <ZcpMabqH+VZv6RCZ@e133344.arm.com> (raw)
In-Reply-To: <20240203-arm64-sve-ptrace-regset-size-v1-1-2c3ba1386b9e@kernel.org>

On Sat, Feb 03, 2024 at 12:16:49PM +0000, Mark Brown wrote:
> Doug Anderson observed that ChromeOS crashes are being reported which
> include failing allocations of order 7 during core dumps due to ptrace
> allocating storage for regsets:
> 
>   chrome: page allocation failure: order:7,
>           mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO),
>           nodemask=(null),cpuset=urgent,mems_allowed=0
>    ...
>   regset_get_alloc+0x1c/0x28
>   elf_core_dump+0x3d8/0xd8c
>   do_coredump+0xeb8/0x1378
> 
> with further investigation showing that this is:
> 
>    [   66.957385] DOUG: Allocating 279584 bytes
> 
> which is the maximum size of the SVE regset. As Doug observes it is not
> entirely surprising that such a large allocation of contiguous memory might
> fail on a long running system.
> 
> The SVE regset is currently sized to hold SVE registers with a VQ of
> SVE_VQ_MAX which is 512, substantially more than the architectural maximum
> of 16 which we might see even in a system emulating the limits of the
> architecture. Since we don't expose the size we tell the regset core
> externally let's define ARCH_SVE_VQ_MAX with the actual architectural
> maximum and use that for the regset, we'll still overallocate most of the
> time but much less so which will be helpful even if the core is fixed to
> not require contiguous allocations.
> 
> We could also teach the ptrace core about runtime discoverable regset sizes
> but that would be a more invasive change and this is being observed in
> practical systems.
> 
> Reported-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> We should probably also use the actual architectural limit for the
> bitmasks we use in the VL enumeration code, though that's both a little
> bit more involved and less immediately a problem.
> ---
>  arch/arm64/include/asm/fpsimd.h | 10 +++++-----
>  arch/arm64/kernel/ptrace.c      |  3 ++-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 50e5f25d3024..cf5f31181bc8 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -62,12 +62,12 @@ static inline void cpacr_restore(unsigned long cpacr)
>   * When we defined the maximum SVE vector length we defined the ABI so
>   * that the maximum vector length included all the reserved for future
>   * expansion bits in ZCR rather than those just currently defined by
> - * the architecture. While SME follows a similar pattern the fact that
> - * it includes a square matrix means that any allocations that attempt
> - * to cover the maximum potential vector length (such as happen with
> - * the regset used for ptrace) end up being extremely large. Define
> - * the much lower actual limit for use in such situations.
> + * the architecture.  Using this length to allocate worst size buffers
> + * results in excessively large allocations, and this effect is even
> + * more pronounced for SME due to ZA.  Define more suitable VLs for
> + * these situations.
>   */
> +#define ARCH_SVE_VQ_MAX 16
>  #define SME_VQ_MAX	16
>  
>  struct task_struct;
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index dc6cf0e37194..e3bef38fc2e2 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1500,7 +1500,8 @@ static const struct user_regset aarch64_regsets[] = {
>  #ifdef CONFIG_ARM64_SVE
>  	[REGSET_SVE] = { /* Scalable Vector Extension */
>  		.core_note_type = NT_ARM_SVE,
> -		.n = DIV_ROUND_UP(SVE_PT_SIZE(SVE_VQ_MAX, SVE_PT_REGS_SVE),
> +		.n = DIV_ROUND_UP(SVE_PT_SIZE(ARCH_SVE_VQ_MAX,
> +					      SVE_PT_REGS_SVE),
>  				  SVE_VQ_BYTES),

Do we need an actual check somewhere that we don't bust this limit?

Since ZCR_ELx_LEN_MASK was changed from 0x1ff to 0xf, it looks like the
kernel itself will not generate an overlarge VL, although it feels a bit
like this guarantee arrives by accident.
Could ARCH_SVE_VQ_MAX be based on ZCR_ELx_LEN_MASK instead?

Userspace could specify vl > sve_vl_from_vq(ARCH_SVE_VQ_MAX) in
PTRACE_SETREGSET; I'm not sure exactly what happens there.

(The original 0x1ff value of ZCR_ELx_LEN_MASK was based on more than
just hope, but it does seem appropriate to restrict it now so that it
matches the formal architecture, as per commit f171f9e4097d
("arm64/fp: Make SVE and SME length register definition match
architecture") )

[...]

Cheers
---Dave