Messages in this thread Patch in this message | | | Date | Mon, 12 Feb 2024 16:50:49 +0000 | From | Dave Martin <> | Subject | Re: [PATCH] arm64/sve: Lower the maximum allocation for the SVE ptrace regset |
| |
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
| |