Messages in this thread | | | Subject | Re: [PATCH v27 24/31] x86/cet/shstk: Handle thread shadow stack | From | Dave Hansen <> | Date | Wed, 21 Jul 2021 11:37:59 -0700 |
| |
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.
| |