[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e653fb9cab51ed2d0ea71f9d322c55420a83a4f5.camel@intel.com>
Date: Tue, 2 Sep 2025 20:37:12 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "oleg@...hat.com" <oleg@...hat.com>
CC: "debug@...osinc.com" <debug@...osinc.com>, "mingo@...nel.org"
<mingo@...nel.org>, "bp@...en8.de" <bp@...en8.de>, "broonie@...nel.org"
<broonie@...nel.org>, "peterz@...radead.org" <peterz@...radead.org>,
"hpa@...or.com" <hpa@...or.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "Mehta, Sohil"
<sohil.mehta@...el.com>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v2 0/5] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER)
in .regset_get() paths
On Fri, 2025-08-29 at 17:06 +0200, Oleg Nesterov wrote:
> > *If* we want to worry about an extra shadow stack allocation (which Dave
> > seems to doubt), we don't need to clear ARCH_SHSTK_SHSTK to avoid
> > allocations.
> > Other thread types already avoid it (vfork, etc). So just add to the
> > existing logic that skips shadow stack allocation. Make it do that for user
> > workers too, and leave ARCH_SHSTK_SHSTK alone.
>
> From 0/5:
>
> However, there is an annoying complication:
> shstk_alloc_thread_stack()
> can alloc the pointless shadow stack for PF_USER_WORKER thread and
> set
> the ARCH_SHSTK_SHSTK flag. This means that ssp_get()->ssp_active()
> can
> return true, and in this case it wouldn't be right to use the
> "unrelated"
> init_fpstate.
Yea the ptrace code currently assumes there will be a non-init SHSTK FPU state.
But if the init stateĀ is currently associated with the FPU, whether it's via a
cleared copy, or some pointer redirection as you proposed, what is the
difference?
Hmm, I actually do see a potential concrete issue...
fpu_clone() will wipe out the FPU state for PF_USER_WORKER, which means if
xsaves decides to use the init optimization for CET, "get_xsave_addr(xsave,
XFEATURE_CET_USER)" could return NULL and trigger a warning. I would think we
could address this by just removing the warning, since the comment is incorrect.
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 0986c2200adc..094a891bfea8 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -196,15 +196,8 @@ int ssp_get(struct task_struct *target, const struct
user_regset *regset,
sync_fpstate(fpu);
cetregs = get_xsave_addr(&fpu->fpstate->regs.xsave, XFEATURE_CET_USER);
- if (WARN_ON(!cetregs)) {
- /*
- * This shouldn't ever be NULL because shadow stack was
- * verified to be enabled above. This means
- * MSR_IA32_U_CET.CET_SHSTK_EN should be 1 and so
- * XFEATURE_CET_USER should not be in the init state.
- */
+ if (cetregs)
return -ENODEV;
- }
return membuf_write(&to, (unsigned long *)&cetregs->user_ssp,
sizeof(cetregs->user_ssp));
@@ -241,15 +234,8 @@ int ssp_set(struct task_struct *target, const struct
user_regset *regset,
fpu_force_restore(fpu);
cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
- if (WARN_ON(!cetregs)) {
- /*
- * This shouldn't ever be NULL because shadow stack was
- * verified to be enabled above. This means
- * MSR_IA32_U_CET.CET_SHSTK_EN should be 1 and so
- * XFEATURE_CET_USER should not be in the init state.
- */
+ if (cetregs)
return -ENODEV;
- }
cetregs->user_ssp = user_ssp;
return 0;
If PF_USER_WORKER's ever do grow the ability to spawn threads, further changes
would be needed to restore CET_SHSTK_EN for the new thread. I actually think
this is a further point towards not having special logic for PF_USER_WORKER FPUs
(beyond the PKRU reasoning). As in, instead of making these proposed changes,
instead rollback the existing differences. But I'm not sure it's worth it at
this time.
Powered by blists - more mailing lists