Re: [PATCH v2 1/2] x86/fpu: Fix memory leak with dynamic fpstate and exec()

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 10/7/2026 10:59 PM, Guixiong Wei wrote:
> The fpstate embedded in struct fpu only accommodates the default
> xfeatures. After a task obtains permission for an XFD-controlled
> xfeature, its first use raises #NM. The handler calls
> __xfd_enable_feature(), which uses fpstate_realloc() to install a larger
> fpstate allocated with vzalloc().
> 
> On exec(), fpu_flush_thread() calls fpstate_reset(), which overwrites
> fpu->fpstate with the embedded fpstate. This leaks the dynamic
> allocation because arch_release_task_struct() cannot free it when the
> task exits.

Hmm, I'd start with Dave's wording here, or tweak it slightly.

> Make fpstate_reset() own the complete lifetime transition. Preserve the
> old pointer, install and initialize the embedded fpstate, and then free
> the old allocation. This detaches the allocation from fpu->fpstate
> before freeing it and makes future reset callers release dynamic state
> automatically.

I'm not sure what the intended argument is here. Does this mean the old 
allocation cannot or should not be freed at the beginning of 
fpstate_reset()? If so, please explain functional issues. Otherwise, I'd 
simply follow Dave earlier suggestion.

> On an AMX-capable KVM guest, 1000 iterations left 1000 dynamic fpstate
> allocations on an unfixed kernel and none with this change.

Since the selftest can reproduce the issue without KVM, I'm not sure why
the KVM guest is relevant here. In any case, the entire sentence could 
be dropped if the problem description is clear enough.

>   
>   void fpstate_reset(struct fpu *fpu)
>   {
> +	struct fpstate *oldfpstate = fpu->fpstate;
> +

Hmm, again, why not just call fpstate_free(fpu->fpstate) here?

I see Dave also noted this comment along the lines:
	/* Free dynamic fpstate (if any) before resetting pointer: */

>   	/* Set the fpstate pointer to the default fpstate */
>   	fpu->fpstate = &fpu->__fpstate;
>   	__fpstate_reset(fpu->fpstate);
> @@ -610,6 +612,8 @@ void fpstate_reset(struct fpu *fpu)
>   	 * guest FPUs, which allows for common guest and userspace ABI.
>   	 */
>   	fpu->guest_perm.__user_state_size = fpu_user_cfg.default_size;
> +
> +	fpstate_free(oldfpstate);
>   }


> @@ -670,6 +674,7 @@ int fpu_clone(struct task_struct *dst, u64 clone_flags, bool minimal,
>   	/* The new task's FPU state cannot be valid in the hardware. */
>   	dst_fpu->last_cpu = -1;
>   
> +	dst_fpu->fpstate = NULL;
>   	fpstate_reset(dst_fpu);

Dave also suggested adding a comment here:

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

>   
>   	if (!cpu_feature_enabled(X86_FEATURE_FPU))
> 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);
>   }

Shouldn't is_valloc be cleared here, or when fpstate_reset() replaces 
fpu->fpstate with the embedded fpstate?

Thanks,
Chang