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

[Dave Hansen <dave.hansen@intel.com>]

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.