[PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths

Oleg Nesterov posted 6 patches 1 month, 3 weeks ago
There is a newer version of this series
arch/x86/include/asm/shstk.h |  8 ++++----
arch/x86/kernel/fpu/regset.c | 46 ++++++++++++++++++++++++++------------------
arch/x86/kernel/fpu/xstate.c | 12 ++++++------
arch/x86/kernel/fpu/xstate.h |  4 ++--
arch/x86/kernel/process.c    |  2 +-
arch/x86/kernel/process_64.c |  2 +-
arch/x86/kernel/shstk.c      | 19 +++++++++++++-----
7 files changed, 55 insertions(+), 38 deletions(-)
[PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Oleg Nesterov 1 month, 3 weeks ago
Sorry, I have no idea how to test these changes, please review. Especially
4/6 and 5/6, I don't really understand shstk.c.

If you are fine with these changes, I'll try to update the fpregs_soft_get()
and user_regset.set() paths as well.

Oleg.
---

 arch/x86/include/asm/shstk.h |  8 ++++----
 arch/x86/kernel/fpu/regset.c | 46 ++++++++++++++++++++++++++------------------
 arch/x86/kernel/fpu/xstate.c | 12 ++++++------
 arch/x86/kernel/fpu/xstate.h |  4 ++--
 arch/x86/kernel/process.c    |  2 +-
 arch/x86/kernel/process_64.c |  2 +-
 arch/x86/kernel/shstk.c      | 19 +++++++++++++-----
 7 files changed, 55 insertions(+), 38 deletions(-)
Re: [PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Oleg Nesterov 1 month, 2 weeks ago
Dave, Sohil, what do you think?

OK, it seems that 5/6 (and thus 6/6) needs more discussion, but what
about 1-3 for the start?

These changes simply shift x86_task_fpu() and sync_fpstate() from
.regset_get() paths into the single helper, get_fpstate(). To me this
makes sense...

Oleg.

On 08/14, Oleg Nesterov wrote:
>
> Sorry, I have no idea how to test these changes, please review. Especially
> 4/6 and 5/6, I don't really understand shstk.c.
>
> If you are fine with these changes, I'll try to update the fpregs_soft_get()
> and user_regset.set() paths as well.
>
> Oleg.
> ---
>
>  arch/x86/include/asm/shstk.h |  8 ++++----
>  arch/x86/kernel/fpu/regset.c | 46 ++++++++++++++++++++++++++------------------
>  arch/x86/kernel/fpu/xstate.c | 12 ++++++------
>  arch/x86/kernel/fpu/xstate.h |  4 ++--
>  arch/x86/kernel/process.c    |  2 +-
>  arch/x86/kernel/process_64.c |  2 +-
>  arch/x86/kernel/shstk.c      | 19 +++++++++++++-----
>  7 files changed, 55 insertions(+), 38 deletions(-)
Re: [PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Dave Hansen 1 month, 2 weeks ago
On 8/15/25 08:52, Oleg Nesterov wrote:
> Dave, Sohil, what do you think?
> 
> OK, it seems that 5/6 (and thus 6/6) needs more discussion, but what
> about 1-3 for the start?
They kinda need fleshed out changelogs first, don't you think?
Especially 1/6. It also needs an actual cover letter explaining what
it's trying to do and the larger context. Ideally, it's self-contained
and as opposed to links to previous discussions.
Re: [PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Oleg Nesterov 1 month, 2 weeks ago
On 08/15, Dave Hansen wrote:
>
> On 8/15/25 08:52, Oleg Nesterov wrote:
> > Dave, Sohil, what do you think?
> >
> > OK, it seems that 5/6 (and thus 6/6) needs more discussion, but what
> > about 1-3 for the start?
> They kinda need fleshed out changelogs first, don't you think?
> Especially 1/6. It also needs an actual cover letter explaining what
> it's trying to do and the larger context. Ideally, it's self-contained
> and as opposed to links to previous discussions.

I agree about the cover letter, but what else do you think the changelog
in 1/6 could say? ;)

OK, I'll try to improve and resend.

Oleg.
Re: [PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Sohil Mehta 1 month, 2 weeks ago
On 8/15/2025 9:02 AM, Oleg Nesterov wrote:
>>> Dave, Sohil, what do you think?

Thank you for doing this series.

I think it would be useful to categorize the impact of the "abuse" in
the cover letter. Is it going to cause kernel crashes, userspace crashes
or just incorrect reporting?

Are there any "must do" fixes that need to be backported in comparison
with the "good to have" optimizations? I am wondering if it might be
possible to structure the series that way to make the separation clear.


> I agree about the cover letter, but what else do you think the changelog
> in 1/6 could say? ;)
> 

For folks like me who are barely familiar with the FPU code, some
additional context or reasoning would be surely be useful.

For example, I don't know why PKRU needs to be passed separately. I know
there is some history there but a line or two in the changelog might help.
Re: [PATCH 0/6] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset_get() paths
Posted by Oleg Nesterov 1 month, 2 weeks ago
On 08/15, Sohil Mehta wrote:
>
> I think it would be useful to categorize the impact of the "abuse" in
> the cover letter.

Yes, let me repeat that this is my fault.

>Is it going to cause kernel crashes, userspace crashes
> or just incorrect reporting?

Heh ;)

In the short term, I'd like to make your patch correct ;)

	[PATCH v3 2/2] x86/fpu: Update the debug flow for x86_task_fpu()
	https://lore.kernel.org/all/20250724013422.307954-2-sohil.mehta@intel.com/

(just in case, no, this series alone is not enough).

In the longer term, rightly or not I'd like to do other changes we have already
discussed. See my other emails in this thread.

Oleg.