x86/fpu: Correct pkru/xstate inconsistency

There are two issues with PKRU handling prior to 5.13. The first is that

when eagerly switching PKRU we check that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

Without reloading the value finish_fpu_load() after being inlined into __switch_to() uses a stale value of current:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   41 f6 45 3e 20          testb  $0x20,0x3e(%r13)
  bb7:   75 1c                   jne    bd5 <__switch_to+0x20e>

By using this_cpu_read() and avoiding the cached value the compiler does insert an additional load instruction and observes the correct value now:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   65 48 8b 05 00 00 00    mov    %gs:0x0(%rip),%rax
  bb9:   00
  bba:   f6 40 3e 20             testb  $0x20,0x3e(%rax)
  bbe:   75 1c                   jne    bdc <__switch_to+0x215>

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

---
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/include/asm/pgtable.h      | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+       if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {
                /*
                 * If the PKRU bit in xsave.header.xfeatures is not set,
                 * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +10 -5

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
        if (!boot_cpu_has(X86_FEATURE_OSPKE))
                return;
 
-       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
        /*
         * The PKRU value in xstate needs to be in sync with the value that is
         * written to the CPU. The FPU restore on return to userland would
         * otherwise load the previous value again.
         */
        fpregs_lock();
-       if (pk)
-               pk->pkru = pkru;
+       /*
+        * The CPU is free to clear the feature bit when the xstate is in the
+        * init state. For this reason, we need to make sure the feature bit is
+        * reset when we're explicitly writing to pkru. If we did not then we
+        * would write to pkru and it would not be saved on a context switch.
+        */
+       current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;
+       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+       BUG_ON(!pk);
+       pk->pkru = pkru;
        __write_pkru(pkru);
        fpregs_unlock();
 }
--

2.35.1.265.g69c8d7142f-goog

Hi Brian,

Quoted 2 lines · Brian Geffon

There are two issues with PKRU handling prior to 5.13. The first is that

Nice catch and work. One question, though: From the above, it seems like this patch only applies to kernels earlier than v5.13 or, more specifically, to v5.4.y and v5.10.y. Is this correct, or should it be applied to the upstream kernel and to all applicable stable releases ?

Thanks,
Guenter

Quoted 97 lines

when eagerly switching PKRU we check that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

Without reloading the value finish_fpu_load() after being inlined into __switch_to() uses a stale value of current:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   41 f6 45 3e 20          testb  $0x20,0x3e(%r13)
  bb7:   75 1c                   jne    bd5 <__switch_to+0x20e>

By using this_cpu_read() and avoiding the cached value the compiler does insert an additional load instruction and observes the correct value now:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   65 48 8b 05 00 00 00    mov    %gs:0x0(%rip),%rax
  bb9:   00
  bba:   f6 40 3e 20             testb  $0x20,0x3e(%rax)
  bbe:   75 1c                   jne    bdc <__switch_to+0x215>

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

---
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/include/asm/pgtable.h      | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+       if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {
                /*
                 * If the PKRU bit in xsave.header.xfeatures is not set,
                 * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +10 -5

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
        if (!boot_cpu_has(X86_FEATURE_OSPKE))
                return;
 
-       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
        /*
         * The PKRU value in xstate needs to be in sync with the value that is
         * written to the CPU. The FPU restore on return to userland would
         * otherwise load the previous value again.
         */
        fpregs_lock();
-       if (pk)
-               pk->pkru = pkru;
+       /*
+        * The CPU is free to clear the feature bit when the xstate is in the
+        * init state. For this reason, we need to make sure the feature bit is
+        * reset when we're explicitly writing to pkru. If we did not then we
+        * would write to pkru and it would not be saved on a context switch.
+        */
+       current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;
+       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+       BUG_ON(!pk);
+       pk->pkru = pkru;
        __write_pkru(pkru);
        fpregs_unlock();
 }
--

2.35.1.265.g69c8d7142f-goog

