2024-03-20 13:19:47

by Ingo Molnar

Subject: [PATCH 0/1] Fast headers: Make task_struct::thread constant size

Turn thread.fpu into a pointer. Since most FPU code internals work by passing
around the FPU pointer already, the code generation impact is small.

This allows us to remove the old kludge of task_struct being variable size:

struct task_struct {

...
/*
* New fields for task_struct should be added above here, so that
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end

/* CPU-specific state of this task: */
struct thread_struct thread;

/*
* WARNING: on x86, 'thread_struct' contains a variable-sized
* structure. It *MUST* be at the end of 'task_struct'.
*
* Do not put anything below here!
*/
};

.. which creates a number of problems, such as requiring thread_struct to be
the last member of the struct - not allowing it to be struct-randomized, etc.

But the primary motivation is to allow the decoupling of task_struct from
hardware details (<asm/processor.h> in particular), and to eventually allow
the per-task infrastructure:

DECLARE_PER_TASK(type, name);
...
per_task(current, name) = val;

.. which requires task_struct to be a constant size struct.

The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
now that the FPU structure is not embedded in the task struct anymore, which
reduces text footprint a bit.

Ingo Molnar (1):
headers/deps: x86/fpu: Make task_struct::thread constant size

arch/x86/include/asm/fpu/sched.h | 2 +-
arch/x86/include/asm/processor.h | 14 ++++++--------
arch/x86/kernel/fpu/context.h | 4 ++--
arch/x86/kernel/fpu/core.c | 51 ++++++++++++++++++++++++++-------------------------
arch/x86/kernel/fpu/init.c | 28 ++++++++++++++++++----------
arch/x86/kernel/fpu/regset.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/signal.c | 18 +++++++++---------
arch/x86/kernel/fpu/xstate.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/xstate.h | 6 +++---
arch/x86/kernel/process.c | 6 +++---
arch/x86/kernel/signal.c | 6 +++---
arch/x86/kernel/traps.c | 2 +-
arch/x86/math-emu/fpu_aux.c | 2 +-
arch/x86/math-emu/fpu_entry.c | 4 ++--
arch/x86/math-emu/fpu_system.h | 2 +-
arch/x86/mm/extable.c | 2 +-
include/linux/sched.h | 13 +++----------
17 files changed, 102 insertions(+), 102 deletions(-)

--
2.40.1

2024-03-20 13:20:03

by Ingo Molnar

Subject: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

Turn thread.fpu into a pointer. Since most FPU code internals work by passing
around the FPU pointer already, the code generation impact is small.

This allows us to remove the old kludge of task_struct being variable size:

struct task_struct {

...
/*
* New fields for task_struct should be added above here, so that
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end

/* CPU-specific state of this task: */
struct thread_struct thread;

/*
* WARNING: on x86, 'thread_struct' contains a variable-sized
* structure. It *MUST* be at the end of 'task_struct'.
*
* Do not put anything below here!
*/
};

.. which creates a number of problems, such as requiring thread_struct to be
the last member of the struct - not allowing it to be struct-randomized, etc.

But the primary motivation is to allow the decoupling of task_struct from
hardware details (<asm/processor.h> in particular), and to eventually allow
the per-task infrastructure:

DECLARE_PER_TASK(type, name);
...
per_task(current, name) = val;

.. which requires task_struct to be a constant size struct.

The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
now that the FPU structure is not embedded in the task struct anymore, which
reduces text footprint a bit.

Signed-off-by: Ingo Molnar <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Uros Bizjak <[email protected]>
---
arch/x86/include/asm/fpu/sched.h | 2 +-
arch/x86/include/asm/processor.h | 14 ++++++--------
arch/x86/kernel/fpu/context.h | 4 ++--
arch/x86/kernel/fpu/core.c | 51 ++++++++++++++++++++++++++-------------------------
arch/x86/kernel/fpu/init.c | 28 ++++++++++++++++++----------
arch/x86/kernel/fpu/regset.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/signal.c | 18 +++++++++---------
arch/x86/kernel/fpu/xstate.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/xstate.h | 6 +++---
arch/x86/kernel/process.c | 6 +++---
arch/x86/kernel/signal.c | 6 +++---
arch/x86/kernel/traps.c | 2 +-
arch/x86/math-emu/fpu_aux.c | 2 +-
arch/x86/math-emu/fpu_entry.c | 4 ++--
arch/x86/math-emu/fpu_system.h | 2 +-
arch/x86/mm/extable.c | 2 +-
include/linux/sched.h | 13 +++----------
17 files changed, 102 insertions(+), 102 deletions(-)

