Message-ID: <7819c425-5792-4cc5-96aa-9c8b012f1a06@intel.com>
Date: Thu, 7 Nov 2024 07:56:56 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Rudi Horn <rudi.horn@...cle.com>,
 Aruna Ramakrishna <aruna.ramakrishna@...cle.com>,
 Thomas Gleixner <tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "x86@...nel.org" <x86@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc: Joe Jin <joe.jin@...cle.com>, Jeff Xu <jeffxu@...omium.org>
Subject: Re: [RFC] Restore PKRU to user-defined value after signal handling

On 11/7/24 03:41, Rudi Horn wrote:
>> But the suggested fix is just beyond hideous. 
> 
> I am new to the kernel mailing list, but I would like to refer to 
> https://subspace.kernel.org/etiquette.html#be-terse-but-polite.

Rudi,

I'd also call your attention to another section of that document that
you quoted:

https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying

Aruna, my apologies for referring to the code that you wrote the way
that I did.  It's not an excuse, but

>> It's yet another reason that the XSAVE architecture complexity hurts more than it helps.
> 
> The XSTATE architecture simply guarantees that an XRSTR using the state 
> recorded by an XSAVE yields the same processor state. It can prevent dirtying
> cache lines by specifying which processor state can just be restored to its 
> zero'd state. All we are doing with this change, is maintaining the invariant that
> the xfeatures value matches the remaining data stored in the xstate buffer.

I appreciate the information about the XSAVE architecture!  That's
certainly a slightly different perspective than I was considering.

I was actually trying to bolster the argument that the XSAVE
architecture really isn't working out well and should be replaced with
something new for new features.

>> Can't we just use the mask that xsave_to_user_sigframe() generated instead
>> of reading it back out of userspace three seconds after it is written?
> 
> This is technically sound, but at this point in the code it is making the
> assumption that xsave_to_user_sigframe() currently matches the value stored
> in the xstate. This is more fragile if any further changes to the xstate are made.

Tell me more, please.  What changes to the XSAVE area are you concerned
about?  There is currently vanishingly little code between the XSAVE and
overwriting the PKRU state.

> However, we could consider the the kernels representation of XSTATE to be 
> a pointer to the xsave buffer and a kernel value of the xfeature field. The
> xsave_to_user_sigframe() would then compute and return the xfeatures 
> field as you requested, and the xfeatures value should written to the xsave buffer 
> at the end of copy_fpregs_to_sigframe, possibly being conditional on if it was dirtied.
> 
> Then update_pkru_in_sigframe then just becomes:
> /*
>  * Update the value of PKRU register that was already pushed onto the signal frame.
>  */
> static inline int update_pkru_in_sigframe(struct xregs_state __user *buf,
> 					  u32 *xfeatures, u32 pkru)
> {
> 	if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
> 		return 0;
> 
> 	if (pkru != 0) {
> 		/* Mark PKRU XSTATE section as in use. */
> 		*xfeatures |= XFEATURE_MASK_PKRU;
> 
> 		return __put_user(pkru,
> 				  (unsigned int __user *)get_xsave_addr_user(
> 					  buf, XFEATURE_PKRU));
> 	}
> 
> 	return 0;
> }
> 
> I think maintaining this invariant is a fair step to perform when fiddling
> with somewhat architecture-internal data structures.

That version seems to me like it would simply punt the complexity of
updating XSTATE_BV (aka xfeatures) to other code.

The question still stands as to whether the new XSTATE_BV value should
be calculated by the kernel or read directly from the userspace buffer.

Powered by blists - more mailing lists