Re: [PATCH RESEND] x86/fpu: Use vmemdup_user() in xstateregs_set()

[Ingo Molnar]

* Thorsten Blum <thorsten.blum@xxxxxxxxx> wrote:

> On Sun, Aug 02, 2026 at 03:20:59PM +0200, Ingo Molnar wrote:
> > * Thorsten Blum <thorsten.blum@xxxxxxxxx> wrote:
> > > > Replace the open-coded vmalloc() and copy_from_user() with
> > > > vmemdup_user() to simplify xstateregs_set().
> > > > 
> > > > vmemdup_user() returns an ERR_PTR() on failure, preserving the existing
> > > > -ENOMEM and -EFAULT error codes. Since vmemdup_user() is backed by
> > > > kvmalloc(), use kvfree() to free the buffer instead.
> > > > 
> > > > Return early on error and drop the obsolete out label.
> > 
> > > I've resent this cleanup a few times and haven't received feedback. Is
> > > this still something you would consider, or would you prefer that I drop
> > > it? It's intended to be a mechanical cleanup with no functional changes.
> > > 
> > > Happy to rerun the relevant x86 XSAVE selftests if it helps.
> > 
> > So the changelog is silent on any potential side-effects of this change.
> > vmalloc will return page-aligned addresses. kmalloc won't. Will this
> > have any effects on performance?
> > 
> > Are there other examples of vmalloc() -> kvmalloc() conversions in
> > the tree, is it a recommended and well-tested technique to simplify
> > the code?
> > 
> > A very quick git log investigation does not seem to be showing
> > any such recent conversion examples:
> > 
> >   starship:~/tip> git log -E --grep='kvmalloc' --since=one-year-ago | grep -w vmalloc
> > 
> > I'm not opposed to it in principle, I just think the 'analysis'
> > side of the patch description is basically non-existent.
> 
> Performance should be similar, and it's not a hot path.
> 
> Page alignment shouldn't matter because the buffer is only used as a
> memcpy() source in copy_uabi_from_kernel_to_xstate().
> 
> The main difference is that vmemdup_user() uses GFP_USER, which adds
> __GFP_HARDWALL and may change cpuset allocations.
> 
> The avx_64 selftests also pass on my Alder Lake CPU:
> 
>   [RUN]   AVX registers: check context switches, 10 iterations, 5 threads.
>   [OK]    No incorrect case was found.
>   [RUN]   AVX registers: inject xstate via ptrace().
>   [OK]    'xfeatures' in SW reserved area was correctly written
>   [OK]    xstate was correctly updated.
>   [RUN]   AVX registers: load xstate and raise SIGUSR1
>   [OK]    'magic1' is valid
>   [OK]    'xfeatures' in SW reserved area is valid
>   [OK]    'xfeatures' in XSAVE header is valid
>   [OK]    xstate delivery was successful
>   [OK]    'magic2' is valid
>   [RUN]   AVX registers: load new xstate from sighandler and check it after sigreturn
>   [OK]    xstate was restored correctly
>   # The kernel does not support feature number: 5
>   # The kernel does not support feature number: 6
>   # The kernel does not support feature number: 7

With this analysis included in the changelog I suppose it's
a worthwile simplification.

Thanks,

	Ingo