Quoted 7 lines · Guenter Roeck

Hi Brian,

Quoted 2 lines · Brian Geffon

There are two issues with PKRU handling prior to 5.13. The first is that

Quoted 4 lines

Nice catch and work. One question, though: From the above, it seems like this patch only applies to kernels earlier than v5.13 or, more specifically, to v5.4.y and v5.10.y. Is this correct, or should it be applied to the upstream kernel and to all applicable stable releases ?

This only applies before 5.13, so 5.10.y and 5.4.y, the behavior decoupled PKRU from xstate in a long series from Thomas Gleixner, but the first commit where this would have been fixed in 5.13 would be:

commit 954436989cc550dd91aab98363240c9c0a4b7e23
Author: Thomas Gleixner <[email protected]>
Date: Wed Jun 23 14:02:21 2021 +0200

x86/fpu: Remove PKRU handling from switch_fpu_finish()

Quoted 101 lines

Thanks,
Guenter

Quoted 97 lines

when eagerly switching PKRU we check that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

Without reloading the value finish_fpu_load() after being inlined into __switch_to() uses a stale value of current:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   41 f6 45 3e 20          testb  $0x20,0x3e(%r13)
  bb7:   75 1c                   jne    bd5 <__switch_to+0x20e>

By using this_cpu_read() and avoiding the cached value the compiler does insert an additional load instruction and observes the correct value now:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   65 48 8b 05 00 00 00    mov    %gs:0x0(%rip),%rax
  bb9:   00
  bba:   f6 40 3e 20             testb  $0x20,0x3e(%rax)
  bbe:   75 1c                   jne    bdc <__switch_to+0x215>

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

---
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/include/asm/pgtable.h      | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+       if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {
                /*
                 * If the PKRU bit in xsave.header.xfeatures is not set,
                 * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +10 -5

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
        if (!boot_cpu_has(X86_FEATURE_OSPKE))
                return;
 
-       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
        /*
         * The PKRU value in xstate needs to be in sync with the value that is
         * written to the CPU. The FPU restore on return to userland would
         * otherwise load the previous value again.
         */
        fpregs_lock();
-       if (pk)
-               pk->pkru = pkru;
+       /*
+        * The CPU is free to clear the feature bit when the xstate is in the
+        * init state. For this reason, we need to make sure the feature bit is
+        * reset when we're explicitly writing to pkru. If we did not then we
+        * would write to pkru and it would not be saved on a context switch.
+        */
+       current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;
+       pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+       BUG_ON(!pk);
+       pk->pkru = pkru;
        __write_pkru(pkru);
        fpregs_unlock();
 }
--

2.35.1.265.g69c8d7142f-goog

This only applies before 5.13, so 5.10.y and 5.4.y, the behavior decoupled PKRU from xstate in a long series from Thomas Gleixner, but the first commit where this would have been fixed in 5.13 would be:

commit 954436989cc550dd91aab98363240c9c0a4b7e23
Author: Thomas Gleixner <[email protected]>
Date: Wed Jun 23 14:02:21 2021 +0200

x86/fpu: Remove PKRU handling from switch_fpu_finish()

There are two issues with PKRU handling prior to 5.13. The first is that when eagerly switching PKRU we check that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

Without reloading the value finish_fpu_load() after being inlined into __switch_to() uses a stale value of current:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   41 f6 45 3e 20          testb  $0x20,0x3e(%r13)
  bb7:   75 1c                   jne    bd5 <__switch_to+0x20e>

By using this_cpu_read() and avoiding the cached value the compiler does insert an additional load instruction and observes the correct value now:

  ba1:   8b 35 00 00 00 00       mov    0x0(%rip),%esi
  ba7:   f0 41 80 4d 01 40       lock orb $0x40,0x1(%r13)
  bad:   e9 00 00 00 00          jmp    bb2 <__switch_to+0x1eb>
  bb2:   65 48 8b 05 00 00 00    mov    %gs:0x0(%rip),%rax
  bb9:   00
  bba:   f6 40 3e 20             testb  $0x20,0x3e(%rax)
  bbe:   75 1c                   jne    bdc <__switch_to+0x215>

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

