Currently, saving register states in the signal frame, the legacy feature
bits are always set in xregs_state->header->xfeatures. This code sequence
can be generalized for reuse in similar cases.
Refactor the logic to ensure a consistent approach across similar usages.
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
---
Changes from the last posting:
https://lore.kernel.org/lkml/20250214010607.7067-2-chang.seok.bae@intel.com/
* No change
This patch and the next were previously posted together. I thought this
refactoring is a meaningful step toward decoupling PKRU from an
unnecessary dependency on XGETBV(1).
---
arch/x86/kernel/fpu/signal.c | 11 +----------
arch/x86/kernel/fpu/xstate.h | 12 ++++++++++++
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index b8b4fa9c2d04..c3ec2512f2bb 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -114,7 +114,6 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
{
struct xregs_state __user *x = buf;
struct _fpx_sw_bytes sw_bytes = {};
- u32 xfeatures;
int err;
/* Setup the bytes not touched by the [f]xsave and reserved for SW. */
@@ -127,12 +126,6 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
err |= __put_user(FP_XSTATE_MAGIC2,
(__u32 __user *)(buf + fpstate->user_size));
- /*
- * Read the xfeatures which we copied (directly from the cpu or
- * from the state in task struct) to the user buffers.
- */
- err |= __get_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
/*
* For legacy compatible, we always set FP/SSE bits in the bit
* vector while saving the state to the user context. This will
@@ -144,9 +137,7 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
* header as well as change any contents in the memory layout.
* xrestore as part of sigreturn will capture all the changes.
*/
- xfeatures |= XFEATURE_MASK_FPSSE;
-
- err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
+ err |= set_xfeature_in_sigframe(x, XFEATURE_MASK_FPSSE);
return !err;
}
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 9a3a8ccf13bf..aadf02aed071 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -69,6 +69,18 @@ static inline u64 xfeatures_mask_independent(void)
return fpu_kernel_cfg.independent_features;
}
+static inline int set_xfeature_in_sigframe(struct xregs_state __user *xbuf, u64 mask)
+{
+ u64 xfeatures;
+ int err;
+
+ /* Read the xfeatures value already saved in the user buffer */
+ err = __get_user(xfeatures, &xbuf->header.xfeatures);
+ xfeatures |= mask;
+ err |= __put_user(xfeatures, &xbuf->header.xfeatures);
+ return err;
+}
+
/*
* Update the value of PKRU register that was already pushed onto the signal frame.
*/
--
2.45.2
* Chang S. Bae <chang.seok.bae@intel.com> wrote:
> +static inline int set_xfeature_in_sigframe(struct xregs_state __user *xbuf, u64 mask)
> +{
> + u64 xfeatures;
> + int err;
> +
> + /* Read the xfeatures value already saved in the user buffer */
> + err = __get_user(xfeatures, &xbuf->header.xfeatures);
> + xfeatures |= mask;
> + err |= __put_user(xfeatures, &xbuf->header.xfeatures);
> + return err;
> +}
For future reference, please put an extra newline before 'return'
statements, so that the code looks more 'balanced':
> +{
> + u64 xfeatures;
> + int err;
> +
> + /* Read the xfeatures value already saved in the user buffer */
> + err = __get_user(xfeatures, &xbuf->header.xfeatures);
> + xfeatures |= mask;
> + err |= __put_user(xfeatures, &xbuf->header.xfeatures);
> +
> + return err;
> +}
I've done that for this patch, so no need to resend it.
Thanks,
Ingo
On 4/16/2025 1:05 AM, Ingo Molnar wrote:
>
> * Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
>> +static inline int set_xfeature_in_sigframe(struct xregs_state __user *xbuf, u64 mask)
>> +{
>> + u64 xfeatures;
>> + int err;
>> +
>> + /* Read the xfeatures value already saved in the user buffer */
>> + err = __get_user(xfeatures, &xbuf->header.xfeatures);
>> + xfeatures |= mask;
>> + err |= __put_user(xfeatures, &xbuf->header.xfeatures);
>> + return err;
>> +}
>
> For future reference, please put an extra newline before 'return'
> statements, so that the code looks more 'balanced':
>
>> +{
>> + u64 xfeatures;
>> + int err;
>> +
>> + /* Read the xfeatures value already saved in the user buffer */
>> + err = __get_user(xfeatures, &xbuf->header.xfeatures);
>> + xfeatures |= mask;
>> + err |= __put_user(xfeatures, &xbuf->header.xfeatures);
>> +
>> + return err;
>> +}
I see. Thanks for the note.
> I've done that for this patch, so no need to resend it.
Thanks!
Chang
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 64e54461ab6e8524a8de4e63b7d1a3e4481b5cf3
Gitweb: https://git.kernel.org/tip/64e54461ab6e8524a8de4e63b7d1a3e4481b5cf3
Author: Chang S. Bae <chang.seok.bae@intel.com>
AuthorDate: Tue, 15 Apr 2025 19:16:57 -07:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 16 Apr 2025 10:01:00 +02:00
x86/fpu: Refactor xfeature bitmask update code for sigframe XSAVE
Currently, saving register states in the signal frame, the legacy feature
bits are always set in xregs_state->header->xfeatures. This code sequence
can be generalized for reuse in similar cases.
Refactor the logic to ensure a consistent approach across similar usages.
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20250416021720.12305-8-chang.seok.bae@intel.com
---
arch/x86/kernel/fpu/signal.c | 11 +----------
arch/x86/kernel/fpu/xstate.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index b8b4fa9..c3ec251 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -114,7 +114,6 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
{
struct xregs_state __user *x = buf;
struct _fpx_sw_bytes sw_bytes = {};
- u32 xfeatures;
int err;
/* Setup the bytes not touched by the [f]xsave and reserved for SW. */
@@ -128,12 +127,6 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
(__u32 __user *)(buf + fpstate->user_size));
/*
- * Read the xfeatures which we copied (directly from the cpu or
- * from the state in task struct) to the user buffers.
- */
- err |= __get_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
-
- /*
* For legacy compatible, we always set FP/SSE bits in the bit
* vector while saving the state to the user context. This will
* enable us capturing any changes(during sigreturn) to
@@ -144,9 +137,7 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
* header as well as change any contents in the memory layout.
* xrestore as part of sigreturn will capture all the changes.
*/
- xfeatures |= XFEATURE_MASK_FPSSE;
-
- err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures);
+ err |= set_xfeature_in_sigframe(x, XFEATURE_MASK_FPSSE);
return !err;
}
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 9a3a8cc..4231e44 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -69,6 +69,19 @@ static inline u64 xfeatures_mask_independent(void)
return fpu_kernel_cfg.independent_features;
}
+static inline int set_xfeature_in_sigframe(struct xregs_state __user *xbuf, u64 mask)
+{
+ u64 xfeatures;
+ int err;
+
+ /* Read the xfeatures value already saved in the user buffer */
+ err = __get_user(xfeatures, &xbuf->header.xfeatures);
+ xfeatures |= mask;
+ err |= __put_user(xfeatures, &xbuf->header.xfeatures);
+
+ return err;
+}
+
/*
* Update the value of PKRU register that was already pushed onto the signal frame.
*/© 2016 - 2025 Red Hat, Inc.