| Message ID | 20220929222936.14584-37-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadowstacks for userspace | expand |
On 9/29/2022 3:29 PM, Rick Edgecombe wrote: > If an xfeature is saved in a buffer, the xfeature's bit will be set in > xsave->header.xfeatures. The CPU may opt to not save the xfeature if it > is in it's init state. In this case the xfeature buffer address cannot > be retrieved with get_xsave_addr(). > > Future patches will need to handle the case of writing to an xfeature > that may not be saved. So provide helpers to init an xfeature in an > xsave buffer. > > This could of course be done directly by reaching into the xsave buffer, > however this would not be robust against future changes to optimize the > xsave buffer by compacting it. In that case the xsave buffer would need > to be re-arranged as well. So the logic properly belongs encapsulated > in a helper where the logic can be unified. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > v2: > - New patch > > arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++------- > arch/x86/kernel/fpu/xstate.h | 6 ++++ > 2 files changed, 53 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index 9258fc1169cc..82cee1f2f0c8 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -942,6 +942,24 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr) > return (void *)xsave + xfeature_get_offset(xcomp_bv, xfeature_nr); > } > > +static int xsave_buffer_access_checks(int xfeature_nr) > +{ > + /* > + * Do we even *have* xsave state? > + */ > + if (!boot_cpu_has(X86_FEATURE_XSAVE)) > + return 1; > + > + /* > + * We should not ever be requesting features that we > + * have not enabled. > + */ > + if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr))) > + return 1; > + > + return 0; > +} > + > /* > * Given the xsave area and a state inside, this function returns the > * address of the state. > @@ -962,17 +980,7 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr) > */ > void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr) > { > - /* > - * Do we even *have* xsave state? > - */ > - if (!boot_cpu_has(X86_FEATURE_XSAVE)) > - return NULL; > - > - /* > - * We should not ever be requesting features that we > - * have not enabled. > - */ > - if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr))) > + if (xsave_buffer_access_checks(xfeature_nr)) > return NULL; > > /* > @@ -992,6 +1000,34 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr) > return __raw_xsave_addr(xsave, xfeature_nr); > } > > +/* > + * Given the xsave area and a state inside, this function > + * initializes an xfeature in the buffer. But, this function sets XSTATE_BV bits in the buffer. That does not *initialize* the state, right? > + * > + * get_xsave_addr() will return NULL if the feature bit is > + * not present in the header. This function will make it so > + * the xfeature buffer address is ready to be retrieved by > + * get_xsave_addr(). Looks like this is used in the next patch to help ptracer(). We have the state copy function -- copy_uabi_to_xstate() that retrieves the address using __raw_xsave_addr() instead of get_xsave_addr(), copies the state, and then updates XSTATE_BV. __raw_xsave_addr() also ensures whether the state is in the compacted format or not. I think you can use it. Also, I'm curious about the reason why you want to update XSTATE_BV first with this new helper. Overall, I'm not sure these new helpers are necessary. Thanks, Chang
On Mon, 2022-10-03 at 12:07 -0700, Chang S. Bae wrote: > > +/* > > + * Given the xsave area and a state inside, this function > > + * initializes an xfeature in the buffer. > > But, this function sets XSTATE_BV bits in the buffer. That does not > *initialize* the state, right? No, it doesn't actually write out the init state to the buffer. > > > + * > > + * get_xsave_addr() will return NULL if the feature bit is > > + * not present in the header. This function will make it so > > + * the xfeature buffer address is ready to be retrieved by > > + * get_xsave_addr(). > > Looks like this is used in the next patch to help ptracer(). > > We have the state copy function -- copy_uabi_to_xstate() that > retrieves > the address using __raw_xsave_addr() instead of get_xsave_addr(), > copies > the state, and then updates XSTATE_BV. > > __raw_xsave_addr() also ensures whether the state is in the > compacted > format or not. I think you can use it. > > Also, I'm curious about the reason why you want to update XSTATE_BV > first with this new helper. > > Overall, I'm not sure these new helpers are necessary. Thomas had experimented with this optimization where init state features weren't saved: https://lore.kernel.org/lkml/20220404103741.809025935@linutronix.de/ It made me think non-fpu code should not assume things about the state of the buffer, as FPU code might have to move things when initing them. So the operation is worth centralizing in a helper. I think you are right, today it is not doing much and could be open coded. I guess the question is, should it be open coded or centralized? I'm fine either way.
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 9258fc1169cc..82cee1f2f0c8 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -942,6 +942,24 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr) return (void *)xsave + xfeature_get_offset(xcomp_bv, xfeature_nr); } +static int xsave_buffer_access_checks(int xfeature_nr) +{ + /* + * Do we even *have* xsave state? + */ + if (!boot_cpu_has(X86_FEATURE_XSAVE)) + return 1; + + /* + * We should not ever be requesting features that we + * have not enabled. + */ + if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr))) + return 1; + + return 0; +} + /* * Given the xsave area and a state inside, this function returns the * address of the state. @@ -962,17 +980,7 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr) */ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr) { - /* - * Do we even *have* xsave state? - */ - if (!boot_cpu_has(X86_FEATURE_XSAVE)) - return NULL; - - /* - * We should not ever be requesting features that we - * have not enabled. - */ - if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr))) + if (xsave_buffer_access_checks(xfeature_nr)) return NULL; /* @@ -992,6 +1000,34 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr) return __raw_xsave_addr(xsave, xfeature_nr); } +/* + * Given the xsave area and a state inside, this function + * initializes an xfeature in the buffer. + * + * get_xsave_addr() will return NULL if the feature bit is + * not present in the header. This function will make it so + * the xfeature buffer address is ready to be retrieved by + * get_xsave_addr(). + * + * Inputs: + * xstate: the thread's storage area for all FPU data + * xfeature_nr: state which is defined in xsave.h (e.g. XFEATURE_FP, + * XFEATURE_SSE, etc...) + * Output: + * 1 if the feature cannot be inited, 0 on success + */ +int init_xfeature(struct xregs_state *xsave, int xfeature_nr) +{ + if (xsave_buffer_access_checks(xfeature_nr)) + return 1; + + /* + * Mark the feature inited. + */ + xsave->header.xfeatures |= BIT_ULL(xfeature_nr); + return 0; +} + #ifdef CONFIG_ARCH_HAS_PKEYS /* diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h index 5ad47031383b..fb8aae678e9f 100644 --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -54,6 +54,12 @@ extern void fpu__init_cpu_xstate(void); extern void fpu__init_system_xstate(unsigned int legacy_size); extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr); +extern int init_xfeature(struct xregs_state *xsave, int xfeature_nr); + +static inline int xfeature_saved(struct xregs_state *xsave, int xfeature_nr) +{ + return xsave->header.xfeatures & BIT_ULL(xfeature_nr); +} static inline u64 xfeatures_mask_supervisor(void) {
If an xfeature is saved in a buffer, the xfeature's bit will be set in xsave->header.xfeatures. The CPU may opt to not save the xfeature if it is in it's init state. In this case the xfeature buffer address cannot be retrieved with get_xsave_addr(). Future patches will need to handle the case of writing to an xfeature that may not be saved. So provide helpers to init an xfeature in an xsave buffer. This could of course be done directly by reaching into the xsave buffer, however this would not be robust against future changes to optimize the xsave buffer by compacting it. In that case the xsave buffer would need to be re-arranged as well. So the logic properly belongs encapsulated in a helper where the logic can be unified. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v2: - New patch arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++------- arch/x86/kernel/fpu/xstate.h | 6 ++++ 2 files changed, 53 insertions(+), 11 deletions(-)