LKML Archive on
 help / color / Atom feed
From: Sebastian Andrzej Siewior <>
To: Yu-cheng Yu <>
	"H. Peter Anvin" <>,
	Thomas Gleixner <>,
	Ingo Molnar <>,
	Dave Hansen <>,
	Tony Luck <>,
	Andy Lutomirski <>, Borislav Petkov <>,
	Rik van Riel <>,
	"Ravi V. Shankar" <>,
	Fenghua Yu <>,
	Peter Zijlstra <>
Subject: Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails
Date: Fri, 20 Dec 2019 21:16:21 +0100
Message-ID: <> (raw)
In-Reply-To: <>

On 2019-12-19 09:40:06 [-0800], Yu-cheng Yu wrote:
> On Thu, 2019-12-19 at 18:16 +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-12-19 08:44:08 [-0800], Yu-cheng Yu wrote:
> > > Yes, this works.  But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
> > > Are there other alternatives?
> > 
> > I don't like the big hammer approach of your very much. It might make
> > all it "correct" but then it might lead to more "invalids" then needed.
> > It also required to export the symbol which I would like to avoid.
> Copying to registers invalids current fpregs context.  It might not cause
> extra register loading, because registers are in fact already invalidated
> and any task owning the context needs to reload anyway.  Setting
> fpu_fpregs_owner_ctx is only to let the rest of the kernel know the
> fact that already happened.
> But, I agree with you the patch does look biggish.

Now that I looked at it:
All kernel loads don't fail. If they fail we end up in the handler and
restore to init-state. So no need to reset `fpu_fpregs_owner_ctx' in this
case. The variable is actually set to task's FPU state so resetting is
not required.
fpu__save() invokes copy_kernel_to_fpregs() (on older boxes) and by
resetting `fpu_fpregs_owner_ctx' we would load it twice (in fpu__save()
and on return to userland).

So far I can tell, the only problematic case is the signal code because
here the state restore *may* fail and we *may* do it in two steps. The
error happens only if both `may' are true.

> > So if this patch works for you and you don't find anything else where it
> > falls apart then I will audit tomorrow all callers which got the
> > "invalidator" added and check for that angle.
> Yes, that works for me.  Also, most of these call sites are under fpregs_lock(),
> and we could use __cpu_invalidate_fpregs_state().
> I was also thinking maybe add warnings when any new code re-introduces the issue,
> but not sure where to add that.  Do you think that is needed?

I was thinking about it. So the `read-FPU-state' function must be
invoked within the fpregs_lock() section. This could be easily
enforced. At fpregs_unlock() time `fpu_fpregs_owner_ctx' must be NULL or
pointing to task's FPU.
My brain is fried for today so I'm sure if this is a sane approach. But
it might be a start.

> Thanks,
> Yu-cheng


  parent reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-12 21:08 [PATCH v2 0/3] Fix small issues in XSAVES Yu-cheng Yu
2019-12-12 21:08 ` [PATCH v2 1/3] x86/fpu/xstate: Fix small issues before adding supervisor xstates Yu-cheng Yu
2019-12-20 20:19   ` Sebastian Andrzej Siewior
2020-01-06 18:15   ` [tip: x86/fpu] x86/fpu/xstate: Fix small issues tip-bot2 for Yu-cheng Yu
2019-12-12 21:08 ` [PATCH v2 2/3] x86/fpu/xstate: Make xfeature_is_supervisor()/xfeature_is_user() return bool Yu-cheng Yu
2019-12-20 20:19   ` Sebastian Andrzej Siewior
2019-12-20 20:33     ` Yu-cheng Yu
2020-01-06 18:15   ` [tip: x86/fpu] " tip-bot2 for Yu-cheng Yu
2019-12-12 21:08 ` [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails Yu-cheng Yu
2019-12-18 15:54   ` Sebastian Andrzej Siewior
2019-12-18 20:53     ` Yu-cheng Yu
2019-12-19 14:22       ` Sebastian Andrzej Siewior
2019-12-19 16:44         ` Yu-cheng Yu
2019-12-19 17:16           ` Sebastian Andrzej Siewior
2019-12-19 17:40             ` Yu-cheng Yu
2019-12-20 19:59               ` [PATCH] x86/fpu: Deacticate FPU state after failure during state load Sebastian Andrzej Siewior
2020-01-07 12:52                 ` [tip: x86/fpu] x86/fpu: Deactivate " tip-bot2 for Sebastian Andrzej Siewior
2020-01-07 20:41                   ` Andy Lutomirski
2020-01-07 20:38                     ` Yu-cheng Yu
2020-01-07 21:11                     ` Sebastian Andrzej Siewior
2020-01-08 11:45                       ` Borislav Petkov
2020-01-08 11:46                     ` Borislav Petkov
2019-12-20 20:16               ` Sebastian Andrzej Siewior [this message]
2019-12-20 20:32                 ` [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails Yu-cheng Yu
  -- strict thread matches above, loose matches on Subject: below --
2019-12-05 18:26 [PATCH " Yu-cheng Yu
2019-12-07  4:38 ` [PATCH v2 " Yu-cheng Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on

Archives are clonable:
	git clone --mirror lkml/git/0.git
	git clone --mirror lkml/git/1.git
	git clone --mirror lkml/git/2.git
	git clone --mirror lkml/git/3.git
	git clone --mirror lkml/git/4.git
	git clone --mirror lkml/git/5.git
	git clone --mirror lkml/git/6.git
	git clone --mirror lkml/git/7.git
	git clone --mirror lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ \
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone