[PATCH] x86/fpu: Free dynamic fpstate on exec()

[PATCH] x86/fpu: Free dynamic fpstate on exec()

[Guixiong Wei]

The fpstate embedded in struct fpu only accommodates the default
xfeatures. When a task first uses a dynamically enabled xfeature,
fpstate_realloc() installs a larger fpstate allocated with vzalloc().

fpu_flush_thread() resets fpu->fpstate to the embedded fpstate on exec()
without freeing the dynamically allocated one. Once the pointer is
overwritten, arch_release_task_struct() cannot free the allocation when
the task exits.

Preserve the old fpstate pointer across fpstate_reset() and free it only
after fpu_reset_fpstate_regs() has invalidated FPU register ownership.
This ordering prevents a context switch from saving FPU registers through
a freed fpstate pointer.

Make fpstate_free() operate on the fpstate pointer itself so it can free
the detached allocation. Use it for the existing reallocation and task
release paths as well.

On an AMX-capable system, 1000 iterations of requesting XTILEDATA
permission, executing TILEZERO and calling execve() on the same image
left 1000 16 KiB allocations attributed to __xfd_enable_feature() in
/proc/vmallocinfo. None remained after this change.

Fixes: 500afbf645a0 ("x86/fpu/xstate: Add fpstate_realloc()/free()")
Cc: stable@vger.kernel.org
Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
---
 arch/x86/include/asm/fpu/api.h |  6 +++---
 arch/x86/kernel/fpu/core.c     | 11 ++++++++++-
 arch/x86/kernel/fpu/xstate.c   | 10 ++++------
 arch/x86/kernel/process.c      |  2 +-
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index 90c63fe19c0fb..3e0c3b7dce73b 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -123,11 +123,11 @@ extern void fpu__resume_cpu(void);
 DECLARE_PER_CPU(bool, kernel_fpu_allowed);
 DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
 
-/* Process cleanup */
+/* Dynamic fpstate cleanup */
 #ifdef CONFIG_X86_64
-extern void fpstate_free(struct fpu *fpu);
+extern void fpstate_free(struct fpstate *fpstate);
 #else
-static inline void fpstate_free(struct fpu *fpu) { }
+static inline void fpstate_free(struct fpstate *fpstate) { }
 #endif
 
 /* fpstate-related functions which are exported to KVM */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index d1aeecd57f5ed..08f5c77d990aa 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -866,8 +866,17 @@ void fpu__clear_user_states(struct fpu *fpu)
 
 void fpu_flush_thread(void)
 {
-	fpstate_reset(x86_task_fpu(current));
+	struct fpu *fpu = x86_task_fpu(current);
+	struct fpstate *oldfpstate = fpu->fpstate;
+
+	fpstate_reset(fpu);
 	fpu_reset_fpstate_regs();
+
+	/*
+	 * Free the old state only after register state ownership has been
+	 * invalidated. Otherwise, a context switch could save into it.
+	 */
+	fpstate_free(oldfpstate);
 }
 /*
  * Load FPU context before returning to userspace.
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a7b6524a9dea2..ea3736a74fd43 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1556,10 +1556,10 @@ static int __init xfd_update_static_branch(void)
 }
 arch_initcall(xfd_update_static_branch)
 
-void fpstate_free(struct fpu *fpu)
+void fpstate_free(struct fpstate *fpstate)
 {
-	if (fpu->fpstate && fpu->fpstate != &fpu->__fpstate)
-		vfree(fpu->fpstate);
+	if (fpstate && fpstate->is_valloc)
+		vfree(fpstate);
 }
 
 /**
@@ -1640,9 +1640,7 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize,
 		xfd_update_state(fpu->fpstate);
 	fpregs_unlock();
 
-	/* Only free valloc'ed state */
-	if (curfps && curfps->is_valloc)
-		vfree(curfps);
+	fpstate_free(curfps);
 
 	return 0;
 }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 346c438ac8801..b7306f257f4b0 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -118,7 +118,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 void arch_release_task_struct(struct task_struct *tsk)
 {
 	if (fpu_state_size_dynamic() && !(tsk->flags & (PF_KTHREAD | PF_USER_WORKER)))
-		fpstate_free(x86_task_fpu(tsk));
+		fpstate_free(x86_task_fpu(tsk)->fpstate);
 }
 #endif
 

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()

[Dave Hansen]

On 9/29/26 08:10, Guixiong Wei wrote:
> Make fpstate_free() operate on the fpstate pointer itself so it can
> free the detached allocation. Use it for the existing reallocation
> and task release paths as well.

