| Message ID | 20250404174435.3288106-4-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Preparatory FPSIMD/SVE/SME fixes | expand |
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. 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. 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.
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index f79b0d5f71ac9..575ececb8b720 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -969,8 +969,6 @@ static int sve_set_common(struct task_struct *target, sve_alloc(target, true); if (!target->thread.sve_state) { ret = -ENOMEM; - clear_tsk_thread_flag(target, TIF_SVE); - target->thread.fp_type = FP_STATE_FPSIMD; goto out; } diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 99ea26d400ffe..40c572869325b 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -443,10 +443,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user) /* From now, fpsimd_thread_switch() won't touch thread.sve_state */ sve_alloc(current, true); - if (!current->thread.sve_state) { - clear_thread_flag(TIF_SVE); + if (!current->thread.sve_state) return -ENOMEM; - } err = __copy_from_user(current->thread.sve_state, (char __user const *)user->sve +
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 unnecessary clearing of TIF_SVE lead to further unnecessary changes in commit: baa8515281b30861 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE") As of that commit, sve_set_common() also sets the task's saved fp_type to FP_STATE_FPSIMD if sve_alloc() fails to allocate memory. This is unnecessary as when sve_state is NULL, the saved fp_type can only legitimately be FP_STATE_FPSIMD. That commit did not update restore_sve_fpsimd_context() similarly, and so restore_sve_fpsimd_context() does not alter the task's saved fp_type if sve_alloc() fails to allocate memory. Remove the unnecessary and confusing clearing of TIF_SVE, and remove the similarly unnecessary and confusing update of the saved fp_type. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Mark Brown <broonie@kernel.org> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kernel/ptrace.c | 2 -- arch/arm64/kernel/signal.c | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-)