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

[Dave Hansen]

On 7/21/21 11:14 AM, John Allen wrote:
>> +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.