Re: [PATCH v27 24/31] x86/cet/shstk: Handle thread shadow stack

On 7/21/21 11:14 AM, John Allen wrote:

quoted

+int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags,
+			     unsigned long stack_size)
+{
+	struct thread_shstk *shstk = &tsk->thread.shstk;
+	struct cet_user_state *state;
+	unsigned long addr;
+
+	if (!stack_size)
+		return -EINVAL;

I've been doing some light testing on AMD hardware and I've found that
this version of the patchset doesn't boot for me. It appears that when
systemd processes start spawning, they hit the above case, return
-EINVAL, and the fork fails. In these cases, copy_thread has been passed
0 for both sp and stack_size.

A few tangential things I noticed:

This hunk is not mentioned in the version changelog at all.  I also
don't see any feedback that might have prompted it.  This is one reason
per-patch changelogs are preferred.

As a general rule, new features should strive to be implemented in a way
that it's *obvious* that they won't break old code.
shstk_alloc_thread_stack() fails that test for me.  If it had:

	if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) // or whatever
		return 0;

in the function, it would be obviously harmless.  Better yet would be
doing the feature check at the shstk_alloc_thread_stack() call site,
that way even the function call can be optimized out.

Further, this confused me because the changelog didn't even mention the
arg -> stack_size rename.  That would have been nice for another patch,
or an extra sentence in the changelog.