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

Bae, Chang Seok
2021-08-13T19:44:04Z

On Aug 13, 2021, at 03:04, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Aug 13, 2021 at 08:04:54AM +0000, Bae, Chang Seok wrote:
>> Yes, each state offset in the non-compacted format is fixed in a machine
>> regardless of RFBM. So, simply take the size like that.
>
> Comment above it please.
>
> Also, why is this special case needed at all?

Without the “compacted” notion in the function name, one might call this even
with !XSAVES. But chances are very low in practice.

>> The query is intended to check whether the xstate buffer is fully expanded or
>> not -- no need to enlarge.
>>
>> If the buffer is already the maximum, the code to retrieve XSTATE_BV, this
>> call, etc should be skipped there.
>>
>> If the query is moved here, I guess this call site code becomes a bit ugly.
>
> Why does it become ugly?
>
> You simply return early without touching the buffer at all.

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.

>> No, it is still pointed by fpu->state and will be freed in the exit path.
>
> Exit path of the task?
>
> All I see is "return -ENOMEM" and no callers of alloc_xstate_buffer()
> are calling free_xstate_buffer()...
>
> And looking further into the patchset:
>
> exc_device_not_available does not call free_xstate_buffer() I'm assuming
>
> force_sig_fault(SIGILL, ILL_ILLOPC,..
>
> later will cause arch_release_task_struct() to happen which will call
> free_xstate_buffer(). Yes, no?

Yes.

> I don't see any freeing in xstateregs_set() either, so what's happening
> there when it returns -ENOMEM?
>
> I guess there we remain with the old buffer, i.e., the ptrace operation
> fails.
>
> Am I close?

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.

Thanks,
Chang