Date: Fri, 2 Feb 2024 18:07:54 +0000
From: Dave Martin <Dave.Martin@....com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Doug Anderson <dianders@...omium.org>,
	Christian Brauner <brauner@...nel.org>,
	Eric Biederman <ebiederm@...ssion.com>, Jan Kara <jack@...e.cz>,
	Kees Cook <keescook@...omium.org>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Oleg Nesterov <oleg@...hat.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mark Brown <broonie@...nel.org>,
	Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

On Fri, Feb 02, 2024 at 04:55:24PM +0000, Al Viro wrote:
> On Fri, Feb 02, 2024 at 04:49:47PM +0000, Al Viro wrote:
> > > +folks from `./scripts/get_maintainer.pl -f arch/arm64/kernel/ptrace.c`
> > > 
> > > Trying to follow the macros to see where "n" comes from is a maze of
> > > twisty little passages, all alike. Hopefully someone from the ARM
> > > world can help tell if the value of 17474 for n here is correct or if
> > > something is wonky.

Nope, that's the "correct" answer...

> > 
> > It might be interesting to have it print the return value of __regset_get()
> > in those cases; if *that* is huge, we really have a problem.  If it ends up
> > small enough to fit into few pages, OTOH...
> > 
> > SVE_VQ_MAX is defined as 255; is that really in units of 128 bits?  IOW,
> > do we really expect to support 32Kbit registers?  That would drive the
> > size into that range, all right, but it would really suck on context
> > switches.
> > 
> > I could be misreading it, though - the macros in there are not easy to
> > follow and I've never dealt with SVE before, so take the above with
> > a cartload of salt.
> 
> Worse - it's SVE_VQ_MAX is 512; sorry about the confusion.  OK, that would
> certainly explain the size (header + 32 registers, each up to 512 * 16 bytes),
> but... ouch.

Mark Brown [+ Cc] has been taking care of SVE in my absence, but
from memory:

The SVE architecture has a really big maximum vector size (16 * 128 =
2048 bits), and there is a theoretical possibility of it getting bigger
in the future, though unlikely.

Real platforms to date have a much smaller limit, though Qemu can go up
to 2048 bits IIUC.

My aim when working on the ABI was to future-proof it against
foreseeable expansion on the architecture side, but this does mean that
we cannot statically determine a sane limit for the vector size.


I suppose we could have had a more sane limit built into the kernel or a
Kconfig option for it, but it seemed simpler just to determine the size
dynamically depending on the task's current state.  This is not so
important for coredumps, but for the the gdbstub wire protocol etc. it
seemed undesirable to have the regset larger than needed.

Hence the reason for adding ->get_size() in
27e64b4be4b8 ("regset: Add support for dynamically sized regsets").

What I guess was not so obvious from the commit message is the
expected relationship between the actual and maximum possible size
of the regset: for SVE the actual size is in practice going to be *much*
smaller than the max, while the max is crazy large because of being an
ABI design limit chosen for futureproofing purposes.



So, if the only reason for trying to migrate to vmalloc() is to cope
with an insanely sized regset on arm64, I think somehow or other we can
avoid that.

Options:

 a) bring back ->get_size() so that we can allocate the correct size
before generating the regset data;

 b) make aarch64_regsets[] __ro_after_init and set
aarch64_regsets[REGSET_SVE].n based on the boot-time probed maximum size
(which will be sane); or

 c) allow membufs to grow if needed (sounds fragile though, and may be
hard to justify just for one arch?).


Thoughts?

If people don't want to bring back get_size(), then (b) doesn't look
too bad.

Cheers
---Dave

Powered by blists - more mailing lists