[PATCH 03/14] arm64/fpsimd: Remove redundant clearing of TIF_SVE

[Mark Rutland]

On Fri, Apr 04, 2025 at 08:27:21PM +0100, Mark Brown wrote:
> On Fri, Apr 04, 2025 at 06:44:24PM +0100, Mark Rutland wrote:
> 
> > TIF_SVE implies that sve_state has been allocated. Barring specific
> > transient periods (e.g. during changes to SVE/SME vector lengths), it is
> > not valid for a task to have TIF_SVE set while sve_state is NULL.
> 
> > This was not taken into account in commit:
> 
> >   7559b7d7d651d397 ("arm64/sve: Better handle failure to allocate SVE register storage")
> 
> > As of that commit, sve_set_common() and restore_sve_fpsimd_context()
> > clear TIF_SVE if sve_alloc() fails to allocate memory. In these cases
> > TIF_SVE cannot legitimately have been set to begin with, and clearing
> > TIF_SVE only serves to complicate the code. No other code paths clear
> > TIF_SVE if sve_alloc() fails to allocate memory.
> 
> The OOM handling is all rather unfortunate, it's inconsistent and
> there's definite issues.

Agreed; it's a mess.

> In the case of sve_set_common() we might be changing the vector length,
> in which case vec_set_vector_length() will have attempted to reallocate
> the SVE state buffer.  It does not check that the buffer was allocated
> so won't return an error, and the sve_alloc() we do in sve_set_common()
> to flush the prior state would then actually try to do an allocation
> rather than flushing and so could potentially fail.  We should handle
> this in vec_set_vector_length() and/or sve_alloc() itself though which
> would mean that this change would be fine.

Yes; we really must handle this consistently at the point the allocation
fails. It's rather annoying that unchecked allocations got introduced
since commit:

  7559b7d7d651d397d ("arm64/sve: Better handle failure to allocate SVE register storage")

... since after that point we really should have known better.

Note that today it's possible to get to vec_set_vector_length() via
other means, e.g. via prctl(PR_SVE_SET_VL, ...), so even without this
patch, that logic is broken.

For the moment I'll drop this patch from this series and send a more
complete fix as part of a follow-up.

> In the case of restore_sve_fpsimd_context() we don't support vector
> length changes via sigreturn so there's no issue, if the user is
> enabling SVE for the first time we won't set TIF_SVE until later.
> 
> > Remove the unnecessary and confusing clearing of TIF_SVE, and remove the
> > similarly unnecessary and confusing update of the saved fp_type.
> 
> The update to the saved fp_type should go into the called functions too,
> if we ended up in a situation where we don't have a SVE state buffer we
> should ensure we don't try to load from it.

Maybe.

TBH, I don't think we should *ever* change the stored representation as
a result of failing to allocate memory. We should be able to allocate
the new buffer before freeing the old one, and if we cannot allocate the
new buffer we can return -ENOMEM without discarding the old state.

Mark.