---
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/include/asm/pgtable.h      | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

There are two issues with PKRU handling prior to 5.13.

The first is that when eagerly switching PKRU we check that current

is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

arch/x86/include/asm/fpu/internal.h +0 -0

 		/*
 		 * If the PKRU bit in xsave.header.xfeatures is not set,
 		 * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +0 -2

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
 	if (!boot_cpu_has(X86_FEATURE_OSPKE))
 		return;
 
-	pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
 	/*
 	 * The PKRU value in xstate needs to be in sync with the value that is
 	 * written to the CPU. The FPU restore on return to userland would
 	 * otherwise load the previous value again.
 	 */

arch/x86/include/asm/pgtable.h +1 -0

+	pk->pkru = pkru;
 	__write_pkru(pkru);
 	fpregs_unlock();
 }

Quoted 1 line · Brian Geffon

There are two issues with PKRU handling prior to 5.13.

Are you sure both of these issues were introduced by 0cecca9d03c? I'm surprised that the get_xsave_addr() issue is not older.

Should this be two patches?

Quoted 2 lines

The first is that when eagerly switching PKRU we check that current

Don't forget to write in imperative mood. No "we's", please.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

This goes for changelogs and comments too.

Quoted 12 lines

is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

It makes *sense* that there would be a place in the context switch code where 'current' is wonky, but I never realized this. This seems really fragile, but *also* trivially detectable.

Is the PKRU code really the only code to use 'current' in a buggy way like this?

Quoted 9 lines

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

^ again

It's probably worth noting that the AMD init tracker is a lot more aggressive than Intel's. On Intel, I think XRSTOR is the only way to get back to the init state. You're obviously hitting this on AMD.

It's also *very* unlikely that PKRU gets back to a value of 0. I think we added a selftest for this case in later kernels.

That helps explain why this bug hung around for so long.

arch/x86/include/asm/fpu/internal.h static inline void switch_fpu_finish(struct fpu *new_fpu) · line 601

Quoted 10 lines

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+     if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {

This really deserves a specific comment.

Quoted 18 lines

arch/x86/include/asm/fpu/internal.h +0 -0

              /*
               * If the PKRU bit in xsave.header.xfeatures is not set,
               * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +0 -2

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
      if (!boot_cpu_has(X86_FEATURE_OSPKE))
              return;
 
-     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
      /*
       * The PKRU value in xstate needs to be in sync with the value that is
       * written to the CPU. The FPU restore on return to userland would
       * otherwise load the previous value again.
       */

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 155

Quoted 10 lines

arch/x86/include/asm/pgtable.h +7 -2

      fpregs_lock();
-     if (pk)
-             pk->pkru = pkru;
+     /*
+      * The CPU is free to clear the feature bit when the xstate is in the
+      * init state. For this reason, we need to make sure the feature bit is
+      * reset when we're explicitly writing to pkru. If we did not then we
+      * would write to pkru and it would not be saved on a context switch.
+      */
+     current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;

I don't think we need to describe how the init optimization works again.
I'm also not sure it's worth mentioning context switches here. It's a
wider problem than that. Maybe:

/*
* All fpregs will be XRSTOR'd from this buffer before returning
* to userspace. Ensure that XRSTOR does not init PKRU and that
* get_xsave_addr() will work.
*/

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 157

Quoted 2 lines

arch/x86/include/asm/pgtable.h +2 -0

+     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+     BUG_ON(!pk);

A BUG_ON() a line before a NULL pointer dereference doesn't tend to do much good.

Quoted 4 lines

arch/x86/include/asm/pgtable.h +1 -0

+     pk->pkru = pkru;
      __write_pkru(pkru);
      fpregs_unlock();
 }

Quoted 16 lines