diff --git a/arch/x86/include/asm/fpu/sched.h b/arch/x86/include/asm/fpu/sched.h
index c485f1944c5f..3cf20ab49b5f 100644
--- a/arch/x86/include/asm/fpu/sched.h
+++ b/arch/x86/include/asm/fpu/sched.h
@@ -41,7 +41,7 @@ static inline void switch_fpu_prepare(struct task_struct *old, int cpu)
{
if (cpu_feature_enabled(X86_FEATURE_FPU) &&
!(old->flags & (PF_KTHREAD | PF_USER_WORKER))) {
- struct fpu *old_fpu = &old->thread.fpu;
+ struct fpu *old_fpu = old->thread.fpu;

save_fpregs_to_fpstate(old_fpu);
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 811548f131f4..151880f2b079 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -491,19 +491,17 @@ struct thread_struct {
#endif

/* Floating point and extended processor state */
- struct fpu fpu;
- /*
- * WARNING: 'fpu' is dynamically-sized. It *MUST* be at
- * the end.
- */
+ struct fpu *fpu;
};

-extern void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size);
-
+/*
+ * X86 doesn't need any embedded-FPU-struct quirks:
+ */
static inline void arch_thread_struct_whitelist(unsigned long *offset,
unsigned long *size)
{
- fpu_thread_struct_whitelist(offset, size);
+ *offset = 0;
+ *size = 0;
}

static inline void
diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
index f6d856bd50bc..96d1f34179b3 100644
--- a/arch/x86/kernel/fpu/context.h
+++ b/arch/x86/kernel/fpu/context.h
@@ -53,7 +53,7 @@ static inline void fpregs_activate(struct fpu *fpu)
/* Internal helper for switch_fpu_return() and signal frame setup */
static inline void fpregs_restore_userregs(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
int cpu = smp_processor_id();

if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
@@ -67,7 +67,7 @@ static inline void fpregs_restore_userregs(void)
* If PKRU is enabled, then the PKRU value is already
* correct because it was either set in switch_to() or in
* flush_thread(). So it is excluded because it might be
- * not up to date in current->thread.fpu.xsave state.
+ * not up to date in current->thread.fpu->xsave state.
*
* XFD state is handled in restore_fpregs_from_fpstate().
*/
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 520deb411a70..93a45de0c67f 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -204,7 +204,7 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
return;

spin_lock_irq(&current->sighand->siglock);
- fpuperm = &current->group_leader->thread.fpu.guest_perm;
+ fpuperm = &current->group_leader->thread.fpu->guest_perm;
perm = fpuperm->__state_perm;

/* First fpstate allocation locks down permissions. */
@@ -316,7 +316,7 @@ EXPORT_SYMBOL_GPL(fpu_update_guest_xfd);
*/
void fpu_sync_guest_vmexit_xfd_state(void)
{
- struct fpstate *fps = current->thread.fpu.fpstate;
+ struct fpstate *fps = current->thread.fpu->fpstate;

lockdep_assert_irqs_disabled();
if (fpu_state_size_dynamic()) {
@@ -330,7 +330,7 @@ EXPORT_SYMBOL_GPL(fpu_sync_guest_vmexit_xfd_state);
int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
{
struct fpstate *guest_fps = guest_fpu->fpstate;
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
struct fpstate *cur_fps = fpu->fpstate;

fpregs_lock();
@@ -430,7 +430,7 @@ void kernel_fpu_begin_mask(unsigned int kfpu_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(&current->thread.fpu);
+ save_fpregs_to_fpstate(current->thread.fpu);
}
__cpu_invalidate_fpregs_state();

@@ -458,7 +458,7 @@ EXPORT_SYMBOL_GPL(kernel_fpu_end);
*/
void fpu_sync_fpstate(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != &current->thread.fpu);
+ WARN_ON_FPU(fpu != current->thread.fpu);

fpregs_lock();
trace_x86_fpu_before_save(fpu);
@@ -543,7 +543,7 @@ void fpstate_reset(struct fpu *fpu)
static inline void fpu_inherit_perms(struct fpu *dst_fpu)
{
if (fpu_state_size_dynamic()) {
- struct fpu *src_fpu = &current->group_leader->thread.fpu;
+ struct fpu *src_fpu = current->group_leader->thread.fpu;

spin_lock_irq(&current->sighand->siglock);
/* Fork also inherits the permissions of the parent */
@@ -563,7 +563,7 @@ static int update_fpu_shstk(struct task_struct *dst, unsigned long ssp)
if (!ssp)
return 0;

- xstate = get_xsave_addr(&dst->thread.fpu.fpstate->regs.xsave,
+ xstate = get_xsave_addr(&dst->thread.fpu->fpstate->regs.xsave,
XFEATURE_CET_USER);

/*
@@ -584,8 +584,19 @@ static int update_fpu_shstk(struct task_struct *dst, unsigned long ssp)
int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
unsigned long ssp)
{
- struct fpu *src_fpu = &current->thread.fpu;
- struct fpu *dst_fpu = &dst->thread.fpu;
+ /*
+ * We allocate the new FPU structure right after the end of the task struct.
+ * task allocation size already took this into account.
+ *
+ * This is safe because task_struct size is a multiple of cacheline size.
+ */
+ struct fpu *dst_fpu = (void *)dst + sizeof(*dst);
+ struct fpu *src_fpu = current->thread.fpu;
+
+ BUILD_BUG_ON(sizeof(*dst) % SMP_CACHE_BYTES != 0);
+ BUG_ON(!src_fpu);
+
+ dst->thread.fpu = dst_fpu;

/* The new task's FPU state cannot be valid in the hardware. */
dst_fpu->last_cpu = -1;
@@ -654,16 +665,6 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
return 0;
}

-/*
- * Whitelist the FPU register state embedded into task_struct for hardened
- * usercopy.
- */
-void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
-{
- *offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
- *size = fpu_kernel_cfg.default_size;
-}
-
/*
* Drops current FPU state: deactivates the fpregs and
* the fpstate. NOTE: it still leaves previous contents
@@ -677,7 +678,7 @@ void fpu__drop(struct fpu *fpu)
{
preempt_disable();

- if (fpu == &current->thread.fpu) {
+ if (fpu == current->thread.fpu) {
/* Ignore delayed exceptions from user space */
asm volatile("1: fwait\n"
"2:\n"
@@ -711,7 +712,7 @@ static inline void restore_fpregs_from_init_fpstate(u64 features_mask)
*/
static void fpu_reset_fpregs(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

fpregs_lock();
__fpu_invalidate_fpregs_state(fpu);
@@ -740,7 +741,7 @@ static void fpu_reset_fpregs(void)
*/
void fpu__clear_user_states(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != &current->thread.fpu);
+ WARN_ON_FPU(fpu != current->thread.fpu);

fpregs_lock();
if (!cpu_feature_enabled(X86_FEATURE_FPU)) {
@@ -773,7 +774,7 @@ void fpu__clear_user_states(struct fpu *fpu)

void fpu_flush_thread(void)
{
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
fpu_reset_fpregs();
}
/*
@@ -814,7 +815,7 @@ void fpregs_lock_and_load(void)
*/
void fpregs_assert_state_consistent(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

if (test_thread_flag(TIF_NEED_FPU_LOAD))
return;
@@ -826,7 +827,7 @@ EXPORT_SYMBOL_GPL(fpregs_assert_state_consistent);

void fpregs_mark_activate(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

fpregs_activate(fpu);
fpu->last_cpu = smp_processor_id();
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 998a08f17e33..f9412d012191 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
+ fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
else
#endif
asm volatile ("fninit");
@@ -71,8 +71,19 @@ static bool __init fpu__probe_without_cpuid(void)
return fsw == 0 && (fcw & 0x103f) == 0x003f;
}

+static struct fpu x86_init_fpu __read_mostly;
+
static void __init fpu__init_system_early_generic(void)
{
+ {
+ int this_cpu = smp_processor_id();
+
+ fpstate_reset(&x86_init_fpu);
+ current->thread.fpu = &x86_init_fpu;
+ per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
+ x86_init_fpu.last_cpu = this_cpu;
+ }
+
if (!boot_cpu_has(X86_FEATURE_CPUID) &&
!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
if (fpu__probe_without_cpuid())
@@ -150,11 +161,13 @@ static void __init fpu__init_task_struct_size(void)
{
int task_size = sizeof(struct task_struct);

+ task_size += sizeof(struct fpu);
+
/*
* Subtract off the static size of the register state.
* It potentially has a bunch of padding.
*/
- task_size -= sizeof(current->thread.fpu.__fpstate.regs);
+ task_size -= sizeof(current->thread.fpu->__fpstate.regs);

/*
* Add back the dynamically-calculated register state
@@ -164,14 +177,9 @@ static void __init fpu__init_task_struct_size(void)

/*
* We dynamically size 'struct fpu', so we require that
- * it be at the end of 'thread_struct' and that
- * 'thread_struct' be at the end of 'task_struct'. If
- * you hit a compile error here, check the structure to
- * see if something got added to the end.
+ * 'state' be at the end of 'it:
*/
CHECK_MEMBER_AT_END_OF(struct fpu, __fpstate);
- CHECK_MEMBER_AT_END_OF(struct thread_struct, fpu);
- CHECK_MEMBER_AT_END_OF(struct task_struct, thread);

arch_task_struct_size = task_size;
}
@@ -204,7 +212,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
fpu_kernel_cfg.default_size = size;
fpu_user_cfg.max_size = size;
fpu_user_cfg.default_size = size;
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
}

/*
@@ -213,8 +221,8 @@ static void __init fpu__init_system_xstate_size_legacy(void)
*/
void __init fpu__init_system(void)
{
- fpstate_reset(&current->thread.fpu);
fpu__init_system_early_generic();
+ fpstate_reset(current->thread.fpu);

/*
* The FPU has to be operational for some of the
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 6bc1eb2a21bd..38bc0b390d02 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -45,7 +45,7 @@ int regset_xregset_fpregs_active(struct task_struct *target, const struct user_r
*/
static void sync_fpstate(struct fpu *fpu)
{
- if (fpu == &current->thread.fpu)
+ if (fpu == current->thread.fpu)
fpu_sync_fpstate(fpu);
}

@@ -63,7 +63,7 @@ static void fpu_force_restore(struct fpu *fpu)
* Only stopped child tasks can be used to modify the FPU
* state in the fpstate buffer:
*/
- WARN_ON_FPU(fpu == &current->thread.fpu);
+ WARN_ON_FPU(fpu == current->thread.fpu);

__fpu_invalidate_fpregs_state(fpu);
}
@@ -71,7 +71,7 @@ static void fpu_force_restore(struct fpu *fpu)
int xfpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;

if (!cpu_feature_enabled(X86_FEATURE_FXSR))
return -ENODEV;
@@ -91,7 +91,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct fxregs_state newstate;
int ret;

@@ -133,7 +133,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
if (!cpu_feature_enabled(X86_FEATURE_XSAVE))
return -ENODEV;

- sync_fpstate(&target->thread.fpu);
+ sync_fpstate(target->thread.fpu);

copy_xstate_to_uabi_buf(to, target, XSTATE_COPY_XSAVE);
return 0;
@@ -143,7 +143,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct xregs_state *tmpbuf = NULL;
int ret;

@@ -187,7 +187,7 @@ int ssp_active(struct task_struct *target, const struct user_regset *regset)
int ssp_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct cet_user_state *cetregs;

if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
@@ -213,7 +213,7 @@ int ssp_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct xregs_state *xsave = &fpu->fpstate->regs.xsave;
struct cet_user_state *cetregs;
unsigned long user_ssp;
@@ -367,7 +367,7 @@ static void __convert_from_fxsr(struct user_i387_ia32_struct *env,
void
convert_from_fxsr(struct user_i387_ia32_struct *env, struct task_struct *tsk)
{
- __convert_from_fxsr(env, tsk, &tsk->thread.fpu.fpstate->regs.fxsave);
+ __convert_from_fxsr(env, tsk, &tsk->thread.fpu->fpstate->regs.fxsave);
}

void convert_to_fxsr(struct fxregs_state *fxsave,
@@ -400,7 +400,7 @@ void convert_to_fxsr(struct fxregs_state *fxsave,
int fpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct user_i387_ia32_struct env;
struct fxregs_state fxsave, *fx;

@@ -432,7 +432,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct user_i387_ia32_struct env;
int ret;

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 247f2225aa9f..6b262d0b8fc1 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -38,7 +38,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
/* Check for the first magic field and other error scenarios. */
if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
fx_sw->xstate_size < min_xstate_size ||
- fx_sw->xstate_size > current->thread.fpu.fpstate->user_size ||
+ fx_sw->xstate_size > current->thread.fpu->fpstate->user_size ||
fx_sw->xstate_size > fx_sw->extended_size)
goto setfx;

@@ -54,7 +54,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
if (likely(magic2 == FP_XSTATE_MAGIC2))
return true;
setfx:
- trace_x86_fpu_xstate_check_failed(&current->thread.fpu);
+ trace_x86_fpu_xstate_check_failed(current->thread.fpu);

/* Set the parameters for fx only state */
fx_sw->magic1 = 0;
@@ -69,13 +69,13 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
static inline bool save_fsave_header(struct task_struct *tsk, void __user *buf)
{
if (use_fxsr()) {
- struct xregs_state *xsave = &tsk->thread.fpu.fpstate->regs.xsave;
+ struct xregs_state *xsave = &tsk->thread.fpu->fpstate->regs.xsave;
struct user_i387_ia32_struct env;
struct _fpstate_32 __user *fp = buf;

fpregs_lock();
if (!test_thread_flag(TIF_NEED_FPU_LOAD))
- fxsave(&tsk->thread.fpu.fpstate->regs.fxsave);
+ fxsave(&tsk->thread.fpu->fpstate->regs.fxsave);
fpregs_unlock();

convert_from_fxsr(&env, tsk);
@@ -188,7 +188,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
{
struct task_struct *tsk = current;
- struct fpstate *fpstate = tsk->thread.fpu.fpstate;
+ struct fpstate *fpstate = tsk->thread.fpu->fpstate;
bool ia32_fxstate = (buf != buf_fx);
int ret;

@@ -276,7 +276,7 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
*/
static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
int ret;

/* Restore enabled features only. */
@@ -336,7 +336,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
bool ia32_fxstate)
{
struct task_struct *tsk = current;
- struct fpu *fpu = &tsk->thread.fpu;
+ struct fpu *fpu = tsk->thread.fpu;
struct user_i387_ia32_struct env;
bool success, fx_only = false;
union fpregs_state *fpregs;
@@ -456,7 +456,7 @@ static inline unsigned int xstate_sigframe_size(struct fpstate *fpstate)
*/
bool fpu__restore_sig(void __user *buf, int ia32_frame)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
void __user *buf_fx = buf;
bool ia32_fxstate = false;
bool success = false;
@@ -503,7 +503,7 @@ unsigned long
fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
unsigned long *buf_fx, unsigned long *size)
{
- unsigned long frame_size = xstate_sigframe_size(current->thread.fpu.fpstate);
+ unsigned long frame_size = xstate_sigframe_size(current->thread.fpu->fpstate);

*buf_fx = sp = round_down(sp - frame_size, 64);
if (ia32_frame && use_fxsr()) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 117e74c44e75..df442b2df55b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -734,7 +734,7 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
*/
init_fpstate.xfd = 0;

- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
}

/*
@@ -844,7 +844,7 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
goto out_disable;

/* Reset the state for the current task */
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);

/*
* Update info used for ptrace frames; use standard-format size and no
@@ -918,7 +918,7 @@ void fpu__resume_cpu(void)
}

if (fpu_state_size_dynamic())
- wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
+ wrmsrl(MSR_IA32_XFD, current->thread.fpu->fpstate->xfd);
}

/*
@@ -1186,8 +1186,8 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
void copy_xstate_to_uabi_buf(struct membuf to, struct task_struct *tsk,
enum xstate_copy_mode copy_mode)
{
- __copy_xstate_to_uabi_buf(to, tsk->thread.fpu.fpstate,
- tsk->thread.fpu.fpstate->user_xfeatures,
+ __copy_xstate_to_uabi_buf(to, tsk->thread.fpu->fpstate,
+ tsk->thread.fpu->fpstate->user_xfeatures,
tsk->thread.pkru, copy_mode);
}

@@ -1327,7 +1327,7 @@ int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf, u
int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
const void __user *ubuf)
{
- return copy_uabi_to_xstate(tsk->thread.fpu.fpstate, NULL, ubuf, &tsk->thread.pkru);
+ return copy_uabi_to_xstate(tsk->thread.fpu->fpstate, NULL, ubuf, &tsk->thread.pkru);
}

static bool validate_independent_components(u64 mask)
@@ -1421,7 +1421,7 @@ static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor)
* The XFD MSR does not match fpstate->xfd. That's invalid when
* the passed in fpstate is current's fpstate.
*/
- if (fpstate->xfd == current->thread.fpu.fpstate->xfd)
+ if (fpstate->xfd == current->thread.fpu->fpstate->xfd)
return false;

/*
@@ -1498,7 +1498,7 @@ void fpstate_free(struct fpu *fpu)
static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
unsigned int usize, struct fpu_guest *guest_fpu)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
struct fpstate *curfps, *newfps = NULL;
unsigned int fpsize;
bool in_use;
@@ -1591,7 +1591,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
* AVX512.
*/
bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
- struct fpu *fpu = &current->group_leader->thread.fpu;
+ struct fpu *fpu = current->group_leader->thread.fpu;
struct fpu_state_perm *perm;
unsigned int ksize, usize;
u64 mask;
@@ -1694,7 +1694,7 @@ int __xfd_enable_feature(u64 xfd_err, struct fpu_guest *guest_fpu)
return -EPERM;
}

- fpu = &current->group_leader->thread.fpu;
+ fpu = current->group_leader->thread.fpu;
perm = guest_fpu ? &fpu->guest_perm : &fpu->perm;
ksize = perm->__state_size;
usize = perm->__user_state_size;
@@ -1799,7 +1799,7 @@ 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(task->thread.fpu.avx512_timestamp);
+ unsigned long timestamp = READ_ONCE(task->thread.fpu->avx512_timestamp);
long delta;

if (!timestamp) {
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 3518fb26d06b..aca8a89bea84 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -22,7 +22,7 @@ static inline void xstate_init_xcomp_bv(struct xregs_state *xsave, u64 mask)

static inline u64 xstate_get_group_perm(bool guest)
{
- struct fpu *fpu = &current->group_leader->thread.fpu;
+ struct fpu *fpu = current->group_leader->thread.fpu;
struct fpu_state_perm *perm;

/* Pairs with WRITE_ONCE() in xstate_request_perm() */
@@ -261,7 +261,7 @@ static inline int xsave_to_user_sigframe(struct xregs_state __user *buf)
* internally, e.g. PKRU. That's user space ABI and also required
* to allow the signal handler to modify PKRU.
*/
- struct fpstate *fpstate = current->thread.fpu.fpstate;
+ struct fpstate *fpstate = current->thread.fpu->fpstate;
u64 mask = fpstate->user_xfeatures;
u32 lmask;
u32 hmask;
@@ -292,7 +292,7 @@ static inline int xrstor_from_user_sigframe(struct xregs_state __user *buf, u64
u32 hmask = mask >> 32;
int err;

- xfd_validate_state(current->thread.fpu.fpstate, mask, true);
+ xfd_validate_state(current->thread.fpu->fpstate, mask, true);

stac();
XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b8441147eb5e..5f3f48713870 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -97,7 +97,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
dst->thread.vm86 = NULL;
#endif
/* Drop the copied pointer to current's fpstate */
- dst->thread.fpu.fpstate = NULL;
+ dst->thread.fpu = NULL;

return 0;
}
@@ -106,7 +106,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
void arch_release_task_struct(struct task_struct *tsk)
{
if (fpu_state_size_dynamic())
- fpstate_free(&tsk->thread.fpu);
+ fpstate_free(tsk->thread.fpu);
}
#endif

@@ -116,7 +116,7 @@ void arch_release_task_struct(struct task_struct *tsk)
void exit_thread(struct task_struct *tsk)
{
struct thread_struct *t = &tsk->thread;
- struct fpu *fpu = &t->fpu;
+ struct fpu *fpu = t->fpu;

if (test_thread_flag(TIF_IO_BITMAP))
io_bitmap_exit(tsk);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 31b6f5dddfc2..7e709ae8e99a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -228,7 +228,7 @@ static void
handle_signal(struct ksignal *ksig, struct pt_regs *regs)
{
bool stepping, failed;
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

if (v8086_mode(regs))
save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
@@ -396,14 +396,14 @@ bool sigaltstack_size_valid(size_t ss_size)
if (!fpu_state_size_dynamic() && !strict_sigaltstack_size)
return true;

- fsize += current->group_leader->thread.fpu.perm.__user_state_size;
+ fsize += current->group_leader->thread.fpu->perm.__user_state_size;
if (likely(ss_size > fsize))
return true;

if (strict_sigaltstack_size)
return ss_size > fsize;

- mask = current->group_leader->thread.fpu.perm.__state_perm;
+ mask = current->group_leader->thread.fpu->perm.__state_perm;
if (mask & XFEATURE_MASK_USER_DYNAMIC)
return ss_size > fsize;

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17e5043..c79e36c4071b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1148,7 +1148,7 @@ DEFINE_IDTENTRY_RAW(exc_debug)
static void math_error(struct pt_regs *regs, int trapnr)
{
struct task_struct *task = current;
- struct fpu *fpu = &task->thread.fpu;
+ struct fpu *fpu = task->thread.fpu;
int si_code;
char *str = (trapnr == X86_TRAP_MF) ? "fpu exception" :
"simd exception";
diff --git a/arch/x86/math-emu/fpu_aux.c b/arch/x86/math-emu/fpu_aux.c
index d62662bdd460..8087953164fc 100644
--- a/arch/x86/math-emu/fpu_aux.c
+++ b/arch/x86/math-emu/fpu_aux.c
@@ -53,7 +53,7 @@ void fpstate_init_soft(struct swregs_state *soft)

void finit(void)
{
- fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
+ fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
}

/*
diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index 91c52ead1226..30400d95d9d0 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -641,7 +641,7 @@ int fpregs_soft_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct swregs_state *s387 = &target->thread.fpu.fpstate->regs.soft;
+ struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
void *space = s387->st_space;
int ret;
int offset, other, i, tags, regnr, tag, newtop;
@@ -692,7 +692,7 @@ int fpregs_soft_get(struct task_struct *target,
const struct user_regset *regset,
struct membuf to)
{
- struct swregs_state *s387 = &target->thread.fpu.fpstate->regs.soft;
+ struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
const void *space = s387->st_space;
int offset = (S387->ftop & 7) * 10, other = 80 - offset;

diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index eec3e4805c75..3417337e7d99 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -73,7 +73,7 @@ static inline bool seg_writable(struct desc_struct *d)
return (d->type & SEG_TYPE_EXECUTE_MASK) == SEG_TYPE_WRITABLE;
}

-#define I387 (&current->thread.fpu.fpstate->regs)
+#define I387 (&current->thread.fpu->fpstate->regs)
#define FPU_info (I387->soft.info)

#define FPU_CS (*(unsigned short *) &(FPU_info->regs->cs))
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index b522933bfa56..cd73a533db69 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -111,7 +111,7 @@ static bool ex_handler_sgx(const struct exception_table_entry *fixup,

/*
* Handler for when we fail to restore a task's FPU state. We should never get
- * here because the FPU state of a task using the FPU (task->thread.fpu.state)
+ * here because the FPU state of a task using the FPU (task->thread.fpu->state)
* should always be valid. However, past bugs have allowed userspace to set
* reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn().
* These caused XRSTOR to fail when switching to the task, leaking the FPU
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3c2abbc587b4..069d262730b4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1550,21 +1550,14 @@ struct task_struct {
struct user_event_mm *user_event_mm;
#endif

+ /* CPU-specific state of this task: */
+ struct thread_struct thread;
+
/*
* New fields for task_struct should be added above here, so that
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end
-
- /* CPU-specific state of this task: */
- struct thread_struct thread;
-
- /*
- * WARNING: on x86, 'thread_struct' contains a variable-sized
- * structure. It *MUST* be at the end of 'task_struct'.
- *
- * Do not put anything below here!
- */
};

#define TASK_REPORT_IDLE (TASK_REPORT + 1)
--
2.40.1

2024-03-25 16:46:42

by kernel test robot

Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

Hello,

kernel test robot noticed "WARNING:at_arch/x86/mm/extable.c:#fixup_exception" on:

commit: 9738bc8321cc5abbab8676f6c1eb1b8c7c3d172d ("[PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size")
url: https://github.com/intel-lab-lkp/linux/commits/Ingo-Molnar/headers-deps-x86-fpu-Make-task_struct-thread-constant-size/20240320-212236
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 35ce64922c8263448e58a2b9e8d15a64e11e9b2d
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

in testcase: boot

compiler: gcc-12
test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+---------------------------------------------------------------------------+------------+------------+
| | 35ce64922c | 9738bc8321 |
+---------------------------------------------------------------------------+------------+------------+
| WARNING:at_arch/x86/mm/extable.c:#fixup_exception | 0 | 17 |
| EIP:fixup_exception | 0 | 17 |
| EIP:restore_fpregs_from_fpstate | 0 | 17 |
| WARNING:at_arch/x86/kernel/fpu/xstate.h:#os_xsave | 0 | 17 |
| EIP:os_xsave | 0 | 17 |
| kernel_BUG_at_mm/usercopy.c | 0 | 17 |
| invalid_opcode:#[##] | 0 | 17 |
| EIP:usercopy_abort | 0 | 17 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 17 |
+---------------------------------------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-lkp/[email protected]


[ 7.844011][ T58] ------------[ cut here ]------------
[ 7.844665][ T58] Bad FPU state detected at restore_fpregs_from_fpstate+0x3d/0x88, reinitializing FPU registers.
[ 7.844686][ T58] WARNING: CPU: 0 PID: 58 at arch/x86/mm/extable.c:126 fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] Modules linked in:
[ 7.846556][ T58] CPU: 0 PID: 58 Comm: modprobe Tainted: G W 6.8.0-rc4-00056-g9738bc8321cc #1
[ 7.846556][ T58] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 7.846556][ T58] EIP: fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] Code: 88 0d a4 50 22 ce e8 9b 07 01 00 0f 0b e9 a8 fe ff ff 89 44 24 04 b2 01 c7 04 24 68 29 a9 cd 88 15 a6 50 22 ce e8 7c 07 01 00 <0f> 0b e9 c0 fe ff ff 0f 0b ba 88 f8 38 ce e9 e9 fe ff ff 8d 74 26
All code
========
0: 88 0d a4 50 22 ce mov %cl,-0x31ddaf5c(%rip) # 0xffffffffce2250aa
6: e8 9b 07 01 00 call 0x107a6
b: 0f 0b ud2
d: e9 a8 fe ff ff jmp 0xfffffffffffffeba
12: 89 44 24 04 mov %eax,0x4(%rsp)
16: b2 01 mov $0x1,%dl
18: c7 04 24 68 29 a9 cd movl $0xcda92968,(%rsp)
1f: 88 15 a6 50 22 ce mov %dl,-0x31ddaf5a(%rip) # 0xffffffffce2250cb
25: e8 7c 07 01 00 call 0x107a6
2a:* 0f 0b ud2 <-- trapping instruction
2c: e9 c0 fe ff ff jmp 0xfffffffffffffef1
31: 0f 0b ud2
33: ba 88 f8 38 ce mov $0xce38f888,%edx
38: e9 e9 fe ff ff jmp 0xffffffffffffff26
3d: 8d .byte 0x8d
3e: 74 26 je 0x66

Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: e9 c0 fe ff ff jmp 0xfffffffffffffec7
7: 0f 0b ud2
9: ba 88 f8 38 ce mov $0xce38f888,%edx
e: e9 e9 fe ff ff jmp 0xfffffffffffffefc
13: 8d .byte 0x8d
14: 74 26 je 0x3c
[ 7.846556][ T58] EAX: 00000000 EBX: cdc38510 ECX: 00000000 EDX: 00000000
[ 7.846556][ T58] ESI: ea1abef0 EDI: 0000000d EBP: ea1abe5c ESP: ea1abde0
[ 7.846556][ T58] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046
[ 7.846556][ T58] CR0: 80050033 CR2: bfda18db CR3: 29d1d000 CR4: 00040690
[ 7.846556][ T58] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 7.846556][ T58] DR6: fffe0ff0 DR7: 00000400
[ 7.846556][ T58] Call Trace:
[ 7.846556][ T58] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[ 7.846556][ T58] ? fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] ? __warn (kernel/panic.c:677)
[ 7.846556][ T58] ? fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] ? report_bug (lib/bug.c:180 lib/bug.c:219)
[ 7.846556][ T58] ? exc_overflow (arch/x86/kernel/traps.c:251)
[ 7.846556][ T58] ? handle_bug (arch/x86/kernel/traps.c:238)
[ 7.846556][ T58] ? exc_invalid_op (arch/x86/kernel/traps.c:259 (discriminator 1))
[ 7.846556][ T58] ? handle_exception (arch/x86/entry/entry_32.S:1049)
[ 7.846556][ T58] ? rt_mutex_debug_task_free (kernel/locking/rtmutex_api.c:487 (discriminator 11))
[ 7.846556][ T58] ? set_memory_global (arch/x86/mm/pat/set_memory.c:2298)
[ 7.846556][ T58] ? exc_overflow (arch/x86/kernel/traps.c:251)
[ 7.846556][ T58] ? fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] ? set_memory_global (arch/x86/mm/pat/set_memory.c:2298)
[ 7.846556][ T58] ? exc_overflow (arch/x86/kernel/traps.c:251)
[ 7.846556][ T58] ? fixup_exception (arch/x86/mm/extable.c:126 arch/x86/mm/extable.c:275)
[ 7.846556][ T58] ? restore_fpregs_from_fpstate (arch/x86/kernel/fpu/core.c:188)
[ 7.846556][ T58] ? __lock_acquire (kernel/locking/lockdep.c:5137)
[ 7.846556][ T58] ? __delete_object (mm/kmemleak.c:798 (discriminator 3))
[ 7.846556][ T58] ? kvm_sched_clock_read (arch/x86/kernel/kvmclock.c:91)
[ 7.846556][ T58] ? sched_clock_noinstr (arch/x86/kernel/tsc.c:267)
[ 7.846556][ T58] ? local_clock_noinstr (kernel/sched/clock.c:282 kernel/sched/clock.c:306)
[ 7.846556][ T58] ? exc_bounds (arch/x86/kernel/traps.c:643)
[ 7.846556][ T58] exc_general_protection (arch/x86/kernel/traps.c:617 arch/x86/kernel/traps.c:677 arch/x86/kernel/traps.c:643)
[ 7.846556][ T58] ? __delete_object (mm/kmemleak.c:798 (discriminator 3))
[ 7.846556][ T58] ? exc_bounds (arch/x86/kernel/traps.c:643)
[ 7.846556][ T58] handle_exception (arch/x86/entry/entry_32.S:1049)
[ 7.846556][ T58] EIP: restore_fpregs_from_fpstate (arch/x86/kernel/fpu/core.c:188)
[ 7.846556][ T58] Code: eb 0a cc cc cc db e2 0f 77 db 45 f4 3e 8d 74 26 00 8b 3d ec f1 bc cd 8b 4d f4 8b 1d e8 f1 bc cd 21 fa 8d 79 40 21 d8 0f ae 2f <8b> 5d f8 8b 7d fc 89 ec 5d 31 c0 31 d2 31 c9 c3 8d 76 00 3e 8d 74
All code
========
0: eb 0a jmp 0xc
2: cc int3
3: cc int3
4: cc int3
5: db e2 fnclex
7: 0f 77 emms
9: db 45 f4 fildl -0xc(%rbp)
c: 3e 8d 74 26 00 ds lea 0x0(%rsi,%riz,1),%esi
11: 8b 3d ec f1 bc cd mov -0x32430e14(%rip),%edi # 0xffffffffcdbcf203
17: 8b 4d f4 mov -0xc(%rbp),%ecx
1a: 8b 1d e8 f1 bc cd mov -0x32430e18(%rip),%ebx # 0xffffffffcdbcf208
20: 21 fa and %edi,%edx
22: 8d 79 40 lea 0x40(%rcx),%edi
25: 21 d8 and %ebx,%eax
27: 0f ae 2f xrstor (%rdi)
2a:* 8b 5d f8 mov -0x8(%rbp),%ebx <-- trapping instruction
2d: 8b 7d fc mov -0x4(%rbp),%edi
30: 89 ec mov %ebp,%esp
32: 5d pop %rbp
33: 31 c0 xor %eax,%eax
35: 31 d2 xor %edx,%edx
37: 31 c9 xor %ecx,%ecx
39: c3 ret
3a: 8d 76 00 lea 0x0(%rsi),%esi
3d: 3e ds
3e: 8d .byte 0x8d
3f: 74 .byte 0x74

Code starting with the faulting instruction
===========================================
0: 8b 5d f8 mov -0x8(%rbp),%ebx
3: 8b 7d fc mov -0x4(%rbp),%edi
6: 89 ec mov %ebp,%esp
8: 5d pop %rbp
9: 31 c0 xor %eax,%eax
b: 31 d2 xor %edx,%edx
d: 31 c9 xor %ecx,%ecx
f: c3 ret
10: 8d 76 00 lea 0x0(%rsi),%esi
13: 3e ds
14: 8d .byte 0x8d
15: 74 .byte 0x74
[ 7.846556][ T58] EAX: 00000007 EBX: 00000007 ECX: c2d836e0 EDX: 00000000
[ 7.846556][ T58] ESI: c2d836a0 EDI: c2d83720 EBP: ea1abf58 ESP: ea1abf4c
[ 7.846556][ T58] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010002
[ 7.846556][ T58] ? exc_bounds (arch/x86/kernel/traps.c:643)
[ 7.846556][ T58] ? restore_fpregs_from_fpstate (arch/x86/kernel/fpu/core.c:179 arch/x86/kernel/fpu/core.c:181)
[ 7.846556][ T58] switch_fpu_return (arch/x86/include/asm/atomic.h:23 include/linux/atomic/atomic-arch-fallback.h:457 include/linux/jump_label.h:260 include/linux/jump_label.h:270 arch/x86/include/asm/trace/fpu.h:57 arch/x86/kernel/fpu/context.h:50 arch/x86/kernel/fpu/context.h:76 arch/x86/kernel/fpu/core.c:788)
[ 7.846556][ T58] syscall_exit_to_user_mode (arch/x86/include/asm/entry-common.h:58 include/linux/entry-common.h:330 kernel/entry/common.c:201 kernel/entry/common.c:212)
[ 7.846556][ T58] ? call_usermodehelper_exec_async (kernel/umh.c:114)
[ 7.846556][ T58] ? call_usermodehelper (kernel/umh.c:65)
[ 7.846556][ T58] ret_from_fork (arch/x86/kernel/process.c:157)
[ 7.846556][ T58] ? call_usermodehelper (kernel/umh.c:65)
[ 7.846556][ T58] ret_from_fork_asm (arch/x86/entry/entry_32.S:741)
[ 7.846556][ T58] entry_INT80_32 (arch/x86/entry/entry_32.S:947)
[ 7.846556][ T58] EIP: 0xb7ee70b0
[ 7.846556][ T58] Code: Unable to access opcode bytes at 0xb7ee7086.

Code starting with the faulting instruction
===========================================


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240325/[email protected]



--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-03-26 17:51:22

by Oleg Nesterov

Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

On 03/20, Ingo Molnar wrote:
>
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
> /* Flush out any pending x87 state: */
> #ifdef CONFIG_MATH_EMULATION
> if (!boot_cpu_has(X86_FEATURE_FPU))
> - fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
> + fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Typo? it should be

&current->thread.fpu->fpstate->regs.soft

> +static struct fpu x86_init_fpu __read_mostly;
> +
> static void __init fpu__init_system_early_generic(void)
> {
> + {
> + int this_cpu = smp_processor_id();
> +
> + fpstate_reset(&x86_init_fpu);
> + current->thread.fpu = &x86_init_fpu;
> + per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> + x86_init_fpu.last_cpu = this_cpu;
> + }

Can't x86_init_fpu be declared inside the block above?

> void __init fpu__init_system(void)
> {
> - fpstate_reset(&current->thread.fpu);
> fpu__init_system_early_generic();
> + fpstate_reset(current->thread.fpu);

It seems that fpstate_reset(current->thread.fpu) is not needed after the
change in fpu__init_system_early_generic() above.

Oleg.

2024-03-26 19:12:48

by Ingo Molnar

Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

* Oleg Nesterov <[email protected]> wrote:

> On 03/20, Ingo Molnar wrote:
> >
> > --- a/arch/x86/kernel/fpu/init.c
> > +++ b/arch/x86/kernel/fpu/init.c
> > @@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
> > /* Flush out any pending x87 state: */
> > #ifdef CONFIG_MATH_EMULATION
> > if (!boot_cpu_has(X86_FEATURE_FPU))
> > - fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
> > + fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Typo? it should be
>
> &current->thread.fpu->fpstate->regs.soft

Thx, fixed.

>
> > +static struct fpu x86_init_fpu __read_mostly;
> > +
> > static void __init fpu__init_system_early_generic(void)
> > {
> > + {
> > + int this_cpu = smp_processor_id();
> > +
> > + fpstate_reset(&x86_init_fpu);
> > + current->thread.fpu = &x86_init_fpu;
> > + per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> > + x86_init_fpu.last_cpu = this_cpu;
> > + }
>
> Can't x86_init_fpu be declared inside the block above?

As a function-local static? I think globals are better defined in a visible
fashion, not hidden among local variables where they are easy to overlook.

But the extra block is unnecessary in any case.

> > void __init fpu__init_system(void)
> > {
> > - fpstate_reset(&current->thread.fpu);
> > fpu__init_system_early_generic();
> > + fpstate_reset(current->thread.fpu);
>
> It seems that fpstate_reset(current->thread.fpu) is not needed after the
> change in fpu__init_system_early_generic() above.

Yeah. Something like the delta patch below?

Thanks,

Ingo

=================>

arch/x86/kernel/fpu/init.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index f9412d012191..de618ec509aa 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(current->thread.fpu->fpstate->regs.soft);
+ fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
else
#endif
asm volatile ("fninit");
@@ -75,14 +75,12 @@ static struct fpu x86_init_fpu __read_mostly;

static void __init fpu__init_system_early_generic(void)
{
- {
- int this_cpu = smp_processor_id();
+ int this_cpu = smp_processor_id();

- fpstate_reset(&x86_init_fpu);
- current->thread.fpu = &x86_init_fpu;
- per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
- x86_init_fpu.last_cpu = this_cpu;
- }
+ fpstate_reset(&x86_init_fpu);
+ current->thread.fpu = &x86_init_fpu;
+ per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
+ x86_init_fpu.last_cpu = this_cpu;

if (!boot_cpu_has(X86_FEATURE_CPUID) &&
!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
@@ -222,7 +220,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
void __init fpu__init_system(void)
{
fpu__init_system_early_generic();
- fpstate_reset(current->thread.fpu);

/*
* The FPU has to be operational for some of the

2024-03-26 19:16:20

by Ingo Molnar

Subject: [PATCH 1/1 -v2] headers/deps: x86/fpu: Make task_struct::thread constant size

* Ingo Molnar <[email protected]> wrote:

> > > void __init fpu__init_system(void)
> > > {
> > > - fpstate_reset(&current->thread.fpu);
> > > fpu__init_system_early_generic();
> > > + fpstate_reset(current->thread.fpu);
> >
> > It seems that fpstate_reset(current->thread.fpu) is not needed after the
> > change in fpu__init_system_early_generic() above.
>
> Yeah. Something like the delta patch below?

Full -v2 patch also attached, for reference.

Thanks,

Ingo

============>
From: Ingo Molnar <[email protected]>
Date: Wed, 20 Mar 2024 13:47:13 +0100
Subject: [PATCH] headers/deps: x86/fpu: Make task_struct::thread constant size

Turn thread.fpu into a pointer. Since most FPU code internals work by passing
around the FPU pointer already, the code generation impact is small.

This allows us to remove the old kludge of task_struct being variable size:

struct task_struct {

...
/*
* New fields for task_struct should be added above here, so that
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end

/* CPU-specific state of this task: */
struct thread_struct thread;

/*
* WARNING: on x86, 'thread_struct' contains a variable-sized
* structure. It *MUST* be at the end of 'task_struct'.
*
* Do not put anything below here!
*/
};

.. which creates a number of problems, such as requiring thread_struct to be
the last member of the struct - not allowing it to be struct-randomized, etc.

But the primary motivation is to allow the decoupling of task_struct from
hardware details (<asm/processor.h> in particular), and to eventually allow
the per-task infrastructure:

DECLARE_PER_TASK(type, name);
...
per_task(current, name) = val;

.. which requires task_struct to be a constant size struct.

The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
now that the FPU structure is not embedded in the task struct anymore, which
reduces text footprint a bit.

Signed-off-by: Ingo Molnar <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Uros Bizjak <[email protected]>
---
arch/x86/include/asm/fpu/sched.h | 2 +-
arch/x86/include/asm/processor.h | 14 +++++------
arch/x86/kernel/fpu/context.h | 4 ++--
arch/x86/kernel/fpu/core.c | 51 ++++++++++++++++++++--------------------
arch/x86/kernel/fpu/init.c | 25 ++++++++++++--------
arch/x86/kernel/fpu/regset.c | 22 ++++++++---------
arch/x86/kernel/fpu/signal.c | 18 +++++++-------
arch/x86/kernel/fpu/xstate.c | 22 ++++++++---------
arch/x86/kernel/fpu/xstate.h | 6 ++---
arch/x86/kernel/process.c | 6 ++---
arch/x86/kernel/signal.c | 6 ++---
arch/x86/kernel/traps.c | 2 +-
arch/x86/math-emu/fpu_aux.c | 2 +-
arch/x86/math-emu/fpu_entry.c | 4 ++--
arch/x86/math-emu/fpu_system.h | 2 +-
arch/x86/mm/extable.c | 2 +-
include/linux/sched.h | 13 +++-------
17 files changed, 99 insertions(+), 102 deletions(-)

diff --git a/arch/x86/include/asm/fpu/sched.h b/arch/x86/include/asm/fpu/sched.h
index c485f1944c5f..3cf20ab49b5f 100644
--- a/arch/x86/include/asm/fpu/sched.h
+++ b/arch/x86/include/asm/fpu/sched.h
@@ -41,7 +41,7 @@ static inline void switch_fpu_prepare(struct task_struct *old, int cpu)
{
if (cpu_feature_enabled(X86_FEATURE_FPU) &&
!(old->flags & (PF_KTHREAD | PF_USER_WORKER))) {
- struct fpu *old_fpu = &old->thread.fpu;
+ struct fpu *old_fpu = old->thread.fpu;

save_fpregs_to_fpstate(old_fpu);
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 811548f131f4..151880f2b079 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -491,19 +491,17 @@ struct thread_struct {
#endif

/* Floating point and extended processor state */
- struct fpu fpu;
- /*
- * WARNING: 'fpu' is dynamically-sized. It *MUST* be at
- * the end.
- */
+ struct fpu *fpu;
};

-extern void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size);
-
+/*
+ * X86 doesn't need any embedded-FPU-struct quirks:
+ */
static inline void arch_thread_struct_whitelist(unsigned long *offset,
unsigned long *size)
{
- fpu_thread_struct_whitelist(offset, size);
+ *offset = 0;
+ *size = 0;
}

static inline void
diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
index f6d856bd50bc..96d1f34179b3 100644
--- a/arch/x86/kernel/fpu/context.h
+++ b/arch/x86/kernel/fpu/context.h
@@ -53,7 +53,7 @@ static inline void fpregs_activate(struct fpu *fpu)
/* Internal helper for switch_fpu_return() and signal frame setup */
static inline void fpregs_restore_userregs(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
int cpu = smp_processor_id();

if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
@@ -67,7 +67,7 @@ static inline void fpregs_restore_userregs(void)
* If PKRU is enabled, then the PKRU value is already
* correct because it was either set in switch_to() or in
* flush_thread(). So it is excluded because it might be
- * not up to date in current->thread.fpu.xsave state.
+ * not up to date in current->thread.fpu->xsave state.
*
* XFD state is handled in restore_fpregs_from_fpstate().
*/
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 520deb411a70..93a45de0c67f 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -204,7 +204,7 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
return;

spin_lock_irq(&current->sighand->siglock);
- fpuperm = &current->group_leader->thread.fpu.guest_perm;
+ fpuperm = &current->group_leader->thread.fpu->guest_perm;
perm = fpuperm->__state_perm;

/* First fpstate allocation locks down permissions. */
@@ -316,7 +316,7 @@ EXPORT_SYMBOL_GPL(fpu_update_guest_xfd);
*/
void fpu_sync_guest_vmexit_xfd_state(void)
{
- struct fpstate *fps = current->thread.fpu.fpstate;
+ struct fpstate *fps = current->thread.fpu->fpstate;

lockdep_assert_irqs_disabled();
if (fpu_state_size_dynamic()) {
@@ -330,7 +330,7 @@ EXPORT_SYMBOL_GPL(fpu_sync_guest_vmexit_xfd_state);
int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
{
struct fpstate *guest_fps = guest_fpu->fpstate;
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
struct fpstate *cur_fps = fpu->fpstate;

fpregs_lock();
@@ -430,7 +430,7 @@ void kernel_fpu_begin_mask(unsigned int kfpu_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(&current->thread.fpu);
+ save_fpregs_to_fpstate(current->thread.fpu);
}
__cpu_invalidate_fpregs_state();

@@ -458,7 +458,7 @@ EXPORT_SYMBOL_GPL(kernel_fpu_end);
*/
void fpu_sync_fpstate(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != &current->thread.fpu);
+ WARN_ON_FPU(fpu != current->thread.fpu);

fpregs_lock();
trace_x86_fpu_before_save(fpu);
@@ -543,7 +543,7 @@ void fpstate_reset(struct fpu *fpu)
static inline void fpu_inherit_perms(struct fpu *dst_fpu)
{
if (fpu_state_size_dynamic()) {
- struct fpu *src_fpu = &current->group_leader->thread.fpu;
+ struct fpu *src_fpu = current->group_leader->thread.fpu;

spin_lock_irq(&current->sighand->siglock);
/* Fork also inherits the permissions of the parent */
@@ -563,7 +563,7 @@ static int update_fpu_shstk(struct task_struct *dst, unsigned long ssp)
if (!ssp)
return 0;

- xstate = get_xsave_addr(&dst->thread.fpu.fpstate->regs.xsave,
+ xstate = get_xsave_addr(&dst->thread.fpu->fpstate->regs.xsave,
XFEATURE_CET_USER);

/*
@@ -584,8 +584,19 @@ static int update_fpu_shstk(struct task_struct *dst, unsigned long ssp)
int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
unsigned long ssp)
{
- struct fpu *src_fpu = &current->thread.fpu;
- struct fpu *dst_fpu = &dst->thread.fpu;
+ /*
+ * We allocate the new FPU structure right after the end of the task struct.
+ * task allocation size already took this into account.
+ *
+ * This is safe because task_struct size is a multiple of cacheline size.
+ */
+ struct fpu *dst_fpu = (void *)dst + sizeof(*dst);
+ struct fpu *src_fpu = current->thread.fpu;
+
+ BUILD_BUG_ON(sizeof(*dst) % SMP_CACHE_BYTES != 0);
+ BUG_ON(!src_fpu);
+
+ dst->thread.fpu = dst_fpu;

/* The new task's FPU state cannot be valid in the hardware. */
dst_fpu->last_cpu = -1;
@@ -654,16 +665,6 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
return 0;
}

-/*
- * Whitelist the FPU register state embedded into task_struct for hardened
- * usercopy.
- */
-void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
-{
- *offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
- *size = fpu_kernel_cfg.default_size;
-}
-
/*
* Drops current FPU state: deactivates the fpregs and
* the fpstate. NOTE: it still leaves previous contents
@@ -677,7 +678,7 @@ void fpu__drop(struct fpu *fpu)
{
preempt_disable();

- if (fpu == &current->thread.fpu) {
+ if (fpu == current->thread.fpu) {
/* Ignore delayed exceptions from user space */
asm volatile("1: fwait\n"
"2:\n"
@@ -711,7 +712,7 @@ static inline void restore_fpregs_from_init_fpstate(u64 features_mask)
*/
static void fpu_reset_fpregs(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

fpregs_lock();
__fpu_invalidate_fpregs_state(fpu);
@@ -740,7 +741,7 @@ static void fpu_reset_fpregs(void)
*/
void fpu__clear_user_states(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != &current->thread.fpu);
+ WARN_ON_FPU(fpu != current->thread.fpu);

fpregs_lock();
if (!cpu_feature_enabled(X86_FEATURE_FPU)) {
@@ -773,7 +774,7 @@ void fpu__clear_user_states(struct fpu *fpu)

void fpu_flush_thread(void)
{
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
fpu_reset_fpregs();
}
/*
@@ -814,7 +815,7 @@ void fpregs_lock_and_load(void)
*/
void fpregs_assert_state_consistent(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

if (test_thread_flag(TIF_NEED_FPU_LOAD))
return;
@@ -826,7 +827,7 @@ EXPORT_SYMBOL_GPL(fpregs_assert_state_consistent);

void fpregs_mark_activate(void)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

fpregs_activate(fpu);
fpu->last_cpu = smp_processor_id();
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 998a08f17e33..de618ec509aa 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
+ fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
else
#endif
asm volatile ("fninit");
@@ -71,8 +71,17 @@ static bool __init fpu__probe_without_cpuid(void)
return fsw == 0 && (fcw & 0x103f) == 0x003f;
}

+static struct fpu x86_init_fpu __read_mostly;
+
static void __init fpu__init_system_early_generic(void)
{
+ int this_cpu = smp_processor_id();
+
+ fpstate_reset(&x86_init_fpu);
+ current->thread.fpu = &x86_init_fpu;
+ per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
+ x86_init_fpu.last_cpu = this_cpu;
+
if (!boot_cpu_has(X86_FEATURE_CPUID) &&
!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
if (fpu__probe_without_cpuid())
@@ -150,11 +159,13 @@ static void __init fpu__init_task_struct_size(void)
{
int task_size = sizeof(struct task_struct);

+ task_size += sizeof(struct fpu);
+
/*
* Subtract off the static size of the register state.
* It potentially has a bunch of padding.
*/
- task_size -= sizeof(current->thread.fpu.__fpstate.regs);
+ task_size -= sizeof(current->thread.fpu->__fpstate.regs);

/*
* Add back the dynamically-calculated register state
@@ -164,14 +175,9 @@ static void __init fpu__init_task_struct_size(void)

/*
* We dynamically size 'struct fpu', so we require that
- * it be at the end of 'thread_struct' and that
- * 'thread_struct' be at the end of 'task_struct'. If
- * you hit a compile error here, check the structure to
- * see if something got added to the end.
+ * 'state' be at the end of 'it:
*/
CHECK_MEMBER_AT_END_OF(struct fpu, __fpstate);
- CHECK_MEMBER_AT_END_OF(struct thread_struct, fpu);
- CHECK_MEMBER_AT_END_OF(struct task_struct, thread);

arch_task_struct_size = task_size;
}
@@ -204,7 +210,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
fpu_kernel_cfg.default_size = size;
fpu_user_cfg.max_size = size;
fpu_user_cfg.default_size = size;
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
}

/*
@@ -213,7 +219,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
*/
void __init fpu__init_system(void)
{
- fpstate_reset(&current->thread.fpu);
fpu__init_system_early_generic();

/*
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 6bc1eb2a21bd..38bc0b390d02 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -45,7 +45,7 @@ int regset_xregset_fpregs_active(struct task_struct *target, const struct user_r
*/
static void sync_fpstate(struct fpu *fpu)
{
- if (fpu == &current->thread.fpu)
+ if (fpu == current->thread.fpu)
fpu_sync_fpstate(fpu);
}

@@ -63,7 +63,7 @@ static void fpu_force_restore(struct fpu *fpu)
* Only stopped child tasks can be used to modify the FPU
* state in the fpstate buffer:
*/
- WARN_ON_FPU(fpu == &current->thread.fpu);
+ WARN_ON_FPU(fpu == current->thread.fpu);

__fpu_invalidate_fpregs_state(fpu);
}
@@ -71,7 +71,7 @@ static void fpu_force_restore(struct fpu *fpu)
int xfpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;

if (!cpu_feature_enabled(X86_FEATURE_FXSR))
return -ENODEV;
@@ -91,7 +91,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct fxregs_state newstate;
int ret;

@@ -133,7 +133,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
if (!cpu_feature_enabled(X86_FEATURE_XSAVE))
return -ENODEV;

- sync_fpstate(&target->thread.fpu);
+ sync_fpstate(target->thread.fpu);

copy_xstate_to_uabi_buf(to, target, XSTATE_COPY_XSAVE);
return 0;
@@ -143,7 +143,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct xregs_state *tmpbuf = NULL;
int ret;

@@ -187,7 +187,7 @@ int ssp_active(struct task_struct *target, const struct user_regset *regset)
int ssp_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct cet_user_state *cetregs;

if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
@@ -213,7 +213,7 @@ int ssp_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct xregs_state *xsave = &fpu->fpstate->regs.xsave;
struct cet_user_state *cetregs;
unsigned long user_ssp;
@@ -367,7 +367,7 @@ static void __convert_from_fxsr(struct user_i387_ia32_struct *env,
void
convert_from_fxsr(struct user_i387_ia32_struct *env, struct task_struct *tsk)
{
- __convert_from_fxsr(env, tsk, &tsk->thread.fpu.fpstate->regs.fxsave);
+ __convert_from_fxsr(env, tsk, &tsk->thread.fpu->fpstate->regs.fxsave);
}

void convert_to_fxsr(struct fxregs_state *fxsave,
@@ -400,7 +400,7 @@ void convert_to_fxsr(struct fxregs_state *fxsave,
int fpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct user_i387_ia32_struct env;
struct fxregs_state fxsave, *fx;

@@ -432,7 +432,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = &target->thread.fpu;
+ struct fpu *fpu = target->thread.fpu;
struct user_i387_ia32_struct env;
int ret;

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 247f2225aa9f..6b262d0b8fc1 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -38,7 +38,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
/* Check for the first magic field and other error scenarios. */
if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
fx_sw->xstate_size < min_xstate_size ||
- fx_sw->xstate_size > current->thread.fpu.fpstate->user_size ||
+ fx_sw->xstate_size > current->thread.fpu->fpstate->user_size ||
fx_sw->xstate_size > fx_sw->extended_size)
goto setfx;

@@ -54,7 +54,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
if (likely(magic2 == FP_XSTATE_MAGIC2))
return true;
setfx:
- trace_x86_fpu_xstate_check_failed(&current->thread.fpu);
+ trace_x86_fpu_xstate_check_failed(current->thread.fpu);

/* Set the parameters for fx only state */
fx_sw->magic1 = 0;
@@ -69,13 +69,13 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
static inline bool save_fsave_header(struct task_struct *tsk, void __user *buf)
{
if (use_fxsr()) {
- struct xregs_state *xsave = &tsk->thread.fpu.fpstate->regs.xsave;
+ struct xregs_state *xsave = &tsk->thread.fpu->fpstate->regs.xsave;
struct user_i387_ia32_struct env;
struct _fpstate_32 __user *fp = buf;

fpregs_lock();
if (!test_thread_flag(TIF_NEED_FPU_LOAD))
- fxsave(&tsk->thread.fpu.fpstate->regs.fxsave);
+ fxsave(&tsk->thread.fpu->fpstate->regs.fxsave);
fpregs_unlock();

convert_from_fxsr(&env, tsk);
@@ -188,7 +188,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
{
struct task_struct *tsk = current;
- struct fpstate *fpstate = tsk->thread.fpu.fpstate;
+ struct fpstate *fpstate = tsk->thread.fpu->fpstate;
bool ia32_fxstate = (buf != buf_fx);
int ret;

@@ -276,7 +276,7 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
*/
static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
int ret;

/* Restore enabled features only. */
@@ -336,7 +336,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
bool ia32_fxstate)
{
struct task_struct *tsk = current;
- struct fpu *fpu = &tsk->thread.fpu;
+ struct fpu *fpu = tsk->thread.fpu;
struct user_i387_ia32_struct env;
bool success, fx_only = false;
union fpregs_state *fpregs;
@@ -456,7 +456,7 @@ static inline unsigned int xstate_sigframe_size(struct fpstate *fpstate)
*/
bool fpu__restore_sig(void __user *buf, int ia32_frame)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
void __user *buf_fx = buf;
bool ia32_fxstate = false;
bool success = false;
@@ -503,7 +503,7 @@ unsigned long
fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
unsigned long *buf_fx, unsigned long *size)
{
- unsigned long frame_size = xstate_sigframe_size(current->thread.fpu.fpstate);
+ unsigned long frame_size = xstate_sigframe_size(current->thread.fpu->fpstate);

*buf_fx = sp = round_down(sp - frame_size, 64);
if (ia32_frame && use_fxsr()) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 117e74c44e75..df442b2df55b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -734,7 +734,7 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
*/
init_fpstate.xfd = 0;

- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);
}

/*
@@ -844,7 +844,7 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
goto out_disable;

/* Reset the state for the current task */
- fpstate_reset(&current->thread.fpu);
+ fpstate_reset(current->thread.fpu);

/*
* Update info used for ptrace frames; use standard-format size and no
@@ -918,7 +918,7 @@ void fpu__resume_cpu(void)
}

if (fpu_state_size_dynamic())
- wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
+ wrmsrl(MSR_IA32_XFD, current->thread.fpu->fpstate->xfd);
}

/*
@@ -1186,8 +1186,8 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
void copy_xstate_to_uabi_buf(struct membuf to, struct task_struct *tsk,
enum xstate_copy_mode copy_mode)
{
- __copy_xstate_to_uabi_buf(to, tsk->thread.fpu.fpstate,
- tsk->thread.fpu.fpstate->user_xfeatures,
+ __copy_xstate_to_uabi_buf(to, tsk->thread.fpu->fpstate,
+ tsk->thread.fpu->fpstate->user_xfeatures,
tsk->thread.pkru, copy_mode);
}

@@ -1327,7 +1327,7 @@ int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf, u
int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
const void __user *ubuf)
{
- return copy_uabi_to_xstate(tsk->thread.fpu.fpstate, NULL, ubuf, &tsk->thread.pkru);
+ return copy_uabi_to_xstate(tsk->thread.fpu->fpstate, NULL, ubuf, &tsk->thread.pkru);
}

static bool validate_independent_components(u64 mask)
@@ -1421,7 +1421,7 @@ static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor)
* The XFD MSR does not match fpstate->xfd. That's invalid when
* the passed in fpstate is current's fpstate.
*/
- if (fpstate->xfd == current->thread.fpu.fpstate->xfd)
+ if (fpstate->xfd == current->thread.fpu->fpstate->xfd)
return false;

/*
@@ -1498,7 +1498,7 @@ void fpstate_free(struct fpu *fpu)
static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
unsigned int usize, struct fpu_guest *guest_fpu)
{
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;
struct fpstate *curfps, *newfps = NULL;
unsigned int fpsize;
bool in_use;
@@ -1591,7 +1591,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
* AVX512.
*/
bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
- struct fpu *fpu = &current->group_leader->thread.fpu;
+ struct fpu *fpu = current->group_leader->thread.fpu;
struct fpu_state_perm *perm;
unsigned int ksize, usize;
u64 mask;
@@ -1694,7 +1694,7 @@ int __xfd_enable_feature(u64 xfd_err, struct fpu_guest *guest_fpu)
return -EPERM;
}

- fpu = &current->group_leader->thread.fpu;
+ fpu = current->group_leader->thread.fpu;
perm = guest_fpu ? &fpu->guest_perm : &fpu->perm;
ksize = perm->__state_size;
usize = perm->__user_state_size;
@@ -1799,7 +1799,7 @@ 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(task->thread.fpu.avx512_timestamp);
+ unsigned long timestamp = READ_ONCE(task->thread.fpu->avx512_timestamp);
long delta;

if (!timestamp) {
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 3518fb26d06b..aca8a89bea84 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -22,7 +22,7 @@ static inline void xstate_init_xcomp_bv(struct xregs_state *xsave, u64 mask)

static inline u64 xstate_get_group_perm(bool guest)
{
- struct fpu *fpu = &current->group_leader->thread.fpu;
+ struct fpu *fpu = current->group_leader->thread.fpu;
struct fpu_state_perm *perm;

/* Pairs with WRITE_ONCE() in xstate_request_perm() */
@@ -261,7 +261,7 @@ static inline int xsave_to_user_sigframe(struct xregs_state __user *buf)
* internally, e.g. PKRU. That's user space ABI and also required
* to allow the signal handler to modify PKRU.
*/
- struct fpstate *fpstate = current->thread.fpu.fpstate;
+ struct fpstate *fpstate = current->thread.fpu->fpstate;
u64 mask = fpstate->user_xfeatures;
u32 lmask;
u32 hmask;
@@ -292,7 +292,7 @@ static inline int xrstor_from_user_sigframe(struct xregs_state __user *buf, u64
u32 hmask = mask >> 32;
int err;

- xfd_validate_state(current->thread.fpu.fpstate, mask, true);
+ xfd_validate_state(current->thread.fpu->fpstate, mask, true);

stac();
XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b8441147eb5e..5f3f48713870 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -97,7 +97,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
dst->thread.vm86 = NULL;
#endif
/* Drop the copied pointer to current's fpstate */
- dst->thread.fpu.fpstate = NULL;
+ dst->thread.fpu = NULL;

return 0;
}
@@ -106,7 +106,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
void arch_release_task_struct(struct task_struct *tsk)
{
if (fpu_state_size_dynamic())
- fpstate_free(&tsk->thread.fpu);
+ fpstate_free(tsk->thread.fpu);
}
#endif

@@ -116,7 +116,7 @@ void arch_release_task_struct(struct task_struct *tsk)
void exit_thread(struct task_struct *tsk)
{
struct thread_struct *t = &tsk->thread;
- struct fpu *fpu = &t->fpu;
+ struct fpu *fpu = t->fpu;

if (test_thread_flag(TIF_IO_BITMAP))
io_bitmap_exit(tsk);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 31b6f5dddfc2..7e709ae8e99a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -228,7 +228,7 @@ static void
handle_signal(struct ksignal *ksig, struct pt_regs *regs)
{
bool stepping, failed;
- struct fpu *fpu = &current->thread.fpu;
+ struct fpu *fpu = current->thread.fpu;

if (v8086_mode(regs))
save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
@@ -396,14 +396,14 @@ bool sigaltstack_size_valid(size_t ss_size)
if (!fpu_state_size_dynamic() && !strict_sigaltstack_size)
return true;

- fsize += current->group_leader->thread.fpu.perm.__user_state_size;
+ fsize += current->group_leader->thread.fpu->perm.__user_state_size;
if (likely(ss_size > fsize))
return true;

if (strict_sigaltstack_size)
return ss_size > fsize;

- mask = current->group_leader->thread.fpu.perm.__state_perm;
+ mask = current->group_leader->thread.fpu->perm.__state_perm;
if (mask & XFEATURE_MASK_USER_DYNAMIC)
return ss_size > fsize;

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17e5043..c79e36c4071b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1148,7 +1148,7 @@ DEFINE_IDTENTRY_RAW(exc_debug)
static void math_error(struct pt_regs *regs, int trapnr)
{
struct task_struct *task = current;
- struct fpu *fpu = &task->thread.fpu;
+ struct fpu *fpu = task->thread.fpu;
int si_code;
char *str = (trapnr == X86_TRAP_MF) ? "fpu exception" :
"simd exception";
diff --git a/arch/x86/math-emu/fpu_aux.c b/arch/x86/math-emu/fpu_aux.c
index d62662bdd460..8087953164fc 100644
--- a/arch/x86/math-emu/fpu_aux.c
+++ b/arch/x86/math-emu/fpu_aux.c
@@ -53,7 +53,7 @@ void fpstate_init_soft(struct swregs_state *soft)

void finit(void)
{
- fpstate_init_soft(&current->thread.fpu.fpstate->regs.soft);
+ fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
}

/*
diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index 91c52ead1226..30400d95d9d0 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -641,7 +641,7 @@ int fpregs_soft_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct swregs_state *s387 = &target->thread.fpu.fpstate->regs.soft;
+ struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
void *space = s387->st_space;
int ret;
int offset, other, i, tags, regnr, tag, newtop;
@@ -692,7 +692,7 @@ int fpregs_soft_get(struct task_struct *target,
const struct user_regset *regset,
struct membuf to)
{
- struct swregs_state *s387 = &target->thread.fpu.fpstate->regs.soft;
+ struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
const void *space = s387->st_space;
int offset = (S387->ftop & 7) * 10, other = 80 - offset;

diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index eec3e4805c75..3417337e7d99 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -73,7 +73,7 @@ static inline bool seg_writable(struct desc_struct *d)
return (d->type & SEG_TYPE_EXECUTE_MASK) == SEG_TYPE_WRITABLE;
}

-#define I387 (&current->thread.fpu.fpstate->regs)
+#define I387 (&current->thread.fpu->fpstate->regs)
#define FPU_info (I387->soft.info)

#define FPU_CS (*(unsigned short *) &(FPU_info->regs->cs))
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index b522933bfa56..cd73a533db69 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -111,7 +111,7 @@ static bool ex_handler_sgx(const struct exception_table_entry *fixup,

/*
* Handler for when we fail to restore a task's FPU state. We should never get
- * here because the FPU state of a task using the FPU (task->thread.fpu.state)
+ * here because the FPU state of a task using the FPU (task->thread.fpu->state)
* should always be valid. However, past bugs have allowed userspace to set
* reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn().
* These caused XRSTOR to fail when switching to the task, leaking the FPU
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3c2abbc587b4..069d262730b4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1550,21 +1550,14 @@ struct task_struct {
struct user_event_mm *user_event_mm;
#endif

+ /* CPU-specific state of this task: */
+ struct thread_struct thread;
+
/*
* New fields for task_struct should be added above here, so that
* they are included in the randomized portion of task_struct.
*/
randomized_struct_fields_end
-
- /* CPU-specific state of this task: */
- struct thread_struct thread;
-
- /*
- * WARNING: on x86, 'thread_struct' contains a variable-sized
- * structure. It *MUST* be at the end of 'task_struct'.
- *
- * Do not put anything below here!
- */
};

#define TASK_REPORT_IDLE (TASK_REPORT + 1)

2024-03-26 21:56:36

by Oleg Nesterov

Subject: Re: [PATCH 1/1] headers/deps: x86/fpu: Make task_struct::thread constant size

On 03/26, Ingo Molnar wrote:
>
> * Oleg Nesterov <[email protected]> wrote:
>
> > > +static struct fpu x86_init_fpu __read_mostly;
> > > +
> > > static void __init fpu__init_system_early_generic(void)
> > > {
> > > + {
> > > + int this_cpu = smp_processor_id();
> > > +
> > > + fpstate_reset(&x86_init_fpu);
> > > + current->thread.fpu = &x86_init_fpu;
> > > + per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> > > + x86_init_fpu.last_cpu = this_cpu;
> > > + }
> >
> > Can't x86_init_fpu be declared inside the block above?
>
> As a function-local static? I think globals are better defined in a visible
> fashion, not hidden among local variables where they are easy to overlook.

OK, but please see below.

> Yeah. Something like the delta patch below?

LGTM. And the whole patch too, although I don't think I can review it ;)

but... Ingo, let me ask 2 stupid questions.

1. Can you split this patch? I mean, 1/2 should add something like

struct fpu *x86_task_fpu(struct task_struct *task)
{
return &target->thread.fpu;
}

somewhere in arch/x86/include and update all the users of ->thread.fpu
to use the new helper. No functional changes.

This way 2/2 which actually turns .fpu into a pointer will be MUCH simpler.

2. Now, 2/2 should change the new helper above to

return target->thread.fpu;

But! why do we need the thread_struct.fpu pointer at all? Can't we simply have

struct fpu *x86_task_fpu(struct task_struct *task)
{
return (void *)task + sizeof(*task);
}

? This is what fpu_clone() (with your patch) does anyway.

Yes, this needs some changes:

- exit_thread() - trivial

- arch_dup_task_struct() clears thread.fpu, but I guess this is unnecessary

- fpu_clone() initializes dst->thread.fpu, no longer needed

- finally, fpu__init_system_early_generic() which initializes the
init_task.thread.fpu pointer.

But this doesn't look difficult? Although I don't understand the magic in
arch/x86/kernel/vmlinux.lds.S ...

Oleg.

2024-03-29 09:10:39

by Ingo Molnar

Subject: [PATCH 2/1] headers/deps: x86/fpu: Remove the thread::fpu pointer

* Oleg Nesterov <[email protected]> wrote:

> On 03/26, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <[email protected]> wrote:
> >
> > > > +static struct fpu x86_init_fpu __read_mostly;
> > > > +
> > > > static void __init fpu__init_system_early_generic(void)
> > > > {
> > > > + {
> > > > + int this_cpu = smp_processor_id();
> > > > +
> > > > + fpstate_reset(&x86_init_fpu);
> > > > + current->thread.fpu = &x86_init_fpu;
> > > > + per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
> > > > + x86_init_fpu.last_cpu = this_cpu;
> > > > + }
> > >
> > > Can't x86_init_fpu be declared inside the block above?
> >
> > As a function-local static? I think globals are better defined in a visible
> > fashion, not hidden among local variables where they are easy to overlook.
>
> OK, but please see below.
>
> > Yeah. Something like the delta patch below?
>
> LGTM. And the whole patch too, although I don't think I can review it ;)
>
> but... Ingo, let me ask 2 stupid questions.
>
> 1. Can you split this patch? I mean, 1/2 should add something like
>
> struct fpu *x86_task_fpu(struct task_struct *task)
> {
> return &target->thread.fpu;
> }
>
> somewhere in arch/x86/include and update all the users of ->thread.fpu
> to use the new helper. No functional changes.
>
> This way 2/2 which actually turns .fpu into a pointer will be MUCH simpler.

Yeah.

> 2. Now, 2/2 should change the new helper above to
>
> return target->thread.fpu;
>
> But! why do we need the thread_struct.fpu pointer at all? Can't we simply have
>
> struct fpu *x86_task_fpu(struct task_struct *task)
> {
> return (void *)task + sizeof(*task);
> }
>
> ? This is what fpu_clone() (with your patch) does anyway.

Correct.

> Yes, this needs some changes:
>
> - exit_thread() - trivial
>
> - arch_dup_task_struct() clears thread.fpu, but I guess this is unnecessary
>
> - fpu_clone() initializes dst->thread.fpu, no longer needed
>
> - finally, fpu__init_system_early_generic() which initializes the
> init_task.thread.fpu pointer.
>
> But this doesn't look difficult? Although I don't understand the magic in
> arch/x86/kernel/vmlinux.lds.S ...

Yeah.

Something like the patch below, on top of my previous patch.

The main/only fishy bit left is:

--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -172,6 +172,10 @@ SECTIONS
/* init_task */
INIT_TASK_DATA(THREAD_SIZE)

+ __x86_init_fpu_begin = .;
+ . = __x86_init_fpu_begin + 128*PAGE_SIZE;
+ __x86_init_fpu_end = .;

.. as we don't know the FPU context switch size in advance, and modern
vector CPUs can be huge. Plus CPU vendors like to push more crap into
the FPU context area (XSAVE area), because the rather opaque
enumeration and variable-size format makes OS support easier for them.
:-/

And in principle init_task should never use its task-fpu context switch
area for real, but I couldn't convince myself (yet) whether that's true
under all circumstances - and getting this wrong would be a memory
corruptor ...

kernel_fpu_begin_mask() has this:

this_cpu_write(in_kernel_fpu, true);

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));
}
__cpu_invalidate_fpregs_state();

and the save_fpregs_to_fpstate() inside the branch should never execute
for init_task.

Thanks,

Ingo


==================>
arch/x86/include/asm/fpu/sched.h | 2 +-
arch/x86/include/asm/processor.h | 5 ++---
arch/x86/include/asm/vm86.h | 2 +-
arch/x86/kernel/fpu/context.h | 2 +-
arch/x86/kernel/fpu/core.c | 30 ++++++++++++++----------------
arch/x86/kernel/fpu/init.c | 7 +++----
arch/x86/kernel/fpu/regset.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/signal.c | 18 +++++++++---------
arch/x86/kernel/fpu/xstate.c | 22 +++++++++++-----------
arch/x86/kernel/fpu/xstate.h | 6 +++---
arch/x86/kernel/process.c | 6 ++----
arch/x86/kernel/signal.c | 6 +++---
arch/x86/kernel/traps.c | 2 +-
arch/x86/kernel/vmlinux.lds.S | 4 ++++
arch/x86/math-emu/fpu_aux.c | 2 +-
arch/x86/math-emu/fpu_entry.c | 4 ++--
arch/x86/math-emu/fpu_system.h | 2 +-
arch/x86/mm/extable.c | 2 +-
18 files changed, 71 insertions(+), 73 deletions(-)

diff --git a/arch/x86/include/asm/fpu/sched.h b/arch/x86/include/asm/fpu/sched.h
index 3cf20ab49b5f..1feaa68b7567 100644
--- a/arch/x86/include/asm/fpu/sched.h
+++ b/arch/x86/include/asm/fpu/sched.h
@@ -41,7 +41,7 @@ static inline void switch_fpu_prepare(struct task_struct *old, int cpu)
{
if (cpu_feature_enabled(X86_FEATURE_FPU) &&
!(old->flags & (PF_KTHREAD | PF_USER_WORKER))) {
- struct fpu *old_fpu = old->thread.fpu;
+ struct fpu *old_fpu = x86_task_fpu(old);

save_fpregs_to_fpstate(old_fpu);
/*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 151880f2b079..fb78720774ea 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -489,11 +489,10 @@ struct thread_struct {

struct thread_shstk shstk;
#endif
-
- /* Floating point and extended processor state */
- struct fpu *fpu;
};

+#define x86_task_fpu(task) ((struct fpu *)((void *)task + sizeof(*task)))
+
/*
* X86 doesn't need any embedded-FPU-struct quirks:
*/
diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 9e8ac5073ecb..94fbcd979aac 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -84,7 +84,7 @@ static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)

static inline void save_v86_state(struct kernel_vm86_regs *a, int b) { }

-#define free_vm86(t) do { } while(0)
+#define free_vm86(t) do { (void)(t); } while(0)

#endif /* CONFIG_VM86 */

diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
index 96d1f34179b3..10d0a720659c 100644
--- a/arch/x86/kernel/fpu/context.h
+++ b/arch/x86/kernel/fpu/context.h
@@ -53,7 +53,7 @@ static inline void fpregs_activate(struct fpu *fpu)
/* Internal helper for switch_fpu_return() and signal frame setup */
static inline void fpregs_restore_userregs(void)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);
int cpu = smp_processor_id();

if (WARN_ON_ONCE(current->flags & (PF_KTHREAD | PF_USER_WORKER)))
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 816d48978da4..0ccabcd3bf62 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -204,7 +204,7 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
return;

spin_lock_irq(&current->sighand->siglock);
- fpuperm = &current->group_leader->thread.fpu->guest_perm;
+ fpuperm = &x86_task_fpu(current->group_leader)->guest_perm;
perm = fpuperm->__state_perm;

/* First fpstate allocation locks down permissions. */
@@ -316,7 +316,7 @@ EXPORT_SYMBOL_GPL(fpu_update_guest_xfd);
*/
void fpu_sync_guest_vmexit_xfd_state(void)
{
- struct fpstate *fps = current->thread.fpu->fpstate;
+ struct fpstate *fps = x86_task_fpu(current)->fpstate;

lockdep_assert_irqs_disabled();
if (fpu_state_size_dynamic()) {
@@ -330,7 +330,7 @@ EXPORT_SYMBOL_GPL(fpu_sync_guest_vmexit_xfd_state);
int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
{
struct fpstate *guest_fps = guest_fpu->fpstate;
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);
struct fpstate *cur_fps = fpu->fpstate;

fpregs_lock();
@@ -430,7 +430,7 @@ void kernel_fpu_begin_mask(unsigned int kfpu_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(current->thread.fpu);
+ save_fpregs_to_fpstate(x86_task_fpu(current));
}
__cpu_invalidate_fpregs_state();

@@ -458,7 +458,7 @@ EXPORT_SYMBOL_GPL(kernel_fpu_end);
*/
void fpu_sync_fpstate(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != current->thread.fpu);
+ WARN_ON_FPU(fpu != x86_task_fpu(current));

fpregs_lock();
trace_x86_fpu_before_save(fpu);
@@ -543,7 +543,7 @@ void fpstate_reset(struct fpu *fpu)
static inline void fpu_inherit_perms(struct fpu *dst_fpu)
{
if (fpu_state_size_dynamic()) {
- struct fpu *src_fpu = current->group_leader->thread.fpu;
+ struct fpu *src_fpu = x86_task_fpu(current->group_leader);

spin_lock_irq(&current->sighand->siglock);
/* Fork also inherits the permissions of the parent */
@@ -563,7 +563,7 @@ static int update_fpu_shstk(struct task_struct *dst, unsigned long ssp)
if (!ssp)
return 0;

- xstate = get_xsave_addr(&dst->thread.fpu->fpstate->regs.xsave,
+ xstate = get_xsave_addr(&x86_task_fpu(dst)->fpstate->regs.xsave,
XFEATURE_CET_USER);

/*
@@ -591,13 +591,11 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
* This is safe because task_struct size is a multiple of cacheline size.
*/
struct fpu *dst_fpu = (void *)dst + sizeof(*dst);
- struct fpu *src_fpu = current->thread.fpu;
+ struct fpu *src_fpu = x86_task_fpu(current);

BUILD_BUG_ON(sizeof(*dst) % SMP_CACHE_BYTES != 0);
BUG_ON(!src_fpu);

- dst->thread.fpu = dst_fpu;
-
/* The new task's FPU state cannot be valid in the hardware. */
dst_fpu->last_cpu = -1;

@@ -678,7 +676,7 @@ void fpu__drop(struct fpu *fpu)
{
preempt_disable();

- if (fpu == current->thread.fpu) {
+ if (fpu == x86_task_fpu(current)) {
/* Ignore delayed exceptions from user space */
asm volatile("1: fwait\n"
"2:\n"
@@ -712,7 +710,7 @@ static inline void restore_fpregs_from_init_fpstate(u64 features_mask)
*/
static void fpu_reset_fpregs(void)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);

fpregs_lock();
__fpu_invalidate_fpregs_state(fpu);
@@ -741,7 +739,7 @@ static void fpu_reset_fpregs(void)
*/
void fpu__clear_user_states(struct fpu *fpu)
{
- WARN_ON_FPU(fpu != current->thread.fpu);
+ WARN_ON_FPU(fpu != x86_task_fpu(current));

fpregs_lock();
if (!cpu_feature_enabled(X86_FEATURE_FPU)) {
@@ -774,7 +772,7 @@ void fpu__clear_user_states(struct fpu *fpu)

void fpu_flush_thread(void)
{
- fpstate_reset(current->thread.fpu);
+ fpstate_reset(x86_task_fpu(current));
fpu_reset_fpregs();
}
/*
@@ -815,7 +813,7 @@ void fpregs_lock_and_load(void)
*/
void fpregs_assert_state_consistent(void)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);

if (test_thread_flag(TIF_NEED_FPU_LOAD))
return;
@@ -827,7 +825,7 @@ EXPORT_SYMBOL_GPL(fpregs_assert_state_consistent);

void fpregs_mark_activate(void)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);

fpregs_activate(fpu);
fpu->last_cpu = smp_processor_id();
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index de618ec509aa..11aa31410df2 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
+ fpstate_init_soft(&x86_task_fpu(current)->fpstate->regs.soft);
else
#endif
asm volatile ("fninit");
@@ -78,7 +78,6 @@ static void __init fpu__init_system_early_generic(void)
int this_cpu = smp_processor_id();

fpstate_reset(&x86_init_fpu);
- current->thread.fpu = &x86_init_fpu;
per_cpu(fpu_fpregs_owner_ctx, this_cpu) = &x86_init_fpu;
x86_init_fpu.last_cpu = this_cpu;

@@ -165,7 +164,7 @@ static void __init fpu__init_task_struct_size(void)
* Subtract off the static size of the register state.
* It potentially has a bunch of padding.
*/
- task_size -= sizeof(current->thread.fpu->__fpstate.regs);
+ task_size -= sizeof(x86_task_fpu(current)->__fpstate.regs);

/*
* Add back the dynamically-calculated register state
@@ -210,7 +209,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
fpu_kernel_cfg.default_size = size;
fpu_user_cfg.max_size = size;
fpu_user_cfg.default_size = size;
- fpstate_reset(current->thread.fpu);
+ fpstate_reset(x86_task_fpu(current));
}

/*
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 38bc0b390d02..19fd159217f7 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -45,7 +45,7 @@ int regset_xregset_fpregs_active(struct task_struct *target, const struct user_r
*/
static void sync_fpstate(struct fpu *fpu)
{
- if (fpu == current->thread.fpu)
+ if (fpu == x86_task_fpu(current))
fpu_sync_fpstate(fpu);
}

@@ -63,7 +63,7 @@ static void fpu_force_restore(struct fpu *fpu)
* Only stopped child tasks can be used to modify the FPU
* state in the fpstate buffer:
*/
- WARN_ON_FPU(fpu == current->thread.fpu);
+ WARN_ON_FPU(fpu == x86_task_fpu(current));

__fpu_invalidate_fpregs_state(fpu);
}
@@ -71,7 +71,7 @@ static void fpu_force_restore(struct fpu *fpu)
int xfpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);

if (!cpu_feature_enabled(X86_FEATURE_FXSR))
return -ENODEV;
@@ -91,7 +91,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct fxregs_state newstate;
int ret;

@@ -133,7 +133,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
if (!cpu_feature_enabled(X86_FEATURE_XSAVE))
return -ENODEV;

- sync_fpstate(target->thread.fpu);
+ sync_fpstate(x86_task_fpu(target));

copy_xstate_to_uabi_buf(to, target, XSTATE_COPY_XSAVE);
return 0;
@@ -143,7 +143,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct xregs_state *tmpbuf = NULL;
int ret;

@@ -187,7 +187,7 @@ int ssp_active(struct task_struct *target, const struct user_regset *regset)
int ssp_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct cet_user_state *cetregs;

if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
@@ -213,7 +213,7 @@ int ssp_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct xregs_state *xsave = &fpu->fpstate->regs.xsave;
struct cet_user_state *cetregs;
unsigned long user_ssp;
@@ -367,7 +367,7 @@ static void __convert_from_fxsr(struct user_i387_ia32_struct *env,
void
convert_from_fxsr(struct user_i387_ia32_struct *env, struct task_struct *tsk)
{
- __convert_from_fxsr(env, tsk, &tsk->thread.fpu->fpstate->regs.fxsave);
+ __convert_from_fxsr(env, tsk, &x86_task_fpu(tsk)->fpstate->regs.fxsave);
}

void convert_to_fxsr(struct fxregs_state *fxsave,
@@ -400,7 +400,7 @@ void convert_to_fxsr(struct fxregs_state *fxsave,
int fpregs_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct user_i387_ia32_struct env;
struct fxregs_state fxsave, *fx;

@@ -432,7 +432,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct fpu *fpu = target->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(target);
struct user_i387_ia32_struct env;
int ret;

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 6b262d0b8fc1..b203bf4617fc 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -38,7 +38,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
/* Check for the first magic field and other error scenarios. */
if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
fx_sw->xstate_size < min_xstate_size ||
- fx_sw->xstate_size > current->thread.fpu->fpstate->user_size ||
+ fx_sw->xstate_size > x86_task_fpu(current)->fpstate->user_size ||
fx_sw->xstate_size > fx_sw->extended_size)
goto setfx;

@@ -54,7 +54,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
if (likely(magic2 == FP_XSTATE_MAGIC2))
return true;
setfx:
- trace_x86_fpu_xstate_check_failed(current->thread.fpu);
+ trace_x86_fpu_xstate_check_failed(x86_task_fpu(current));

/* Set the parameters for fx only state */
fx_sw->magic1 = 0;
@@ -69,13 +69,13 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
static inline bool save_fsave_header(struct task_struct *tsk, void __user *buf)
{
if (use_fxsr()) {
- struct xregs_state *xsave = &tsk->thread.fpu->fpstate->regs.xsave;
+ struct xregs_state *xsave = &x86_task_fpu(tsk)->fpstate->regs.xsave;
struct user_i387_ia32_struct env;
struct _fpstate_32 __user *fp = buf;

fpregs_lock();
if (!test_thread_flag(TIF_NEED_FPU_LOAD))
- fxsave(&tsk->thread.fpu->fpstate->regs.fxsave);
+ fxsave(&x86_task_fpu(tsk)->fpstate->regs.fxsave);
fpregs_unlock();

convert_from_fxsr(&env, tsk);
@@ -188,7 +188,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
{
struct task_struct *tsk = current;
- struct fpstate *fpstate = tsk->thread.fpu->fpstate;
+ struct fpstate *fpstate = x86_task_fpu(tsk)->fpstate;
bool ia32_fxstate = (buf != buf_fx);
int ret;

@@ -276,7 +276,7 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
*/
static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);
int ret;

/* Restore enabled features only. */
@@ -336,7 +336,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
bool ia32_fxstate)
{
struct task_struct *tsk = current;
- struct fpu *fpu = tsk->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(tsk);
struct user_i387_ia32_struct env;
bool success, fx_only = false;
union fpregs_state *fpregs;
@@ -456,7 +456,7 @@ static inline unsigned int xstate_sigframe_size(struct fpstate *fpstate)
*/
bool fpu__restore_sig(void __user *buf, int ia32_frame)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);
void __user *buf_fx = buf;
bool ia32_fxstate = false;
bool success = false;
@@ -503,7 +503,7 @@ unsigned long
fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
unsigned long *buf_fx, unsigned long *size)
{
- unsigned long frame_size = xstate_sigframe_size(current->thread.fpu->fpstate);
+ unsigned long frame_size = xstate_sigframe_size(x86_task_fpu(current)->fpstate);

*buf_fx = sp = round_down(sp - frame_size, 64);
if (ia32_frame && use_fxsr()) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 4c613e77ead1..dade16d92484 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -735,7 +735,7 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
*/
init_fpstate.xfd = 0;

- fpstate_reset(current->thread.fpu);
+ fpstate_reset(x86_task_fpu(current));
}

/*
@@ -845,7 +845,7 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
goto out_disable;

/* Reset the state for the current task */
- fpstate_reset(current->thread.fpu);
+ fpstate_reset(x86_task_fpu(current));

/*
* Update info used for ptrace frames; use standard-format size and no
@@ -919,7 +919,7 @@ void fpu__resume_cpu(void)
}

if (fpu_state_size_dynamic())
- wrmsrl(MSR_IA32_XFD, current->thread.fpu->fpstate->xfd);
+ wrmsrl(MSR_IA32_XFD, x86_task_fpu(current)->fpstate->xfd);
}

/*
@@ -1187,8 +1187,8 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
void copy_xstate_to_uabi_buf(struct membuf to, struct task_struct *tsk,
enum xstate_copy_mode copy_mode)
{
- __copy_xstate_to_uabi_buf(to, tsk->thread.fpu->fpstate,
- tsk->thread.fpu->fpstate->user_xfeatures,
+ __copy_xstate_to_uabi_buf(to, x86_task_fpu(tsk)->fpstate,
+ x86_task_fpu(tsk)->fpstate->user_xfeatures,
tsk->thread.pkru, copy_mode);
}

@@ -1328,7 +1328,7 @@ int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf, u
int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
const void __user *ubuf)
{
- return copy_uabi_to_xstate(tsk->thread.fpu->fpstate, NULL, ubuf, &tsk->thread.pkru);
+ return copy_uabi_to_xstate(x86_task_fpu(tsk)->fpstate, NULL, ubuf, &tsk->thread.pkru);
}

static bool validate_independent_components(u64 mask)
@@ -1422,7 +1422,7 @@ static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor)
* The XFD MSR does not match fpstate->xfd. That's invalid when
* the passed in fpstate is current's fpstate.
*/
- if (fpstate->xfd == current->thread.fpu->fpstate->xfd)
+ if (fpstate->xfd == x86_task_fpu(current)->fpstate->xfd)
return false;

/*
@@ -1499,7 +1499,7 @@ void fpstate_free(struct fpu *fpu)
static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
unsigned int usize, struct fpu_guest *guest_fpu)
{
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);
struct fpstate *curfps, *newfps = NULL;
unsigned int fpsize;
bool in_use;
@@ -1592,7 +1592,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
* AVX512.
*/
bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
- struct fpu *fpu = current->group_leader->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current->group_leader);
struct fpu_state_perm *perm;
unsigned int ksize, usize;
u64 mask;
@@ -1695,7 +1695,7 @@ int __xfd_enable_feature(u64 xfd_err, struct fpu_guest *guest_fpu)
return -EPERM;
}

- fpu = current->group_leader->thread.fpu;
+ fpu = x86_task_fpu(current->group_leader);
perm = guest_fpu ? &fpu->guest_perm : &fpu->perm;
ksize = perm->__state_size;
usize = perm->__user_state_size;
@@ -1800,7 +1800,7 @@ 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(task->thread.fpu->avx512_timestamp);
+ unsigned long timestamp = READ_ONCE(x86_task_fpu(task)->avx512_timestamp);
long delta;

if (!timestamp) {
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 18854304838b..8c1942dcf9bf 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -22,7 +22,7 @@ static inline void xstate_init_xcomp_bv(struct xregs_state *xsave, u64 mask)

static inline u64 xstate_get_group_perm(bool guest)
{
- struct fpu *fpu = current->group_leader->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current->group_leader);
struct fpu_state_perm *perm;

/* Pairs with WRITE_ONCE() in xstate_request_perm() */
@@ -267,7 +267,7 @@ static inline int xsave_to_user_sigframe(struct xregs_state __user *buf)
* internally, e.g. PKRU. That's user space ABI and also required
* to allow the signal handler to modify PKRU.
*/
- struct fpstate *fpstate = current->thread.fpu->fpstate;
+ struct fpstate *fpstate = x86_task_fpu(current)->fpstate;
u64 mask = fpstate->user_xfeatures;
u32 lmask;
u32 hmask;
@@ -298,7 +298,7 @@ static inline int xrstor_from_user_sigframe(struct xregs_state __user *buf, u64
u32 hmask = mask >> 32;
int err;

- xfd_validate_state(current->thread.fpu->fpstate, mask, true);
+ xfd_validate_state(x86_task_fpu(current)->fpstate, mask, true);

stac();
XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 5f3f48713870..4184c085627e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -96,8 +96,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
#ifdef CONFIG_VM86
dst->thread.vm86 = NULL;
#endif
- /* Drop the copied pointer to current's fpstate */
- dst->thread.fpu = NULL;

return 0;
}
@@ -106,7 +104,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
void arch_release_task_struct(struct task_struct *tsk)
{
if (fpu_state_size_dynamic())
- fpstate_free(tsk->thread.fpu);
+ fpstate_free(x86_task_fpu(tsk));
}
#endif

@@ -116,7 +114,7 @@ void arch_release_task_struct(struct task_struct *tsk)
void exit_thread(struct task_struct *tsk)
{
struct thread_struct *t = &tsk->thread;
- struct fpu *fpu = t->fpu;
+ struct fpu *fpu = x86_task_fpu(tsk);

if (test_thread_flag(TIF_IO_BITMAP))
io_bitmap_exit(tsk);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 7e709ae8e99a..d7906f5979a4 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -228,7 +228,7 @@ static void
handle_signal(struct ksignal *ksig, struct pt_regs *regs)
{
bool stepping, failed;
- struct fpu *fpu = current->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(current);

if (v8086_mode(regs))
save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
@@ -396,14 +396,14 @@ bool sigaltstack_size_valid(size_t ss_size)
if (!fpu_state_size_dynamic() && !strict_sigaltstack_size)
return true;

- fsize += current->group_leader->thread.fpu->perm.__user_state_size;
+ fsize += x86_task_fpu(current->group_leader)->perm.__user_state_size;
if (likely(ss_size > fsize))
return true;

if (strict_sigaltstack_size)
return ss_size > fsize;

- mask = current->group_leader->thread.fpu->perm.__state_perm;
+ mask = x86_task_fpu(current->group_leader)->perm.__state_perm;
if (mask & XFEATURE_MASK_USER_DYNAMIC)
return ss_size > fsize;

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c79e36c4071b..b19fb494c57c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1148,7 +1148,7 @@ DEFINE_IDTENTRY_RAW(exc_debug)
static void math_error(struct pt_regs *regs, int trapnr)
{
struct task_struct *task = current;
- struct fpu *fpu = task->thread.fpu;
+ struct fpu *fpu = x86_task_fpu(task);
int si_code;
char *str = (trapnr == X86_TRAP_MF) ? "fpu exception" :
"simd exception";
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 56451fd2099e..d21331187b20 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -172,6 +172,10 @@ SECTIONS
/* init_task */
INIT_TASK_DATA(THREAD_SIZE)

+ __x86_init_fpu_begin = .;
+ . = __x86_init_fpu_begin + 128*PAGE_SIZE;
+ __x86_init_fpu_end = .;
+
#ifdef CONFIG_X86_32
/* 32 bit has nosave before _edata */
NOSAVE_DATA
diff --git a/arch/x86/math-emu/fpu_aux.c b/arch/x86/math-emu/fpu_aux.c
index 8087953164fc..5f253ae406b6 100644
--- a/arch/x86/math-emu/fpu_aux.c
+++ b/arch/x86/math-emu/fpu_aux.c
@@ -53,7 +53,7 @@ void fpstate_init_soft(struct swregs_state *soft)

void finit(void)
{
- fpstate_init_soft(&current->thread.fpu->fpstate->regs.soft);
+ fpstate_init_soft(&x86_task_fpu(current)->fpstate->regs.soft);
}

/*
diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index 30400d95d9d0..5034df617740 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -641,7 +641,7 @@ int fpregs_soft_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
+ struct swregs_state *s387 = &x86_task_fpu(target)->fpstate->regs.soft;
void *space = s387->st_space;
int ret;
int offset, other, i, tags, regnr, tag, newtop;
@@ -692,7 +692,7 @@ int fpregs_soft_get(struct task_struct *target,
const struct user_regset *regset,
struct membuf to)
{
- struct swregs_state *s387 = &target->thread.fpu->fpstate->regs.soft;
+ struct swregs_state *s387 = &x86_task_fpu(target)->fpstate->regs.soft;
const void *space = s387->st_space;
int offset = (S387->ftop & 7) * 10, other = 80 - offset;

diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index 3417337e7d99..5e238e930fe3 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -73,7 +73,7 @@ static inline bool seg_writable(struct desc_struct *d)
return (d->type & SEG_TYPE_EXECUTE_MASK) == SEG_TYPE_WRITABLE;
}

-#define I387 (&current->thread.fpu->fpstate->regs)
+#define I387 (&x86_task_fpu(current)->fpstate->regs)
#define FPU_info (I387->soft.info)

#define FPU_CS (*(unsigned short *) &(FPU_info->regs->cs))
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index cd73a533db69..bedbde17b536 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -111,7 +111,7 @@ static bool ex_handler_sgx(const struct exception_table_entry *fixup,

/*
* Handler for when we fail to restore a task's FPU state. We should never get
- * here because the FPU state of a task using the FPU (task->thread.fpu->state)
+ * here because the FPU state of a task using the FPU (x86_task_fpu(task)->state)
* should always be valid. However, past bugs have allowed userspace to set
* reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn().
* These caused XRSTOR to fail when switching to the task, leaking the FPU

2024-03-29 09:45:37

by Ingo Molnar

Subject: [PATCH 3/1] x86/fpu: Remove init_task FPU state dependencies, add debugging warning

* Ingo Molnar <[email protected]> wrote:

> > - exit_thread() - trivial
> >
> > - arch_dup_task_struct() clears thread.fpu, but I guess this is unnecessary
> >
> > - fpu_clone() initializes dst->thread.fpu, no longer needed
> >
> > - finally, fpu__init_system_early_generic() which initializes the
> > init_task.thread.fpu pointer.
> >
> > But this doesn't look difficult? Although I don't understand the magic in
> > arch/x86/kernel/vmlinux.lds.S ...
>
> Yeah.
>
> Something like the patch below, on top of my previous patch.
>
> The main/only fishy bit left is:
>
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -172,6 +172,10 @@ SECTIONS
> /* init_task */
> INIT_TASK_DATA(THREAD_SIZE)
>
> + __x86_init_fpu_begin = .;
> + . = __x86_init_fpu_begin + 128*PAGE_SIZE;
> + __x86_init_fpu_end = .;
>
> ... as we don't know the FPU context switch size in advance, and modern
> vector CPUs can be huge. Plus CPU vendors like to push more crap into
> the FPU context area (XSAVE area), because the rather opaque
> enumeration and variable-size format makes OS support easier for them.
> :-/
>
> And in principle init_task should never use its task-fpu context switch
> area for real, but I couldn't convince myself (yet) whether that's true
> under all circumstances - and getting this wrong would be a memory
> corruptor ...
>
> kernel_fpu_begin_mask() has this:
>
> this_cpu_write(in_kernel_fpu, true);
>
> 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));
> }
> __cpu_invalidate_fpregs_state();
>
> and the save_fpregs_to_fpstate() inside the branch should never execute
> for init_task.

So there were a few places, the patch below should cure all warnings
that I was able to trigger.

- x86_64 boots to user-space in KVM/qemu
- i386 math-emu boots to init (not sure if it works though)

Note that we weren't actually using src_fpu for anything useful in
fpu_clone() anymore.

So this now looks like something that looks useful and clean, but the
changes are quite a bit more scary than I wanted them to be ...

Thanks,

Ingo


======================>
From: Ingo Molnar <[email protected]>
Date: Fri, 29 Mar 2024 10:33:27 +0100
Subject: [PATCH] x86/fpu: Remove init_task FPU state dependencies, add debugging warning

Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/processor.h | 6 +++++-
arch/x86/kernel/fpu/core.c | 12 +++++++++---
arch/x86/kernel/fpu/init.c | 5 ++---
arch/x86/kernel/fpu/xstate.c | 3 ---
arch/x86/kernel/vmlinux.lds.S | 4 ----
5 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index fb78720774ea..d90c31e87b96 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -491,7 +491,11 @@ struct thread_struct {
#endif
};

-#define x86_task_fpu(task) ((struct fpu *)((void *)task + sizeof(*task)))
+#ifdef CONFIG_X86_DEBUG_FPU
+extern struct fpu *x86_task_fpu(struct task_struct *task);
+#else
+# define x86_task_fpu(task) ((struct fpu *)((void *)task + sizeof(*task)))
+#endif

/*
* X86 doesn't need any embedded-FPU-struct quirks:
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 0ccabcd3bf62..fdc3b227800d 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -51,6 +51,15 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
*/
DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);

+#ifdef CONFIG_X86_DEBUG_FPU
+struct fpu *x86_task_fpu(struct task_struct *task)
+{
+ WARN_ON_ONCE(task == &init_task);
+
+ return (void *)task + sizeof(*task);
+}
+#endif
+
/*
* Can we use the FPU in kernel mode with the
* whole "kernel_fpu_begin/end()" sequence?
@@ -591,10 +600,8 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
* This is safe because task_struct size is a multiple of cacheline size.
*/
struct fpu *dst_fpu = (void *)dst + sizeof(*dst);
- struct fpu *src_fpu = x86_task_fpu(current);

BUILD_BUG_ON(sizeof(*dst) % SMP_CACHE_BYTES != 0);
- BUG_ON(!src_fpu);

/* The new task's FPU state cannot be valid in the hardware. */
dst_fpu->last_cpu = -1;
@@ -657,7 +664,6 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
if (update_fpu_shstk(dst, ssp))
return 1;

- trace_x86_fpu_copy_src(src_fpu);
trace_x86_fpu_copy_dst(dst_fpu);

return 0;
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 11aa31410df2..53580e59e5db 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -38,7 +38,7 @@ static void fpu__init_cpu_generic(void)
/* Flush out any pending x87 state: */
#ifdef CONFIG_MATH_EMULATION
if (!boot_cpu_has(X86_FEATURE_FPU))
- fpstate_init_soft(&x86_task_fpu(current)->fpstate->regs.soft);
+ ;
else
#endif
asm volatile ("fninit");
@@ -164,7 +164,7 @@ static void __init fpu__init_task_struct_size(void)
* Subtract off the static size of the register state.
* It potentially has a bunch of padding.
*/
- task_size -= sizeof(x86_task_fpu(current)->__fpstate.regs);
+ task_size -= sizeof(union fpregs_state);

/*
* Add back the dynamically-calculated register state
@@ -209,7 +209,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
fpu_kernel_cfg.default_size = size;
fpu_user_cfg.max_size = size;
fpu_user_cfg.default_size = size;
- fpstate_reset(x86_task_fpu(current));
}

/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index dade16d92484..75196e4d40bd 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -844,9 +844,6 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
if (err)
goto out_disable;

- /* Reset the state for the current task */
- fpstate_reset(x86_task_fpu(current));
-
/*
* Update info used for ptrace frames; use standard-format size and no
* supervisor xstates:
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index d21331187b20..56451fd2099e 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -172,10 +172,6 @@ SECTIONS
/* init_task */
INIT_TASK_DATA(THREAD_SIZE)

- __x86_init_fpu_begin = .;
- . = __x86_init_fpu_begin + 128*PAGE_SIZE;
- __x86_init_fpu_end = .;
-
#ifdef CONFIG_X86_32
/* 32 bit has nosave before _edata */
NOSAVE_DATA

2024-03-29 15:55:50

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/fpu] x86/vm86: Make sure the free_vm86(task) definition uses its parameter even in the !CONFIG_VM86 case

The following commit has been merged into the x86/fpu branch of tip:

Commit-ID: 5ca28d24aecd0809d93da2ea73a4f6e4b2ccfa78
Gitweb: https://git.kernel.org/tip/5ca28d24aecd0809d93da2ea73a4f6e4b2ccfa78
Author: Ingo Molnar <[email protected]>
AuthorDate: Fri, 29 Mar 2024 14:20:40 +01:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Fri, 29 Mar 2024 14:24:50 +01:00

x86/vm86: Make sure the free_vm86(task) definition uses its parameter even in the !CONFIG_VM86 case

I ran into a case in new code where free_vm86(task) was the only
consumer of the 'task' variable:

arch/x86/kernel/process.c: In function ‘exit_thread’:
arch/x86/kernel/process.c:118:31: error: unused variable ‘t’ [-Werror=unused-variable]

Robustify the definition in the !CONFIG_VM86 case.

Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/include/asm/vm86.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 9e8ac50..62ee199 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -84,7 +84,7 @@ static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)

static inline void save_v86_state(struct kernel_vm86_regs *a, int b) { }

-#define free_vm86(t) do { } while(0)
+#define free_vm86(task) do { (void)(task); } while(0)

#endif /* CONFIG_VM86 */