[8/9] x86/fpu: correctly check for kthreads

Message ID 20190814104131.20190-9-mark.rutland@arm.com
State New
Headers show
Series
  • kthread detection cleanup
Related show

Commit Message

Mark Rutland Aug. 14, 2019, 10:41 a.m.
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

Comments

Sebastian Andrzej Siewior Aug. 14, 2019, 1:07 p.m. | #1
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
Mark Rutland Aug. 14, 2019, 1:39 p.m. | #2
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.
Sebastian Andrzej Siewior Aug. 14, 2019, 2:55 p.m. | #3
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

Patch

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;