Quoted 11 lines

is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

It makes *sense* that there would be a place in the context switch code where 'current' is wonky, but I never realized this. This seems really fragile, but *also* trivially detectable.

Quoted 2 lines

Is the PKRU code really the only code to use 'current' in a buggy way like this?

Yes, because the remaining code in __switch_to() references the next task as next_p rather than current. Technically, it might be more correct to pass next_p to switch_fpu_finish(), what do you think? This may make sense since we're also passing the next fpu anyway.

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

---
 arch/x86/include/asm/fpu/internal.h | 13 ++++++++-----
 arch/x86/kernel/process_32.c        |  6 ++----
 arch/x86/kernel/process_64.c        |  6 ++----
 3 files changed, 12 insertions(+), 13 deletions(-)

Quoted 27 lines · Brian Geffon

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

Quoted 5 lines

---
 arch/x86/include/asm/fpu/internal.h | 13 ++++++++-----
 arch/x86/kernel/process_32.c        |  6 ++----
 arch/x86/kernel/process_64.c        |  6 ++----
 3 files changed, 12 insertions(+), 13 deletions(-)

So this is ONLY for 5.4.y and 5.10.y? I'm really really loath to take non-upstream changes as 95% of the time (really) it goes wrong.

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

And finally, what's wrong with 60+ patches to backport to fix a severe issue? What's preventing that from happening? Did you try it and see what exactly is involved?

thanks,

greg k-h

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7.

Quoted 7 lines · Brian Geffon

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7.

I've got an i7-1165G7, but I'm not seeing any failures on a 5.11 distro kernel.

How long does this take for you to reproduce?

Quoted 3 lines · Brian Geffon

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

Quoted 4 lines

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7.

I've got an i7-1165G7, but I'm not seeing any failures on a 5.11 distro kernel.

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Brian Geffon

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

Quoted 8 lines

Quoted 4 lines

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7.

I've got an i7-1165G7, but I'm not seeing any failures on a 5.11 distro kernel.

Hi Dave,
I suspect the reason you're not seeing it is toolchain related, I'm building with clang 14.0.0 and it produces the sequence of instructions which use the cached value. Let me know if there is anything I can do to help you investigate this further.

Quoted 35 lines · Greg KH

Quoted 32 lines · Brian Geffon

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

---
 arch/x86/include/asm/fpu/internal.h | 13 ++++++++-----
 arch/x86/kernel/process_32.c        |  6 ++----
 arch/x86/kernel/process_64.c        |  6 ++----
 3 files changed, 12 insertions(+), 13 deletions(-)

Quoted 2 lines

So this is ONLY for 5.4.y and 5.10.y? I'm really really loath to take non-upstream changes as 95% of the time (really) it goes wrong.

That's correct, this bug was introduced in 5.2 and that code was completely refactored in 5.13 indirectly fixing it.

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7. I'm happy to commit to addressing any bugs that may appear. I'll see what the maintainers say, but there is also a smaller fix that just involves using this_cpu_read() in switch_fpu_finish() for this specific issue, although that approach isn't as clean.

Quoted 36 lines · Brian Geffon

Quoted 35 lines · Greg KH

Quoted 32 lines · Brian Geffon

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

---
 arch/x86/include/asm/fpu/internal.h | 13 ++++++++-----
 arch/x86/kernel/process_32.c        |  6 ++----
 arch/x86/kernel/process_64.c        |  6 ++----
 3 files changed, 12 insertions(+), 13 deletions(-)

Quoted 5 lines

Quoted 2 lines

So this is ONLY for 5.4.y and 5.10.y? I'm really really loath to take non-upstream changes as 95% of the time (really) it goes wrong.

That's correct, this bug was introduced in 5.2 and that code was completely refactored in 5.13 indirectly fixing it.

What series of commits contain that work?

And again, why not just take them? What is wrong with that if this is such a big issue?

Quoted 5 lines

Quoted 4 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

