Re: [PATCH 02/10] x86/fpu: Clean up and rename variables in signal frame handling

Am Mo., 15. Juni 2026 um 21:38 Uhr schrieb Andrei Vagin <[email protected]>:
>
> Clean up signal frame handling code by renaming several variables for
> clarity and consistency, and moving masking logic closer to its usage.
>
> - Rename 'fxbuf' to 'buf_fx' in check_xstate_in_sigframe() for consistency.
> - Rename label 'setfx' to 'err_setfx' in check_xstate_in_sigframe() to
>   indicate it is an error path.
> - In __restore_fpregs_from_user(), rename 'ufeatures' to 'task_xfeatures'
>   and 'xrestore' to 'xrestore_mask'.
> - Move the masking logic 'xrestore_mask &= task_xfeatures' from
>   restore_fpregs_from_user() into __restore_fpregs_from_user().
> - Rename 'xrestore' to 'xrestore_mask' in restore_fpregs_from_user() to
>   match the name in __restore_fpregs_from_user() and __fpu_restore_sig().
> - In __fpu_restore_sig(), rename 'buf' to 'buf_f' to distinguish it from
>   'buf_fx', and 'user_xfeatures' to 'xrestore_mask'.
>
> No functional changes.
>
> Suggested-by: Ingo Molnar <[email protected]>
> Signed-off-by: Andrei Vagin <[email protected]>

Reviewed-by: Alexander Mikhalitsyn <[email protected]>

