Re: [PATCH v2 0/5] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Wed, 2025-08-27 at 16:51 +0200, Oleg Nesterov wrote:
> > 
> > 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.
> 
> Again, I don't understand... To me, currently arch/x86/kernel/fpu/regset.c
> adds a special case for PF_USER_WORKER, this series tries to remove it (but
> we need a bit more of simple changes).

That commit I dug up? It didn't have a super strong justification either. Can
you say what your intended benefit is?

> 
> > > 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.
> 
> OK, then how/where we can clear this flag if we avoid the pointless shadow
> stack allocation for PF_USER_WORKER?

*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.

> 
> > It also doesn't make sense to clear ARCH_SHSTK_SHSTK for user workers.
> 
> Why?

Because ARCH_SHSTK_SHSTK is supposed to be inherited by children. It adds a
special case for no reason.

> 
> > I think Dave also questioned whether a rare extra shadow stack is really a
> > problem.
> 
> Sure, it is not really a problem. In that it is not a bug. But why we can't
> avoid the pointless shadow stack / ARCH_SHSTK_SHSTK for user workers ? 4/5
> doesn't complicate this code.
> 
> Plus, again, the current code is not consistent. fpu_clone() won't do
> update_fpu_shstk() in this case. Not a bug too, but imo deserves a cleanup.

I thought we discussed that the user worker logic already wipes the whole FPU
state though, so we don't need to call update_fpu_shstk(). Did I get that wrong?