[PATCH v2 1/2] x86/fpu: Fix NULL dereference in avx512_status()

[PATCH v2 1/2] x86/fpu: Fix NULL dereference in avx512_status()

[Sohil Mehta]

From: Fushuai Wang <wangfushuai@baidu.com>

When CONFIG_X86_DEBUG_FPU is set, reading /proc/[kthread]/arch_status
causes a NULL pointer dereference.

For Kthreads tasks:
  proc_pid_arch_status()
    avx512_status()
      x86_task_fpu() => returns NULL when CONFIG_X86_DEBUG_FPU=y
      x86_task_fpu()->avx512_timestamp => NULL dereference

Kernel threads aren't expected to access FPU state directly. However,
avx512_timestamp resides within struct fpu which lead to this unique
situation.

It is uncertain whether kernel threads use AVX-512 in a meaningful way
that needs userspace reporting. For now, avoid reporting AVX-512 usage
for kernel threads.

Fixes: 22aafe3bcb67 ("x86/fpu: Remove init_task FPU state dependencies, add debugging warning for PF_KTHREAD tasks")
Signed-off-by: Fushuai Wang <wangfushuai@baidu.com>
Co-developed-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v2:
 - Avoid making the fix dependent on CONFIG_X86_DEBUG_FPU.
 - Include PF_USER_WORKER in the kernel thread check.
 - Update commit message for clarity.