Quoted 8 lines

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7. I'm happy to commit to addressing any bugs that may appear. I'll see what the maintainers say, but there is also a smaller fix that just involves using this_cpu_read() in switch_fpu_finish() for this specific issue, although that approach isn't as clean.

Can you add the test to the in-kernel tests so that we make sure it is fixed and never comes back?

thanks,

greg k-h

Quoted 11 lines

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7. I'm happy to commit to addressing any bugs that may appear. I'll see what the maintainers say, but there is also a smaller fix that just involves using this_cpu_read() in switch_fpu_finish() for this specific issue, although that approach isn't as clean.

Can you add the test to the in-kernel tests so that we make sure it is fixed and never comes back?

Quoted 11 lines · Greg KH

Quoted 11 lines

Quoted 3 lines

How was this tested, and what do the maintainers of this subsystem think? And will you be around to fix the bugs in this when they are found?

This has been trivial to reproduce, I've used a small repro which I've put here: https://gist.github.com/bgaff/9f8cbfc8dd22e60f9492e4f0aff8f04f
, I also was able to reproduce this using the protection_keys self tests on a 11th Gen Core i5-1135G7. I'm happy to commit to addressing any bugs that may appear. I'll see what the maintainers say, but there is also a smaller fix that just involves using this_cpu_read() in switch_fpu_finish() for this specific issue, although that approach isn't as clean.

Quoted 2 lines

Can you add the test to the in-kernel tests so that we make sure it is fixed and never comes back?

It would be great if Brian could confirm this. But, I'm 99% sure that this can be reproduced in the vm/protection_keys.c selftest, if you run it for long enough.

The symptom here is corruption of the PKRU register. I created *lots* of bugs like this during protection keys development so the selftest keeps a shadow copy of the register to specifically watch for corruption.

It's _plausible_ that no one ever ran the pkey selftests with a clang-compiled kernel for long enough to hit this issue.

How would you and Greg KH like to proceed with this? I'm happy to help however I can.

Quoted 2 lines · Brian Geffon

How would you and Greg KH like to proceed with this? I'm happy to help however I can.

If I could wave a magic wand, I'd just apply the whole FPU rewrite to stable.

My second choice would be to stop managing PKRU with XSAVE. x86_pkru_load() uses WRPKRU instead of XSAVE and keeps the task's PKRU in task->pkru instead of the XSAVE buffer. Doing that will take some care, including pulling XFEATURE_PKRU out of the feature mask (RFBM) at XRSTOR. I _think_ that can be done in a manageable set of patches which
will keep stable close to mainline. I recognize that more bugs might
get introduced in the process which are unique to stable.

If you give that a shot and realize that it's not feasible to do a subset, then we can fall back to the minimal fix. I'm not asking for a multi-month engineering effort here. Maybe an hour or two to see if it's really as scary as it looks.

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

Quoted 21 lines · Brian Geffon

When eagerly switching PKRU in switch_fpu_finish() it checks that current is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. To resolve this reference next_p directly rather than relying on current.

As written it's possible when switching from a kernel thread to a userspace thread to observe a cached PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all that is considered in that situation are the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

This behavior only exists between 5.2 and 5.13 when it was fixed by a rewrite decoupling PKRU from xstate, in:
commit 954436989cc5 ("x86/fpu: Remove PKRU handling from switch_fpu_finish()")

Unfortunately backporting the fix from 5.13 is probably not realistic as it's part of a 60+ patch series which rewrites most of the PKRU handling.

Quoted 6 lines

Fixes 0cecca9d03c9 ("x86/fpu: Eager switch PKRU state")

Signed-off-by Brian Geffon <[email protected]>

Signed-off-by Willis Kung <[email protected]>

Tested-by Willis Kung <[email protected]>

Cc <[email protected]> # v5.4.x

Cc <[email protected]> # v5.10.x

I don't like forking the stable code from mainline. But I also think that backporting the FPU reworking that changed the PKRU handling is likely to cause more bugs in stable than it fixes.

