Re: [PATCH] x86/fpu: Fix potential NULL dereference in avx512_status()

[Fushuai Wang <wangfushuai@baidu.com>]

>> Can you please share any other warnings that were triggered before this
>> Oops message? Also, I'll try to generate this locally. Any specific
>> configuration needed for reproducing this apart from CONFIG_X86_DEBUG_FPU?
>
>I was able to reproduce this on a system with X86_FEATURE_AVX512F. The
>issue only happens while reading arch_status on a kthread.
>
>$cat /proc/[kthread]/arch_status     => NULL pointer exception
>$cat /proc/[user thread]/arch_status => No issue seen
>
>Can you confirm that you are seeing the same behavior?

Confirmed, same issue here.

>Unfortunately, avx512_timestamp resides within struct fpu. So getting
>that value for a kthread would mean going through x86_task_fpu().
>
>I am wondering if we ever need to expose the AVX512 usage for kernel
>threads? If not, then we can do what you currently have but without the
>CONFIG_X86_DEBUG_FPU restriction. All kernel threads would always print
>the AVX512_elapsed_ms as -1.
>
>However, this would be a user visible change so we should probably get
>more inputs. I tried this experiment on an older kernel without the
>above issue. Among all the active kthreads on my system a handful of
>them show a valid value for AVX512 usage. The rest of them all show -1.
>
>PID: 2594
>CMD: avahi-daemon: running [SAP.local]
>  /proc/2594/arch_status content:
>AVX512_elapsed_ms:      46032
>
>PID: 2729
>CMD: sshd: /usr/sbin/sshd -D [listener] 0 of 10-100 startups
>  /proc/2729/arch_status content:
>AVX512_elapsed_ms:      396656
>
>To keep the older behavior, we might need to consider moving
>avx512_timestamp out of struct fpu. Though, I am uncertain about its
>implication.

I think avx512_elapsed_ms should logically belong in the FPU structure,
as it's a field inherently tied to FPU operations? To keep the older
behavior, we can set CONFIG_X86_DEBUG_FPU=n, maybe?
Is there a better approach to ensure kernel threads always correctly output
avx512_elapsed_ms. Directly get FPU struct pointer without using x86_task_fpu()?

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 9aa9ac8399ae..f989bc125e9b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1859,9 +1859,10 @@ long fpu_xstate_prctl(int option, unsigned long arg2)
  */
 static void avx512_status(struct seq_file *m, struct task_struct *task)
 {
-       unsigned long timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
+       unsigned long timestamp;
        long delta;

+       timestamp = READ_ONCE((struct fpu *)((void *)(task) + sizeof(*(task)))->avx512_timestamp);
        if (!timestamp) {
                /*
                 * Report -1 if no AVX512 usage