# Re: \[PATCH v28 25/32\] x86/cet/shstk: Handle thread shadow stack

- From: Yu, Yu-cheng \<hidden\>
- Date: 2021-07-23 17:30:42 
- List: linux-doc
- Page: https://ratatoskr.run/linux-doc/2021/07/1456181
- Thread: https://ratatoskr.run/linux-doc/2021/07/1455208/t.md

```text
On 7/22/2021 2:05 PM, Dave Hansen wrote:
> On 7/22/21 1:52 PM, Yu-cheng Yu wrote:
>> +	if (!stack_size)
>> +		stack_size = min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G);
>> +
>> +	if (!shstk->size)
>> +		return 0;
>> +
>> +	/*
>> +	 * For CLONE_VM, except vfork, the child needs a separate shadow
>> +	 * stack.
>> +	 */
>> +	if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM)
>> +		return 0;
>> +
>> +	/*
>> +	 * This is in clone() syscall and fpu__copy() already copies xstates
>> +	 * from the parent.  If get_xsave_addr() returns null, then XFEATURE_
>> +	 * CET_USER is still in init state, which certainly is an error.
>> +	 */
>> +	state = get_xsave_addr(&tsk->thread.fpu.state.xsave, XFEATURE_CET_USER);
>> +	if (!state)
>> +		return -EINVAL;
>
> I don't care much for that comment.
>
> This code is meant to copy shadow stack config information into children
> when it is already enabled.  We *just* checked for that above in the
> "shstk->size" check.  The fact that this is called from clone() is
> irrelevant.  The shadow stack enabling status *is*.
>
> I think I'd rather this be more along the lines of:
>
> 	/*
> 	 * 'tsk' is configured with a shadow stack and the fpu.state is
> 	 * up to date since it was just copied from the parent.  There
> 	 * must be a valid non-init CET state location in the buffer.
> 	 */
>
> There is also a strong enough assumption violation that I'd probably
> WARN() in addition to returning -EINVAL.
>

Yes, I will update the comment and put in the WARN().

Thanks,
Yu-cheng
```