Re: [PATCH v9 08/26] x86/fpu/xstate: Introduce helpers to manage the XSTATE buffer dynamically

[Bae, Chang Seok]

On Aug 18, 2021, at 02:28, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Fri, Aug 13, 2021 at 07:43:53PM +0000, Bae, Chang Seok wrote:
>> Without the “compacted” notion in the function name, one might
>> call this even with !XSAVES. But chances are very low in practice.
> 
> So leave only the first two which are obvious and are more likely to
> happen - the first one is going to be the most likely on non-dynamic
> setups and the second one is on dynamic systems.
> 
> For all the other configurations, just do the loop and that's it.
> 
> *IF* an optimization needs to happen there, then it can happen latter,
> supplied with perf numbers to justify it.

No, this non-compacted thing is not for optimization. SDM is not quite clear
about the logic behind the non-compacted format -- some state’s offset does
not always match with the 'size + offset' of the previous one, even without
64B-alignment. So, the loop is only for the compacted format, not the
non-compacted one. 

It was refactored to use in the new helper to find feature_nr’s start point.
If the size is added up here, it is not ‘i’'s start point anymore.

>> Perhaps, the call site in the ptrace path becomes like this, I think:
>> 
>> +	if (xfeatures_mask_user_dynamic) {
>> +		u64 state_mask;
>> +
>> +		/* Retrieve XSTATE_BV. */
>> +		memcpy(&state_mask, (kbuf ?: tmpbuf) + offsetof(struct xregs_state, header),
>> +		       sizeof(u64));
>> +
>> +		/* Expand the xstate buffer based on the XSTATE_BV. */
>> +		ret = realloc_xstate_buffer(fpu, state_mask & xfeatures_mask_user_dynamic);
>> +		if (ret)
>> +			goto out;
>> +	}
>> 
>> Maybe retrieve XSTATE_BV is inevitable here. Then, it is not that ugly.
> 
> Lemme see if I can follow: here, a task is being ptraced and the tracer
> process does PTRACE_SETREGS to set the xregs and you want to go and read
> out the XSTATE_BV vector from the supplied xstate buffer to see how much
> to enlarge the buffer.
> 
> Which makes me go, whut?
> 
> Why doesn't the task already have a large enough buffer?
> 
> IOW and IIUC, you should not have to ever resize the xstate buffer of a
> task in ptrace.

Sorry, it looks like I missed adding the permission check in the above.

[ I saw the discussion has (re-)started for the allocation API though, assume
  that the resize happens transparently for now. ]

Saying the ptracee has never used AMX -- it has a small buffer. Then, if the
ptracer attempts to inject tile data, and the buffer is resized here, it
fails. This precludes AMX state injection as allowed only after the ptracee
ever used the state. My concern is it may make it confusing to ptrace users at
least.

>> In this case, the ptracer just failed to inject some context. But the
>> ptracee’s context in the (old) buffer is intact. It will resume and eventually
>> exit. I think arch_release_task_struct()->free_xstate_buffer() will take care
>> of the old buffer.
> 
> You think or you know?
> 
> How about verifying it.

Let me consider a case to mimic the situation somehow.

Thanks,
Chang