This fix is at least isolated to the protection keys code.

Acked-by Dave Hansen <[email protected]>

Quoted 1 line · Brian Geffon

There are two issues with PKRU handling prior to 5.13.

Are you sure both of these issues were introduced by 0cecca9d03c? I'm surprised that the get_xsave_addr() issue is not older.

Should this be two patches?

Quoted 1 line

The first is that when eagerly switching PKRU we check that current

Don't forget to write in imperative mood. No "we's", please.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

This goes for changelogs and comments too.

Quoted 11 lines

is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

It makes *sense* that there would be a place in the context switch code where 'current' is wonky, but I never realized this. This seems really fragile, but *also* trivially detectable.

Is the PKRU code really the only code to use 'current' in a buggy way like this?

Quoted 8 lines

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

^ again

It's probably worth noting that the AMD init tracker is a lot more aggressive than Intel's. On Intel, I think XRSTOR is the only way to get back to the init state. You're obviously hitting this on AMD.

It's also *very* unlikely that PKRU gets back to a value of 0. I think we added a selftest for this case in later kernels.

That helps explain why this bug hung around for so long.

arch/x86/include/asm/fpu/internal.h static inline void switch_fpu_finish(struct fpu *new_fpu) · line 601

Quoted 10 lines

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+     if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {

This really deserves a specific comment.

Quoted 18 lines

arch/x86/include/asm/fpu/internal.h +0 -0

              /*
               * If the PKRU bit in xsave.header.xfeatures is not set,
               * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +0 -2

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
      if (!boot_cpu_has(X86_FEATURE_OSPKE))
              return;
 
-     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
      /*
       * The PKRU value in xstate needs to be in sync with the value that is
       * written to the CPU. The FPU restore on return to userland would
       * otherwise load the previous value again.
       */

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 155

Quoted 10 lines

arch/x86/include/asm/pgtable.h +7 -2

      fpregs_lock();
-     if (pk)
-             pk->pkru = pkru;
+     /*
+      * The CPU is free to clear the feature bit when the xstate is in the
+      * init state. For this reason, we need to make sure the feature bit is
+      * reset when we're explicitly writing to pkru. If we did not then we
+      * would write to pkru and it would not be saved on a context switch.
+      */
+     current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;

I don't think we need to describe how the init optimization works again.
I'm also not sure it's worth mentioning context switches here. It's a
wider problem than that. Maybe:

/*
* All fpregs will be XRSTOR'd from this buffer before returning
* to userspace. Ensure that XRSTOR does not init PKRU and that
* get_xsave_addr() will work.
*/

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 157

Quoted 2 lines

arch/x86/include/asm/pgtable.h +2 -0

+     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+     BUG_ON(!pk);

A BUG_ON() a line before a NULL pointer dereference doesn't tend to do much good.

Quoted 4 lines

arch/x86/include/asm/pgtable.h +1 -0

+     pk->pkru = pkru;
      __write_pkru(pkru);
      fpregs_unlock();
 }

Quoted 46 lines · Dave Hansen

Quoted 1 line · Brian Geffon

There are two issues with PKRU handling prior to 5.13.

Are you sure both of these issues were introduced by 0cecca9d03c? I'm surprised that the get_xsave_addr() issue is not older.

Should this be two patches?

Quoted 1 line

The first is that when eagerly switching PKRU we check that current

Don't forget to write in imperative mood. No "we's", please.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

This goes for changelogs and comments too.

Quoted 11 lines

is not a kernel thread as kernel threads will never use PKRU. It's possible that this_cpu_read_stable() on current_task (ie. get_current()) is returning an old cached value. By forcing the read with this_cpu_read() the correct task is used. Without this it's possible when switching from a kernel thread to a userspace thread that we'll still observe the PF_KTHREAD flag and never restore the PKRU. And as a result this issue only occurs when switching from a kernel thread to a userspace thread, switching from a non kernel thread works perfectly fine because all we consider in that situation is the flags from some other non kernel task and the next fpu is passed in to switch_fpu_finish().

