Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Dave Martin <Dave.Martin@arm.com>]

Hi,

On Tue, Dec 03, 2024 at 04:12:33PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:33:18PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
> 
> > > +	get_cpu_fpsimd_context();
> 
> > > +		if (current->thread.svcr & SVCR_SM_MASK) {
> > > +			memset(&current->thread.uw.fpsimd_state.vregs, 0,
> > > +			       sizeof(current->thread.uw.fpsimd_state.vregs));
> 
> > Do we need to hold the CPU fpsimd context across this memset?
> 
> > IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
> > dumping of the regs into thread_struct (from current's PoV), but never
> > spontaneously set again.  So ... -> [*]
> 
> Yes, we could drop the lock here.  OTOH this is very simple and easy to
> understand.

Ack; it works either way.

Since this is a Fixes: patch, it may be better to keep it simple.

> 
> > > +		/* Ensure any copies on other CPUs aren't reused */
> > > +		fpsimd_flush_task_state(current);
> 
> > (This is very similar to fpsimd_flush_thread(); can they be unified?)
> 
> I have a half finished series to replace the whole setup around
> accessing the state with get/put operations for working on the state
> which should remove all these functions.  The pile of similarly and
> confusingly named operations we have for working on the state is one of
> the major sources of issues with this code, even when actively working
> on the code it's hard to remember exactly which operation does what
> never mind the rules for which is needed.

Sure, something like that would definitely help.

Cheers
---Dave