I had to stare at this for a moment to get what was going on. This is
trying to say something like:

	fpstate_free() only frees the fpstate currently associated with
	a 'struct fpu'. It is unable to free an fpstate that has been
	detached from the fpu, such as after a reset.

> On an AMX-capable system, 1000 iterations of requesting XTILEDATA
> permission, executing TILEZERO and calling execve() on the same image
> left 1000 16 KiB allocations attributed to __xfd_enable_feature() in
> /proc/vmallocinfo. None remained after this change.

I really value being succinct in these changelogs. Both the subject and
the changelog don't really get to the point. Shouldn't this be:

	x86/fpu: Fix memory leak with dynamic fpstate and exec()

?

In the changelog, shouldn't it say "leak" somewhere?

>  void fpu_flush_thread(void)
>  {
> -	fpstate_reset(x86_task_fpu(current));
> +	struct fpu *fpu = x86_task_fpu(current);
> +	struct fpstate *oldfpstate = fpu->fpstate;
> +
> +	fpstate_reset(fpu);
>  	fpu_reset_fpstate_regs();
> +
> +	/*
> +	 * Free the old state only after register state ownership has been
> +	 * invalidated. Otherwise, a context switch could save into it.
> +	 */
> +	fpstate_free(oldfpstate);
>  }

I'm not sure I like where this lands.

The problem with the code now is that it expects it is always OK to call
fpstate_reset(). In fact, it is dangerous to call fpstate_reset() on any
fpu that has dynamic state allocated because it will be leaked.

The patch fixes one of the call sites of fpstate_reset() but didn't make
fpstate_reset() itself any safer.