> ---
>  arch/x86/kernel/fpu/signal.c | 40 ++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 20b638c507ca..42c3d78bd849 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -24,15 +24,15 @@
>   * Check for the presence of extended state information in the
>   * user fpstate pointer in the sigcontext.
>   */
> -static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
> +static inline bool check_xstate_in_sigframe(struct fxregs_state __user *buf_fx,
>                                             struct _fpx_sw_bytes *fx_sw)
>  {
>         int min_xstate_size = sizeof(struct fxregs_state) +
>                               sizeof(struct xstate_header);
> -       void __user *fpstate = fxbuf;
> +       void __user *fpstate = buf_fx;
>         unsigned int magic2;
>
> -       if (__copy_from_user(fx_sw, &fxbuf->sw_reserved[0], sizeof(*fx_sw)))
> +       if (__copy_from_user(fx_sw, &buf_fx->sw_reserved[0], sizeof(*fx_sw)))
>                 return false;
>
>         /* Check for the first magic field and other error scenarios. */
> @@ -40,7 +40,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
>             fx_sw->xstate_size < min_xstate_size ||
>             fx_sw->xstate_size > x86_task_fpu(current)->fpstate->user_size ||
>             fx_sw->xstate_size > fx_sw->extended_size)
> -               goto setfx;
> +               goto err_setfx;
>
>         /*
>          * Check for the presence of second magic word at the end of memory
> @@ -53,7 +53,7 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
>
>         if (likely(magic2 == FP_XSTATE_MAGIC2))
>                 return true;
> -setfx:
> +err_setfx:
>         trace_x86_fpu_xstate_check_failed(x86_task_fpu(current));
>
>         /* Set the parameters for fx only state */
> @@ -240,15 +240,17 @@ bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size, u
>         return true;
>  }
>
> -static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
> -                                     u64 xrestore, bool fx_only)
> +static int __restore_fpregs_from_user(void __user *buf, u64 task_xfeatures,
> +                                     u64 xrestore_mask, bool fx_only)
>  {
>         if (use_xsave()) {
> -               u64 init_bv = ufeatures & ~xrestore;
> +               u64 init_bv = task_xfeatures & ~xrestore_mask;
>                 int ret;

We had an off-list discussion about this place with Andrei. Here we
implicitly assume that:

[new code]
init_bv = task_xfeatures & ~xrestore_mask
xrestore_mask &= task_xfeatures

is equivalent to:

[old code]
xrestore_mask &= task_xfeatures
init_bv = task_xfeatures & ~xrestore_mask

For xrestore_mask is is obvious.
For init_bv, it may be not that obvious.

If we consider a function bv(x, y) = x & ~y, then we want to ensure
that the following properly holds:
bv(x, y) = bv(x, (y & x)).

bv(x, (y & x)) = x & ~(y & x) = x & (~y | ~x) = (x & ~y) | (x & ~x) =
bv(x, y) | 0 = bv(x, y).

>
> +               /* Restore enabled features only. */
> +               xrestore_mask &= task_xfeatures;
>                 if (likely(!fx_only))
> -                       ret = xrstor_from_user_sigframe(buf, xrestore);
> +                       ret = xrstor_from_user_sigframe(buf, xrestore_mask);
>                 else
>                         ret = fxrstor_from_user_sigframe(buf);
>
> @@ -266,20 +268,18 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
>   * Attempt to restore the FPU registers directly from user memory.
>   * Pagefaults are handled and any errors returned are fatal.
>   */
> -static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
> +static bool restore_fpregs_from_user(void __user *buf, u64 xrestore_mask, bool fx_only)
>  {
>         struct fpu *fpu = x86_task_fpu(current);
>         int ret;
>
> -       /* Restore enabled features only. */
> -       xrestore &= fpu->fpstate->user_xfeatures;
>  retry:
>         fpregs_lock();
>         /* Ensure that XFD is up to date */
>         xfd_update_state(fpu->fpstate);
>         pagefault_disable();
>         ret = __restore_fpregs_from_user(buf, fpu->fpstate->user_xfeatures,
> -                                        xrestore, fx_only);
> +                                        xrestore_mask, fx_only);
>         pagefault_enable();
>
>         if (unlikely(ret)) {
> @@ -324,7 +324,7 @@ static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_onl
>         return true;
>  }
>
> -static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
> +static bool __fpu_restore_sig(void __user *buf_f, void __user *buf_fx,
>                               bool ia32_fxstate)
>  {
>         struct task_struct *tsk = current;
> @@ -332,7 +332,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>         struct user_i387_ia32_struct env;
>         bool success, fx_only = false;
>         union fpregs_state *fpregs;
> -       u64 user_xfeatures = 0;
> +       u64 xrestore_mask = 0;
>
>         if (use_xsave()) {
>                 struct _fpx_sw_bytes fx_sw_user;
> @@ -341,14 +341,14 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>                         return false;
>
>                 fx_only = !fx_sw_user.magic1;
> -               user_xfeatures = fx_sw_user.xfeatures;
> +               xrestore_mask = fx_sw_user.xfeatures;
>         } else {
> -               user_xfeatures = XFEATURE_MASK_FPSSE;
> +               xrestore_mask = XFEATURE_MASK_FPSSE;
>         }
>
>         if (likely(!ia32_fxstate)) {
>                 /* Restore the FPU registers directly from user memory. */
> -               return restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only);
> +               return restore_fpregs_from_user(buf_fx, xrestore_mask, fx_only);
>         }
>
>         /*
> @@ -356,7 +356,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>          * to be ignored for histerical raisins. The legacy state is folded
>          * in once the larger state has been copied.
>          */
> -       if (__copy_from_user(&env, buf, sizeof(env)))
> +       if (__copy_from_user(&env, buf_f, sizeof(env)))
>                 return false;
>
>         /*
> @@ -420,7 +420,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>                  *
>                  * Preserve supervisor states!
>                  */
> -               u64 mask = user_xfeatures | xfeatures_mask_supervisor();
> +               u64 mask = xrestore_mask | xfeatures_mask_supervisor();
>
>                 fpregs->xsave.header.xfeatures &= mask;
>                 success = !os_xrstor_safe(fpu->fpstate,
> --
> 2.54.0.1189.g8c84645362-goog
>
>