It makes *sense* that there would be a place in the context switch code where 'current' is wonky, but I never realized this. This seems really fragile, but *also* trivially detectable.

Is the PKRU code really the only code to use 'current' in a buggy way like this?

Quoted 8 lines

The second issue is when using write_pkru() we only write to the xstate when the feature bit is set because get_xsave_addr() returns NULL when the feature bit is not set. This is problematic as the CPU is free to clear the feature bit when it observes the xstate in the init state, this behavior seems to be documented a few places throughout the kernel. If the bit was cleared then in write_pkru() we would happily write to PKRU without ever updating the xstate, and the FPU restore on return to userspace would load the old value agian.

Quoted 6 lines

^ again

It's probably worth noting that the AMD init tracker is a lot more aggressive than Intel's. On Intel, I think XRSTOR is the only way to get back to the init state. You're obviously hitting this on AMD.

Brian should correct me here, but I think we have seen this with one specific Intel CPU.

Brian, would it make sense to list the affected CPU model(s), or at least the ones where we have observed the problem ?

Thanks,
Guenter

Quoted 68 lines

It's also *very* unlikely that PKRU gets back to a value of 0. I think we added a selftest for this case in later kernels.

That helps explain why this bug hung around for so long.

arch/x86/include/asm/fpu/internal.h static inline void switch_fpu_finish(struct fpu *new_fpu) · line 601

Quoted 10 lines

arch/x86/include/asm/fpu/internal.h +1 -1

index 03b3de491b5e..540bda5bdd28 100644
@@ -598,7 +598,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->flags & PF_KTHREAD)) {
+     if (!(this_cpu_read(current_task)->flags & PF_KTHREAD)) {

This really deserves a specific comment.

Quoted 18 lines

arch/x86/include/asm/fpu/internal.h +0 -0

              /*
               * If the PKRU bit in xsave.header.xfeatures is not set,
               * then the PKRU component was in init state, which means

arch/x86/include/asm/pgtable.h +0 -2

index 9e71bf86d8d0..aa381b530de0 100644
@@ -140,16 +140,22 @@ static inline void write_pkru(u32 pkru)
      if (!boot_cpu_has(X86_FEATURE_OSPKE))
              return;
 
-     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
-
      /*
       * The PKRU value in xstate needs to be in sync with the value that is
       * written to the CPU. The FPU restore on return to userland would
       * otherwise load the previous value again.
       */

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 155

Quoted 10 lines

arch/x86/include/asm/pgtable.h +7 -2

      fpregs_lock();
-     if (pk)
-             pk->pkru = pkru;
+     /*
+      * The CPU is free to clear the feature bit when the xstate is in the
+      * init state. For this reason, we need to make sure the feature bit is
+      * reset when we're explicitly writing to pkru. If we did not then we
+      * would write to pkru and it would not be saved on a context switch.
+      */
+     current->thread.fpu.state.xsave.header.xfeatures |= XFEATURE_MASK_PKRU;

I don't think we need to describe how the init optimization works again.
I'm also not sure it's worth mentioning context switches here. It's a
wider problem than that. Maybe:

/*
* All fpregs will be XRSTOR'd from this buffer before returning
* to userspace. Ensure that XRSTOR does not init PKRU and that
* get_xsave_addr() will work.
*/

arch/x86/include/asm/pgtable.h static inline void write_pkru(u32 pkru) · line 157

Quoted 2 lines

arch/x86/include/asm/pgtable.h +2 -0

+     pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
+     BUG_ON(!pk);

A BUG_ON() a line before a NULL pointer dereference doesn't tend to do much good.

Quoted 4 lines

arch/x86/include/asm/pgtable.h +1 -0

+     pk->pkru = pkru;
      __write_pkru(pkru);
      fpregs_unlock();
 }