Re: [PATCH 5/6] x86/shstk: don't create the shadow stack for PF_USER_WORKERs

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

On Fri, 2025-08-15 at 14:17 +0200, Oleg Nesterov wrote:
> > but I'm not sure
> > it is right to fully disable shadow stack in thread.features.
> 
> Why?

The bit in thread.features is like a sticky bit that is inherrited whenver a
thread is cloned. How it works normally is that the first thread in the app
(really glibc loader) enables shadow stacks, then new threads automatically
inherit that shadow stack is enabled. So in practice it is like a process wide
thing, but stored on each thread. This process-wide behavior is to add to the
security. You don't want to allow a protected app to spawn a new thread that
escapes the enforcement. There is a way to manually disable shadow stack per-
thread, but it is protected by ARCH_SHSTK_LOCK, which gets set by glibc loader
before jumping into the actual app.

When shadow stack is enabled, depending on the circumstances a new shadow stack
will automatically be allocated for a new thread. shstk->base and shstk->size
are about that automatically enabled shadow stack.

So what are we trying to do for PF_USER_WORKER? Prevent them from wasting a VMA
with an unused shadow stack? Or set PF_USER_WORKER's aside from the logic that
is about more than protecting the individual thread in the process?

> 
> > First of all,
> > disabling it from shstk_alloc_thread_stack() seems weird. It just handles
> > allocating shadow stacks.
> 
> I agree in advance with any other change.
> 
> > Lastly, it doesn't seem there is any way to clone from IO uring today,
> 
> Not sure I understand... create_io_thread() ?

There was some discussion in the past about adding a clone, but the comment was
more about whether it fit the concepts in involved.

https://lwn.net/Articles/908268/

> 
> > How about just adding the 'minimal' condition to:
> >  	if (clone_flags & CLONE_VFORK) {
> >  		shstk->base = 0;
> >  		shstk->size = 0;
> >  		return 0;
> >  	}
> > ...then update all the comments where vfork is called out as the only case
> > that
> > does this?
> 
> but create_io_thread() and vhost_task_create() do not use CLONE_VFORK?

No, to make it have the same logic as the vfork case (which doesn't allocate a
new shadow stack).

Like:
 	if ((clone_flags & CLONE_VFORK) || minimal) {
 		shstk->base = 0;
 		shstk->size = 0;
 		return 0;
 	}

Or as Mark was suggesting, replace it with something like:
 	if (needs_new_shstk(clone_args)) {
 		shstk->base = 0;
 		shstk->size = 0;
 		return 0;
 	}