v1: https://lore.kernel.org/lkml/20250717094308.94450-1-wangfushuai@baidu.com/
---
 arch/x86/kernel/fpu/xstate.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 9aa9ac8399ae..a75077c645b6 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1855,19 +1855,18 @@ long fpu_xstate_prctl(int option, unsigned long arg2)
 #ifdef CONFIG_PROC_PID_ARCH_STATUS
 /*
  * Report the amount of time elapsed in millisecond since last AVX512
- * use in the task.
+ * use in the task. Report -1 if no AVX-512 usage.
  */
 static void avx512_status(struct seq_file *m, struct task_struct *task)
 {
-	unsigned long timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
-	long delta;
+	unsigned long timestamp = 0;
+	long delta = -1;
 
-	if (!timestamp) {
-		/*
-		 * Report -1 if no AVX512 usage
-		 */
-		delta = -1;
-	} else {
+	/* Do not report AVX-512 usage for kernel threads */
+	if (!(task->flags & (PF_KTHREAD | PF_USER_WORKER)))
+		timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
+
+	if (timestamp) {
 		delta = (long)(jiffies - timestamp);
 		/*
 		 * Cap to LONG_MAX if time difference > LONG_MAX
-- 
2.43.0

[PATCH v2 2/2] x86/fpu: Update the debug flow for x86_task_fpu()

[Sohil Mehta]

Kernel threads aren't expected to directly access struct fpu using
x86_task_fpu(). They typically access the FPU state using pairs of
kernel_fpu_begin()/kernel_fpu_end().

When CONFIG_X86_DEBUG_FPU is set, any unintentional usage of
x86_task_fpu() by kernel threads is flagged with a WARN_ON_ONCE().
However, along with the warning, x86_task_fpu() returns a NULL pointer,
which deviates from the flow without the debug config.

Changing the return value could make failures harder to debug by masking
problems or introducing its own set of issues. Keep the behavior of
x86_task_fpu() consistent across debug and non-debug configurations
except for the warning.

Also, update the warning to include PF_USER_WORKER, as these tasks are
treated the same as kernel threads in the FPU context.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v2: New patch

This patch is less urgent than Patch 1 which fixes the real issue. I
haven't used a Fixes tag since this addresses a potential problem rather
than fixing an actual bug.
---
 arch/x86/kernel/fpu/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index ea138583dd92..ba16dda697b1 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -58,8 +58,7 @@ DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
 #ifdef CONFIG_X86_DEBUG_FPU
 struct fpu *x86_task_fpu(struct task_struct *task)
 {
-	if (WARN_ON_ONCE(task->flags & PF_KTHREAD))
-		return NULL;
+	WARN_ON_ONCE(task->flags & (PF_KTHREAD | PF_USER_WORKER));
 
 	return (void *)task + sizeof(*task);
 }
-- 
2.43.0

Re: [PATCH v2 2/2] x86/fpu: Update the debug flow for x86_task_fpu()

[Dave Hansen]

On 7/21/25 14:53, Sohil Mehta wrote:
> Also, update the warning to include PF_USER_WORKER, as these tasks are
> treated the same as kernel threads in the FPU context.

How so?

Re: [PATCH v2 2/2] x86/fpu: Update the debug flow for x86_task_fpu()

[Sohil Mehta]

On 7/21/2025 3:34 PM, Dave Hansen wrote:
> On 7/21/25 14:53, Sohil Mehta wrote:
>> Also, update the warning to include PF_USER_WORKER, as these tasks are
>> treated the same as kernel threads in the FPU context.
> 
> How so?

There are quite a few checks that special case PF_KTHREAD along with
PF_USER_WORKER. For example, FPSTATE allocation and freeing as well as
some context switch paths. Did I misinterpret something?

I believe it originates here:
https://lore.kernel.org/all/560c844c-f128-555b-40c6-31baff27537f@kernel.dk/

switch_fpu():
	if (!test_tsk_thread_flag(old, TIF_NEED_FPU_LOAD) &&
	    cpu_feature_enabled(X86_FEATURE_FPU) &&
	    !(old->flags & (PF_KTHREAD | PF_USER_WORKER))) {
		struct fpu *old_fpu = x86_task_fpu(old);


arch_release_task_struct():
	if (fpu_state_size_dynamic() && !(tsk->flags & (PF_KTHREAD |
PF_USER_WORKER)))
		fpstate_free(x86_task_fpu(tsk));


fpregs_restore_userregs():
if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
	return;

kernel_fpu_begin_mask():
	if (!(current->flags & (PF_KTHREAD | PF_USER_WORKER)) &&
	    !test_thread_flag(TIF_NEED_FPU_LOAD)) {
		set_thread_flag(TIF_NEED_FPU_LOAD);
		save_fpregs_to_fpstate(x86_task_fpu(current));
	}

Re: [PATCH v2 1/2] x86/fpu: Fix NULL dereference in avx512_status()

[Dave Hansen]

On 7/21/25 14:53, Sohil Mehta wrote:
> From: Fushuai Wang <wangfushuai@baidu.com>
> 
> When CONFIG_X86_DEBUG_FPU is set, reading /proc/[kthread]/arch_status
> causes a NULL pointer dereference.
> 
> For Kthreads tasks:
>   proc_pid_arch_status()
>     avx512_status()
>       x86_task_fpu() => returns NULL when CONFIG_X86_DEBUG_FPU=y
>       x86_task_fpu()->avx512_timestamp => NULL dereference

This seems to imply that CONFIG_X86_DEBUG_FPU _triggers_ the bug.

It certainly makes it obvious, but isn't there a bug with or without
CONFIG_X86_DEBUG_FPU? Even without it, I think it'll access out of the
init_task bounds.

> Kernel threads aren't expected to access FPU state directly. However,
> avx512_timestamp resides within struct fpu which lead to this unique
> situation.

What does this mean? Most kernel threads have a 'struct fpu', right?

> It is uncertain whether kernel threads use AVX-512 in a meaningful way
> that needs userspace reporting. For now, avoid reporting AVX-512 usage
> for kernel threads.

It would be idea if this was more explicit about how this changes the
ABI for kernel threads.

Could we make this more precise, please?

	Report "AVX512_elapsed_ms: -1" for kernel tasks, whether they
	use AVX-512 or not. This is the same value that is reported for
	user tasks which have not been detected using AVX-512.

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 9aa9ac8399ae..a75077c645b6 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1855,19 +1855,18 @@ long fpu_xstate_prctl(int option, unsigned long arg2)
>  #ifdef CONFIG_PROC_PID_ARCH_STATUS
>  /*
>   * Report the amount of time elapsed in millisecond since last AVX512
> - * use in the task.
> + * use in the task. Report -1 if no AVX-512 usage.
>   */
>  static void avx512_status(struct seq_file *m, struct task_struct *task)
>  {
> -	unsigned long timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
> -	long delta;
> +	unsigned long timestamp = 0;
> +	long delta = -1;
>  
> -	if (!timestamp) {
> -		/*
> -		 * Report -1 if no AVX512 usage
> -		 */
> -		delta = -1;
> -	} else {
> +	/* Do not report AVX-512 usage for kernel threads */
> +	if (!(task->flags & (PF_KTHREAD | PF_USER_WORKER)))
> +		timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
> +
> +	if (timestamp) {
>  		delta = (long)(jiffies - timestamp);
>  		/*
>  		 * Cap to LONG_MAX if time difference > LONG_MAX

Can we just do:

        unsigned long timestamp;
        long delta;

	if (task->flags & (PF_KTHREAD | PF_USER_WORKER))
		return;

	timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);

	...

for now, please? That way, there's no made up value for kernel threads.
The value just isn't present in the file.

Re: [PATCH v2 1/2] x86/fpu: Fix NULL dereference in avx512_status()

[Sohil Mehta]

On 7/21/2025 3:33 PM, Dave Hansen wrote:
> On 7/21/25 14:53, Sohil Mehta wrote:
>> From: Fushuai Wang <wangfushuai@baidu.com>
>>
>> When CONFIG_X86_DEBUG_FPU is set, reading /proc/[kthread]/arch_status
>> causes a NULL pointer dereference.
>>
>> For Kthreads tasks:
>>   proc_pid_arch_status()
>>     avx512_status()
>>       x86_task_fpu() => returns NULL when CONFIG_X86_DEBUG_FPU=y
>>       x86_task_fpu()->avx512_timestamp => NULL dereference
> 
> This seems to imply that CONFIG_X86_DEBUG_FPU _triggers_ the bug.
> 
> It certainly makes it obvious, but isn't there a bug with or without
> CONFIG_X86_DEBUG_FPU? Even without it, I think it'll access out of the
> init_task bounds.
> 

Yeah, there might be a bug without CONFIG_X86_DEBUG_FPU as well. I am
not sure. The NULL dereference only happens with CONFIG_X86_DEBUG_FPU
because it explicitly passes a NULL pointer.

Maybe we can just focus on the WARN_ON_ONCE(task->flags & PF_KTHREAD)
instead of the NULL dereference. For the init_task (PID 0) we never
follow this path since it doesn't show up in /proc.

>> Kernel threads aren't expected to access FPU state directly. However,
>> avx512_timestamp resides within struct fpu which lead to this unique
>> situation.
> 
> What does this mean? Most kernel threads have a 'struct fpu', right?
> 

I meant to say that even though kernel threads have a struct fpu, they
never access it directly using x86_task_fpu(). Kernel threads typically
use kernel_fpu_begin()/_end but reading the avx512_timestamp value is
the only unique reason, a pointer to struct fpu is needed.

>> It is uncertain whether kernel threads use AVX-512 in a meaningful way
>> that needs userspace reporting. For now, avoid reporting AVX-512 usage
>> for kernel threads.
> 
> It would be idea if this was more explicit about how this changes the
> ABI for kernel threads.
> 
> Could we make this more precise, please?
> 
> 	Report "AVX512_elapsed_ms: -1" for kernel tasks, whether they
> 	use AVX-512 or not. This is the same value that is reported for
> 	user tasks which have not been detected using AVX-512.
> 

Sure, will do.

>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 9aa9ac8399ae..a75077c645b6 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1855,19 +1855,18 @@ long fpu_xstate_prctl(int option, unsigned long arg2)
>>  #ifdef CONFIG_PROC_PID_ARCH_STATUS
>>  /*
>>   * Report the amount of time elapsed in millisecond since last AVX512
>> - * use in the task.
>> + * use in the task. Report -1 if no AVX-512 usage.
>>   */
>>  static void avx512_status(struct seq_file *m, struct task_struct *task)
>>  {
>> -	unsigned long timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
>> -	long delta;
>> +	unsigned long timestamp = 0;
>> +	long delta = -1;
>>  
>> -	if (!timestamp) {
>> -		/*
>> -		 * Report -1 if no AVX512 usage
>> -		 */
>> -		delta = -1;
>> -	} else {
>> +	/* Do not report AVX-512 usage for kernel threads */
>> +	if (!(task->flags & (PF_KTHREAD | PF_USER_WORKER)))
>> +		timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
>> +
>> +	if (timestamp) {
>>  		delta = (long)(jiffies - timestamp);
>>  		/*
>>  		 * Cap to LONG_MAX if time difference > LONG_MAX
> 
> Can we just do:
> 
>         unsigned long timestamp;
>         long delta;
> 
> 	if (task->flags & (PF_KTHREAD | PF_USER_WORKER))
> 		return;
> 

I considered this, but it seemed like a bigger ABI change than the one
proposed.

1) We are already reporting -1 as the AVX512_elapsed_ms for most, if not
all kernel threads. On an SPR system with Ubuntu, I couldn't find any
kernel thread that showed anything other than -1. (But I wasn't running
any workloads either.)

2) Even if there are a few kernel threads that have AVX-512 usage,
reporting -1 instead of a valid number would only lead to slightly
suboptimal scheduler decisions.
But reporting AVX512_elapsed_ms only for some threads might cause
userspace to break unexpectedly if it isn't written to handle that.


> 	timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
> 
> 	...
> 
> for now, please? That way, there's no made up value for kernel threads.
> The value just isn't present in the file.

I felt the current one is the lesser of the two evils. I am fine with
your approach if you still prefer it.

I have now realized that there are too many unknowns for me to make a
reliable call. This patch was a result of sunk-cost fallacy :)