Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap

From: "Björn Töpel" <bjorn@kernel.org>
To: Vineet Gupta <vineetg@rivosinc.com>,
	Andy Chiu <andy.chiu@sifive.com>,
	linux-riscv@lists.infradead.org, palmer@dabbelt.com,
	anup@brainfault.org, atishp@atishpatra.org,
	kvm-riscv@lists.infradead.org, kvm@vger.kernel.org
Cc: greentime.hu@sifive.com, guoren@linux.alibaba.com,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Heiko Stuebner <heiko.stuebner@vrull.eu>,
	Andrew Jones <ajones@ventanamicro.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Jisheng Zhang <jszhang@kernel.org>,
	Vincent Chen <vincent.chen@sifive.com>,
	Guo Ren <guoren@kernel.org>, Li Zhengyu <lizhengyu3@huawei.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Richard Henderson <richard.henderson@linaro.org>
Subject: Re: [PATCH -next v13 10/19] riscv: Allocate user's vector context in the first-use trap
Date: Tue, 14 Feb 2023 07:43:21 +0100	[thread overview]
Message-ID: <87sff8ags6.fsf@all.your.base.are.belong.to.us> (raw)
In-Reply-To: <82551518-7b7e-8ac9-7325-5d99d3be0406@rivosinc.com>

Vineet Gupta <vineetg@rivosinc.com> writes:

> On 2/7/23 06:36, Björn Töpel wrote:
>>> +bool rvv_first_use_handler(struct pt_regs *regs)
>>> +{
>>> +	__user u32 *epc = (u32 *)regs->epc;
>>> +	u32 tval = (u32)regs->badaddr;
>>> +
>>> +	/* If V has been enabled then it is not the first-use trap */
>>> +	if (vstate_query(regs))
>>> +		return false;
>>> +	/* Get the instruction */
>>> +	if (!tval) {
>>> +		if (__get_user(tval, epc))
>>> +			return false;
>>> +	}
>>> +	/* Filter out non-V instructions */
>>> +	if (!insn_is_vector(tval))
>>> +		return false;
>>> +	/* Sanity check. datap should be null by the time of the first-use trap */
>>> +	WARN_ON(current->thread.vstate.datap);
>>> +	/*
>>> +	 * Now we sure that this is a V instruction. And it executes in the
>>> +	 * context where VS has been off. So, try to allocate the user's V
>>> +	 * context and resume execution.
>>> +	 */
>>> +	if (rvv_thread_zalloc()) {
>>> +		force_sig(SIGKILL);
>>> +		return true;
>>> +	}
>> Should the altstack size be taken into consideration, like x86 does in
>> validate_sigaltstack() (see __xstate_request_perm()).
>
> For a preexisting alternate stack ?

Yes.

> Otherwise there is no 
> "configuration" like x86 to cross-check against and V fault implies 
> large'ish signal stack.
> See below as well.
>
>> Related; Would it make sense to implement sigaltstack_size_valid() for
>> riscv, analogous to x86?
>
> Indeed we need to do that for the case where alt stack is being setup, 
> *after* V fault-on-first use.
> But how to handle an existing alt stack which might not be big enough to 
> handle V state ?

What I'm getting at is a stricter check at the time of fault
(SIGILL/enable V) handling. If the *existing* altstack is not big
enough, kill the process -- similar to the rvv_thread_zalloc() handling
above.

So, two changes:

1. Disallow V-enablement if the existing altstack does not fit a V-sized
   frame.
2. Sanitize altstack changes when V is enabled.

Other than the altstack handling, I think the series is a good state! It
would great if we could see a v14 land in -next...

Björn