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.
