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

[Guixiong Wei]

> From: "Chang S. Bae"<chang.seok.bae@xxxxxxxxx>
> Date:  Thu, Oct 15, 2026, 08:19
> Subject:  Re: [PATCH v2 1/2] x86/fpu: Fix memory leak with dynamic fpstate and exec()
> To: "Guixiong Wei"<weiguixiong@xxxxxxxxxxxxx>, <x86@xxxxxxxxxx>
> Cc: "Thomas Gleixner"<tglx@xxxxxxxxxx>, "Ingo Molnar"<mingo@xxxxxxxxxx>, "Borislav Petkov"<bp@xxxxxxxxx>, "Dave Hansen"<dave.hansen@xxxxxxxxxxxxxxx>, "H . Peter Anvin"<hpa@xxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, <stable@xxxxxxxxxxxxxxx>
> 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.
Will do. v3 will picks up Dave's phrasing for the opening paragraphs
describing how the dynamic fpstate is allocated and leaked.

> 
> > 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.
Agreed, no functional reason. Will free at the beginning of
fpstate_reset() in v3.

> 
> > 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.
> 
Will do. Dropped in v3; 

> >   
> >   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: */
> 
Will do.

> >           /* 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;
> 
Will do.

> >   
> >           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?
> 
No need: is_valloc lives inside the vmalloc'ed buffer itself, so
vfree() releases the flag together with the storage.

> Thanks,
> Chang
>