Wouldn't this be simpler and more foolproof against new users?

 void fpstate_reset(struct fpu *fpu)
 {
+	/* Free dynamic fpstate (if any) before resetting pointer: */
+	fpstate_free(fpu);

        /* Set the fpstate pointer to the default fpstate */
        fpu->fpstate = &fpu->__fpstate;
        __fpstate_reset(fpu->fpstate);

It will mean one more change in fpu_clone(), but I think that's OK:

 int fpu_clone(struct task_struct *dst, u64 clone_flags, bool minimal,
               unsigned long ssp)
 {

        /* The new task's FPU state cannot be valid in the hardware. */
        dst_fpu->last_cpu = -1;

+	/* Break the link to the other FPU's fpstate:  */
+	dst_fpu->fpstate = NULL;
        fpstate_reset(dst_fpu);

I'd much rather special case clone() than the other paths.

Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()

[Dave Hansen]

On 9/29/26 08:10, Guixiong Wei wrote:
> Preserve the old fpstate pointer across fpstate_reset() and free it only
> after fpu_reset_fpstate_regs() has invalidated FPU register ownership.
> This ordering prevents a context switch from saving FPU registers through
> a freed fpstate pointer.

Are there any fpstate_reset() paths where this matters?

I mean, the init task, obviously not. It doesn't have a dynamic fpstate.
During exec() this reset happens while:

	bprm->mm = NULL;

among other things, so I really don't think it's even remotely in the
context of getting context-switched to.

For clone(), the destination task is also not in any condition to be
context-switched to.

So why defend against context switching? What am I missing?

Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()

[Chang S. Bae]

On 9/29/2026 8:10 AM, Guixiong Wei wrote:
> The fpstate embedded in struct fpu only accommodates the default
> xfeatures. When a task first uses a dynamically enabled xfeature,
> fpstate_realloc() installs a larger fpstate allocated with vzalloc().
> 
> fpu_flush_thread() resets fpu->fpstate to the embedded fpstate on exec()
> without freeing the dynamically allocated one. Once the pointer is
> overwritten, arch_release_task_struct() cannot free the allocation when
> the task exits.

Thanks for the finding and fixing this. I see Dave has given good 
suggestions. I think you can follow those into a revision.

...

> On an AMX-capable system, 1000 iterations of requesting XTILEDATA
> permission, executing TILEZERO and calling execve() on the same image
> left 1000 16 KiB allocations attributed to __xfd_enable_feature() in
> /proc/vmallocinfo. None remained after this change.

I would be interested in turning this into a selftest if possible. It 
would be great if you could.

Otherwise, I wrote a case (using XRSTOR instead) and then saw that 
amount from fpstate_relloac() in vmallocinfo:

   Caller/Module                            Difference
   ---------------------------------------------------
   fpstate_realloc.constprop.0 (pages=3)    +16384000
   ...

BTW, just curious if it happens with real-world applications. The 
permission is revoked on exec(), that is a very edge of the TMUL use if 
so. Is it a case running another application right after matrix 
multiplications?

Thanks,
Chang

Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()

[魏桂雄]

> From: "Dave Hansen"<dave.hansen@intel.com>
> Date:  Wed, Sep 30, 2026, 00:54
> Subject:  Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()
> To: "Guixiong Wei"<weiguixiong@bytedance.com>, <x86@kernel.org>
> Cc: "Thomas Gleixner"<tglx@kernel.org>, "Ingo Molnar"<mingo@redhat.com>, "Borislav Petkov"<bp@alien8.de>, "Dave Hansen"<dave.hansen@linux.intel.com>, "H . Peter Anvin"<hpa@zytor.com>, "Chang S . Bae"<chang.seok.bae@intel.com>, <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
> On 9/29/26 08:10, Guixiong Wei wrote:
> > Preserve the old fpstate pointer across fpstate_reset() and free it only
> > after fpu_reset_fpstate_regs() has invalidated FPU register ownership.
> > This ordering prevents a context switch from saving FPU registers through
> > a freed fpstate pointer.
> 
> Are there any fpstate_reset() paths where this matters?
> 
> I mean, the init task, obviously not. It doesn't have a dynamic fpstate.
> During exec() this reset happens while:
> 
>         bprm->mm = NULL;
> 
> among other things, so I really don't think it's even remotely in the
> context of getting context-switched to.
> 
> For clone(), the destination task is also not in any condition to be
> context-switched to.
> 
> So why defend against context switching? What am I missing?
> 
You are right. Neither existing fpstate_reset() call path needs
to defend against a context switch. I overcomplicated the ordering
rationale and will drop it.

I will move the dynamic fpstate cleanup into fpstate_reset() as you
suggested. It will preserve the old pointer, install and initialize the
embedded fpstate, and then free the old allocation. This makes
fpstate_reset() own the complete state transition.

I will also update the subject and describe the leak.

Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()

[Guixiong Wei]

> From: "Chang S. Bae"<chang.seok.bae@intel.com>
> Date:  Wed, Sep 30, 2026, 14:51
> Subject:  Re: [PATCH] x86/fpu: Free dynamic fpstate on exec()
> To: "Guixiong Wei"<weiguixiong@bytedance.com>, <x86@kernel.org>
> Cc: "Thomas Gleixner"<tglx@kernel.org>, "Ingo Molnar"<mingo@redhat.com>, "Borislav Petkov"<bp@alien8.de>, "Dave Hansen"<dave.hansen@linux.intel.com>, "H . Peter Anvin"<hpa@zytor.com>, <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
> On 9/29/2026 8:10 AM, Guixiong Wei wrote:
> > The fpstate embedded in struct fpu only accommodates the default
> > xfeatures. When a task first uses a dynamically enabled xfeature,
> > fpstate_realloc() installs a larger fpstate allocated with vzalloc().
> > 
> > fpu_flush_thread() resets fpu->fpstate to the embedded fpstate on exec()
> > without freeing the dynamically allocated one. Once the pointer is
> > overwritten, arch_release_task_struct() cannot free the allocation when
> > the task exits.
> 
> Thanks for the finding and fixing this. I see Dave has given good 
> suggestions. I think you can follow those into a revision.

Sure. I will add function test_exec to tools/testing/selftests/x86/amx.c,
covering the AMX use and exec sequence in the next revision.

> 
> ...
> 
> > On an AMX-capable system, 1000 iterations of requesting XTILEDATA
> > permission, executing TILEZERO and calling execve() on the same image
> > left 1000 16 KiB allocations attributed to __xfd_enable_feature() in
> > /proc/vmallocinfo. None remained after this change.
> 
> I would be interested in turning this into a selftest if possible. It 
> would be great if you could.
> 
> Otherwise, I wrote a case (using XRSTOR instead) and then saw that 
> amount from fpstate_relloac() in vmallocinfo:
> 
>    Caller/Module                            Difference
>    ---------------------------------------------------
>    fpstate_realloc.constprop.0 (pages=3)    +16384000
>    ...
> 
> BTW, just curious if it happens with real-world applications. The 
> permission is revoked on exec(), that is a very edge of the TMUL use if 
> so. Is it a case running another application right after matrix 
> multiplications?
> 

I found this while developing an AMX application that calls exec()
after performing AMX computations.

> Thanks,
> Chang
>