PF_USER_WORKERs and shadow stack

PF_USER_WORKERs and shadow stack

[Oleg Nesterov]

I know nothing about the shadow stacks, perhaps I missed something obvious.

But it seems that if a features_enabled(ARCH_SHSTK_SHSTK) thread creates a
PF_USER_WORKER thread, shstk_alloc_thread_stack() will allocate the shadow
stack for no reason.

Don't we need something like the "patch" below? PF_USER_WORKERs never return
to userspace. Note also that update_fpu_shstk() won't be called in this case.

Oleg.

--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -209,9 +209,15 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
 	 * is disabled, new_ssp will remain 0, and fpu_clone() will know not to
 	 * update it.
 	 */
-	new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
-	if (IS_ERR_VALUE(new_ssp))
-		return PTR_ERR((void *)new_ssp);
+	if (args->fn) {
+		new_ssp = 0;
+		// clear p->thread -> shstk/features,
+		// reset_thread_features() won't work
+	} else {
+		new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
+		if (IS_ERR_VALUE(new_ssp))
+			return PTR_ERR((void *)new_ssp);
+	}
 
 	fpu_clone(p, clone_flags, args->fn, new_ssp);
 

Re: PF_USER_WORKERs and shadow stack

[Dave Hansen]

On 8/13/25 09:28, Oleg Nesterov wrote:
> But it seems that if a features_enabled(ARCH_SHSTK_SHSTK) thread creates a
> PF_USER_WORKER thread, shstk_alloc_thread_stack() will allocate the shadow
> stack for no reason.

Is this costing us anything other than some CPU cycles and 160 bytes of
memory for a VMA?

Re: PF_USER_WORKERs and shadow stack

[Oleg Nesterov]

On 08/13, Dave Hansen wrote:
>
> On 8/13/25 09:28, Oleg Nesterov wrote:
> > But it seems that if a features_enabled(ARCH_SHSTK_SHSTK) thread creates a
> > PF_USER_WORKER thread, shstk_alloc_thread_stack() will allocate the shadow
> > stack for no reason.
>
> Is this costing us anything other than some CPU cycles and 160 bytes of
> memory for a VMA?

Well, I guess no, but I do have another reason for "something-like-this" cleanup.
I am working on other changes which should eliminate x86_task_fpu(PF_USER_WORKER).
Hopefully I'll send the patches tomorrow. To remind, see
https://lore.kernel.org/all/20250812125700.GA11290@redhat.com/

So I'd like to ensure that ssp_active() can't return T in ssp_get().


And... Dave, I understand that it is very easy to criticize someone else's code ;)
But - if I am right - the current logic doesn't look clean to me. Regardless.

Oleg.

Re: PF_USER_WORKERs and shadow stack

[Oleg Nesterov]

On 08/13, Oleg Nesterov wrote:
>
> On 08/13, Dave Hansen wrote:
> >
> > On 8/13/25 09:28, Oleg Nesterov wrote:
> > > But it seems that if a features_enabled(ARCH_SHSTK_SHSTK) thread creates a
> > > PF_USER_WORKER thread, shstk_alloc_thread_stack() will allocate the shadow
> > > stack for no reason.
> >
> > Is this costing us anything other than some CPU cycles and 160 bytes of
> > memory for a VMA?
>
> Well, I guess no, but I do have another reason for "something-like-this" cleanup.
> I am working on other changes which should eliminate x86_task_fpu(PF_USER_WORKER).
> Hopefully I'll send the patches tomorrow. To remind, see
> https://lore.kernel.org/all/20250812125700.GA11290@redhat.com/
>
> So I'd like to ensure that ssp_active() can't return T in ssp_get().

Sorry for noise, in case I wasn't clear...

I meant, can't return T if target->flags & PF_USER_WORKER

Oleg.

> And... Dave, I understand that it is very easy to criticize someone else's code ;)
> But - if I am right - the current logic doesn't look clean to me. Regardless.
>
> Oleg.

Re: PF_USER_WORKERs and shadow stack

[Dave Hansen]

On 8/13/25 12:14, Oleg Nesterov wrote:
> On 08/13, Dave Hansen wrote:
>> On 8/13/25 09:28, Oleg Nesterov wrote:
>>> But it seems that if a features_enabled(ARCH_SHSTK_SHSTK) thread creates a
>>> PF_USER_WORKER thread, shstk_alloc_thread_stack() will allocate the shadow
>>> stack for no reason.
>> Is this costing us anything other than some CPU cycles and 160 bytes of
>> memory for a VMA?
> Well, I guess no, but I do have another reason for "something-like-this" cleanup.
> I am working on other changes which should eliminate x86_task_fpu(PF_USER_WORKER).
> Hopefully I'll send the patches tomorrow. To remind, see
> https://lore.kernel.org/all/20250812125700.GA11290@redhat.com/

Yep, I assumed the efforts were connected.

> So I'd like to ensure that ssp_active() can't return T in ssp_get().
> 
> And... Dave, I understand that it is very easy to criticize someone else's code 😉
> But - if I am right - the current logic doesn't look clean to me. Regardless.

Hey, I'm all for having "clean" code. But if we're going to add
complexity (aka. code) to the kernel, we should know what it's getting
us other than "cleanliness".

BTW, how many PF_USER_WORKER threads _are_ there out there? I wouldn't
have thought that they were prevalent enough to justify much of an
effort here.

Re: PF_USER_WORKERs and shadow stack

[Oleg Nesterov]

On 08/13, Dave Hansen wrote:
>
> On 8/13/25 12:14, Oleg Nesterov wrote:
>
> > So I'd like to ensure that ssp_active() can't return T in ssp_get().
> >
> > And... Dave, I understand that it is very easy to criticize someone else's code 😉
> > But - if I am right - the current logic doesn't look clean to me. Regardless.
>
> Hey, I'm all for having "clean" code. But if we're going to add
> complexity (aka. code) to the kernel, we should know what it's getting
> us other than "cleanliness".
>
> BTW, how many PF_USER_WORKER threads _are_ there out there? I wouldn't
> have thought that they were prevalent enough to justify much of an
> effort here.

Agreed. I'll send the patches I have in a minute. To me they don't add
too much complexity and imo they cleanup the changed code.

But! I understand that cleanups are always subjective, so please review
and share your opinion.

Oleg.