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

[Bae, Chang Seok]

On Aug 13, 2021, at 03:04, Borislav Petkov <bp@xxxxxxxxx> 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