| Message ID | 20190814104131.20190-9-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | kthread detection cleanup | expand |
On 2019-08-14 11:41:30 [+0100], Mark Rutland wrote: > Per commit: > > 0cecca9d03c964ab ("x86/fpu: Eager switch PKRU state") > > ... switch_fpu_state() is trying to distinguish user threads from > kthreads, such that kthreads consistently use init_pkru_value. It does > do by looking at current->mm. > > In general, a non-NULL current->mm doesn't imply that current is a > kthread, as kthreads can install an mm via use_mm(), and so it's > preferable to use is_kthread() to determine whether a thread is a > kthread. I think this was missed in commit. 8d3289f2fa1e0 ("x86/fpu: Don't use current->mm to check for a kthread") A kthread with use_mm() would load here non-existing FPU state. Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Sebastian
On Wed, Aug 14, 2019 at 03:07:08PM +0200, Sebastian Andrzej Siewior wrote: > On 2019-08-14 11:41:30 [+0100], Mark Rutland wrote: > > Per commit: > > > > 0cecca9d03c964ab ("x86/fpu: Eager switch PKRU state") > > > > ... switch_fpu_state() is trying to distinguish user threads from > > kthreads, such that kthreads consistently use init_pkru_value. It does > > do by looking at current->mm. > > > > In general, a non-NULL current->mm doesn't imply that current is a > > kthread, as kthreads can install an mm via use_mm(), and so it's > > preferable to use is_kthread() to determine whether a thread is a > > kthread. > > I think this was missed in commit. > 8d3289f2fa1e0 ("x86/fpu: Don't use current->mm to check for a kthread") Yup, though if it's a bug it's been a bug since commit: 0cecca9d03c964ab ("x86/fpu: Eager switch PKRU state") ... which I guess the fixes tag would have to mention. > > A kthread with use_mm() would load here non-existing FPU state. > > Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Given the above, should I add the fixes tag (for 0cecca9d03c964ab)? Thanks, Mark.
On 2019-08-14 14:39:10 [+0100], Mark Rutland wrote: > I think this was missed in commit. > > 8d3289f2fa1e0 ("x86/fpu: Don't use current->mm to check for a kthread") > > Yup, though if it's a bug it's been a bug since commit: > > 0cecca9d03c964ab ("x86/fpu: Eager switch PKRU state") > > ... which I guess the fixes tag would have to mention. > > > > > A kthread with use_mm() would load here non-existing FPU state. > > > > Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Given the above, should I add the fixes tag (for 0cecca9d03c964ab)? Buh. Either that or hch's commit (8d3289f2fa1e0 ("x86/fpu: Don't use current->mm to check for a kthread"). That will trigger a stable backport (for 5.2) asking what to do about is_kthread() so please leave a hint to use the PF_ flag like hch did. Now that I think about it, even if we end up in kthread with use_mm() then its FPU state is non-existent which means get_xsave_addr() returns NULL. This is okay / expected but it triggers the WARN_ONCE(). > Thanks, > Mark. Sebastian
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index 4c95c365058a..ed3d85fa7c67 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -607,7 +607,7 @@ static inline void switch_fpu_finish(struct fpu *new_fpu) * PKRU state is switched eagerly because it needs to be valid before we * return to userland e.g. for a copy_to_user() operation. */ - if (current->mm) { + if (!is_kthread(current)) { pk = get_xsave_addr(&new_fpu->state.xsave, XFEATURE_PKRU); if (pk) pkru_val = pk->pkru;
Per commit: 0cecca9d03c964ab ("x86/fpu: Eager switch PKRU state") ... switch_fpu_state() is trying to distinguish user threads from kthreads, such that kthreads consistently use init_pkru_value. It does do by looking at current->mm. In general, a non-NULL current->mm doesn't imply that current is a kthread, as kthreads can install an mm via use_mm(), and so it's preferable to use is_kthread() to determine whether a thread is a kthread. For consistency, let's use is_kthread() here. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Borislav Petkov <bp@suse.de> Cc: Christoph Hellwig <hch@lst.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Rik van Riel <riel@surriel.com> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/x86/include/asm/fpu/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.11.0