linux-kernel - Re: [PATCH v2 0/5] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2491b7c6ce97bc9f16549a5dfd15e41edf17d218.camel@intel.com>
Date: Wed, 27 Aug 2025 14:12:52 +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 Mon, 2025-08-25 at 15:47 +0200, Oleg Nesterov wrote:
> I tried to add more details in this "[PATCH v2 0/5]" cover letter, in particular
> to explain why does this series include "[PATCH v2 4/5] x86/shstk: don't create the
> shadow stack for PF_USER_WORKERs". I thought that your were asking to explain this
> part...
> 
> So. Sorry if it wasn't clear, this series is not a bug fix or something like this.
> This starts the cleanups I was thinking about year ago, see
> 
> 	https://lore.kernel.org/all/20240606120038.GB22450@redhat.com/
> 
> Then later we can probably make more changes so that the kernel threads
> (PF_KTHREADs and PF_USER_WORKERs) will run without "struct fpu" attached
> to task_struct, so that x86_task_fpu() should return NULL regardless of
> CONFIG_X86_DEBUG_FPU.

To save space?

> 
> But even the WARN_ON_ONCE(task->flags & (PF_KTHREAD|PF_USER_WORKER)) in
> x86_task_fpu() makes sense to me.
> 
> Say, switch_fpu() has no reason to check "PF_KTHREAD | PF_USER_WORKER",
> this check should die. 
> 

Digging through git, the reason for the PF_USER_WORKER check in switch_fpu() was
originally: "more of a cosmetic thing that was found while debugging and issue
and pondering why the FPU flag is set on these threads."

So a goal could be to make the code make more sense? If that is a reason, then I
have some concerns with it. The simpler code would need to somehow work with
that (I think...) user workers should still have a PKRU value. So then does
ptrace need branching logic for xstateregs_get/set() with a struct fpu and
without? If so, is that ultimately simpler?

> But if something goes wrong, it would be nice to
> have a warning for io threads as well.

I guess I question whether it really makes sense to add a special case for
PF_USER_WORKER, including the existing logic. But I'm still trying to piece
together a clearly stated benefit.

> 
> But as I said, I understand that cleanups are always subjective. It seems
> that nobody is interested, and the only reviewer (you ;) doesn't like these
> changes. I am going to give up.
> 
> That said... Could you explain why do you dislike 4/5 ?

As I said, shstk_alloc_thread_stack() shouldn't clear ARCH_SHSTK_SHSTK because
the function is about shadow stack allocation. It also doesn't make sense to
clear ARCH_SHSTK_SHSTK for user workers. It seemed like Mark (arm shadow stack
person) agreed on those...

I think Dave also questioned whether a rare extra shadow stack is really a
problem.