Messages in this thread | | | Date | Thu, 17 Oct 2019 17:09:03 +0100 | From | Dave Martin <> | Subject | Re: [PATCH 2/3] arm64: nofpsmid: Clear TIF_FOREIGN_FPSTATE flag for early tasks |
| |
On Thu, Oct 17, 2019 at 01:42:37PM +0100, Suzuki K Poulose wrote: > Hi Dave > > Thanks for the comments. > > On 11/10/2019 12:26, Dave Martin wrote: > >On Thu, Oct 10, 2019 at 06:15:16PM +0100, Suzuki K Poulose wrote: > >>We detect the absence of FP/SIMD after we boot the SMP CPUs, and by then > >>we have kernel threads running already with TIF_FOREIGN_FPSTATE set which > >>could be inherited by early userspace applications (e.g, modprobe triggered > >>from initramfs). This could end up in the applications stuck in > >>do_nofity_resume() as we never clear the TIF flag, once we now know that > >>we don't support FP. > >> > >>Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag > >>for tasks which may have them set, as we would have done in the normal > >>case, but avoiding touching the hardware state (since we don't support any). > >> > >>Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD") > >>Cc: Will Deacon <will@kernel.org> > >>Cc: Mark Rutland <mark.rutland@arm.com> > >>Cc: Catalin Marinas <catalin.marinas@arm.com> > >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > >>--- > >> arch/arm64/kernel/fpsimd.c | 26 ++++++++++++++++---------- > >> 1 file changed, 16 insertions(+), 10 deletions(-) > >> > >>diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >>index 37d3912cfe06..dfcdd077aeca 100644 > >>--- a/arch/arm64/kernel/fpsimd.c > >>+++ b/arch/arm64/kernel/fpsimd.c > >>@@ -1128,12 +1128,19 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state, > >> */ > >> void fpsimd_restore_current_state(void) > >> { > >>- if (!system_supports_fpsimd()) > >>- return; > >>- > >> get_cpu_fpsimd_context(); > >>- > >>- if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > >>+ /* > >>+ * For the tasks that were created before we detected the absence of > >>+ * FP/SIMD, the TIF_FOREIGN_FPSTATE could be set via fpsimd_thread_switch() > >>+ * and/or could be inherited from the parent(init_task has this set). Even > >>+ * though userspace has not run yet, this could be inherited by the > >>+ * processes forked from one of those tasks (e.g, modprobe from initramfs). > >>+ * If the system doesn't support FP/SIMD, we must clear the flag for the > >>+ * tasks mentioned above, to indicate that the FPSTATE is clean (as we > >>+ * can't have one) to avoid looping for ever to clear the flag. > >>+ */ > >>+ if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE) && > >>+ system_supports_fpsimd()) { > > > >I'm not too keen on this approach: elsewhere we just stub out all the > >FPSIMD handling logic if !system_supports_fpsimd() -- I think we should > >be using this test everywhere rather than relying on TIF_FOREIGN_FPSTATE. > > We used to do this. But the flag is not cleared anymore once we detect > the absence of FP/SIMD. > > >Rather, I feel that TIF_FOREIGN_FPSTATE means "if this is a user task > >and this task is current() and the system supports FPSIMD at all, this > >task's FPSIMD state is not loaded in the cpu". > > Yes, that is correct. However, we ran some tasks, even before we detected > that the FPSIMD is missing. So, we need to clear the flag for those tasks > to make sure the flag state is consistent, as explained in the comment.
I think there's a misunderstanding here somewhere.
What I'm saying it that we shouldn't even look at TIF_FOREIGN_FPSTATE if !system_supports_fpsimd() -- i.e., when checking whether there is any FPSIMD context handling work to do, !system_supports_fpsimd() should take precedence.
Firstly, this replaces the runtime TIF_FOREIGN_FPSTATE check with a static key check in the !system_supprts_fpsimd() case, and second, the "work to do" condition is never wrong, even temporarily.
The "work to do" condition is now
system_supports_fpsimd() && test_thread_flag(TIF_FOREIGN_FPSTATE)
instead of
test_thread_flag(TIF_FOREIGN_FPSTATE).
Code that _only writes_ the TIF_FORGIEN_FPSTATE flag can continue to do so harmlessly if we do things this way.
In do_notify_resume() this doesn't quite work, but it's acceptable to fall spuriously into fpsimd_restore_current_state() provided that we check for !system_supports_fpsimd() in there. Which we already do. In this one case, we should clear TIF_FOREIGN_FPSTATE so this backwards checking doesn't send do_notify_resume() into a spin waiting for the flag to go clear.
Another option is to clear TIF_FOREIGN_FPSTATE from _TIF_WORK_MASK when checking for pending work in do_notify_resume(), so that we effectively ignore TIF_FOREIGN_FPSTATE here too. This would be a static key based check again, so should be fast.
I think this is a cleaner approach than trying to clean up TIF_FOREIGN_FPSTATE for each task lazily in some random places -- otherwise we may need to do the cleanup anywhere that the flag is accessed, and that happens all over the place.
Does that make sense? More below.
> Another option is to clear this flag in fpsimd_thread_switch(), something like, > rather than sprinkling the "flag fixup" everywhere: > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index dfcdd077aeca..2d8091b6ebfb 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -982,9 +982,14 @@ void fpsimd_thread_switch(struct task_struct *next) > { > bool wrong_task, wrong_cpu; > > - if (!system_supports_fpsimd()) > + if (!system_supports_fpsimd()) { > + /* > + * Clear any TIF flags which may have been set, before we > + * detected the absense of FPSIMD. > + */ > + clear_task_thread_flag(next, TIF_FOREIGN_FPSTATE); > return; > - > + } > __get_cpu_fpsimd_context(); > > > And also at : > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index a47462def04b..cd8e94d5dc8d 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -374,7 +374,10 @@ int copy_thread(unsigned long clone_flags, unsigned > long stack_start, > * Otherwise we could erroneously skip reloading the FPSIMD > * registers for p. > */ > - fpsimd_flush_task_state(p); > + if (system_supports_fpsimd()) > + fpsimd_flush_task_state(p); > + else > + clear_tsk_thread_flag(p, TIF_FOREIGN_FPSTATE); > > if (likely(!(p->flags & PF_KTHREAD))) { > *childregs = *current_pt_regs(); > > > That way we make sure the flag doesn't violate our assumption and we can > bail out calling into the stubs with the existing checks.
But we may still go wrong where this flag is checked in signal handling / ptrace / KVM, no?
> >I think we should ensure that any check on TIF_FOREIGN_FPSTATE is > >shadowed by a check on system_supports_fpsimd() somewhere. This already > >exists in many places -- we just need to fill in the missing ones. > > > >fpsimd_save() is a backend function that should only be called if > >system_supports_fpsimd(), so that should not need any check internally, > >but we should make sure that calls to this function are appropriately > >protected with in if (system_supports_fpsimd()). > > Agree. > > > > >For other maintenance functions intended for outside callers: > > > > * fpsimd_bind_task_to_cpu() > This was/is called from fpsimd_{update,restore}_current_state() > which are protected with system_supports_fpsimd() check already. > > > * fpsimd_bind_state_to_cpu() > > This is only used by the KVM code and will only be used after we > have finalized the capabilities and thus we are covered by the > system_supports_fpsimd() check in __hyp_handle_fpsimd() which > sets the FP_ENABLED flag. > > > * fpsimd_flush_task_state() > > This seemed rather innocent, but looking at the callers, I realise > that we need the check in fpr_{get/set} for NT_REGS and return errors > if we are asked to deal with FP regs. > > > * fpsimd_save_and_flush_cpu_state() > > This must not be called and is only triggered from KVM (covered) and > the PM notifier (which is not registered if fp/simd is missing). > > > > >the situation is less clear. Does is make sense to call these at all > >if !system_supports_fpsimd()? I'm not currently sure. We could at > >least drop some WARN_ON() into these to check, after revieweing their > >callsites. > > Sure, I agree.
My point is that we shouldn't knowingly introduce fragility, because this code is hard enough to understand already -- I've spent literally months of my life fighting it ;)
Hacking TIF_FOREIGN_FPSTATE at a few random sites feels inherently more fragile than simply ignoring the flag when !system_supports_fpsimd().
If there's a simple approach that can never go wrong, we should go for that unless it introduces unacceptable overheads.
[...]
Cheers ---Dave
| |