linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: "Bae, Chang Seok" <chang.seok.bae@intel.com>
Cc: Andrew Lutomirski <luto@kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"Metzger, Markus T" <markus.t.metzger@intel.com>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [regression in -rc1] Re: [PATCH v6 2/8] x86/fsgsbase/64: Introduce FS/GS base helper functions
Date: Wed, 24 Oct 2018 12:43:09 -0700	[thread overview]
Message-ID: <CALCETrV=dJ-3CGM3oB7xgNYsKE_Tbpu5i7H4qwKpuNmEsjyL_A@mail.gmail.com> (raw)
In-Reply-To: <C9BB696F3A938947B10DCAD29FAB8FFA66CCBC7E@CRSMSX101.amr.corp.intel.com>

On Wed, Oct 24, 2018 at 12:29 PM Bae, Chang Seok
<chang.seok.bae@intel.com> wrote:
>
> On Wed, Oct 24, 2018 at 12:22 PM Andy Lutomirski <luto@kernel.org>
> > On Wed, Oct 24, 2018 at 12:13 PM Bae, Chang Seok
> > <chang.seok.bae@intel.com> wrote:
> > >
> > > On Tue, Sep 18, 2018 at 12:02 PM Andy Lutomirski <luto@kernel.org>
> > > > On Tue, Sep 18, 2018 at 4:09 PM Chang S. Bae
> > > > <chang.seok.bae@intel.com>
> > > > wrote:
> > > > >
> > > > > With new helpers, FS/GS base access is centralized.
> > > > > Eventually, when FSGSBASE instruction enabled, it will be faster.
> > > >
> > > > Sorry for not catching this during review, but:
> > > >
> > > > > +void x86_fsbase_write_cpu(unsigned long fsbase) {
> > > > > +       /*
> > > > > +        * Set the selector to 0 as a notion, that the segment base is
> > > > > +        * overwritten, which will be checked for skipping the segment load
> > > > > +        * during context switch.
> > > > > +        */
> > > > > +       loadseg(FS, 0);
> > > >
> > > > ^^^
> > > >
> > > > what?
> > > >
> > > > > +       wrmsrl(MSR_FS_BASE, fsbase); }
> > > >
> > > > I don't understand what the comment is trying to say, but the sole
> > > > caller so far of this function is x86_gsbase_write_task(), and the code looks
> > incorrect.
> > > >
> > > > Ingo, I think we need to address this during this merge window,
> > > > probably by removing the comment and the loadseg() call (and the
> > > > same for gsbase...inactive).  But first, Chang, can you explain what
> > > > exactly your intent is here?
> > >
> > > It's coming from do_arch_prctl_64(). If you think it really makes
> > > confusion in x86_fsbase_write_cpu(), how about moving it to
> > x86_fsbase_write_task()?
> >
> > Why should ..write_task() magically change the index but only if it's writing
> > current?
> >
> > I think you should move it all the way out to the caller
> > (do_arch_prctl_64()?) and we can see if it makes sense there.
> >
>
> Okay. x86_fsbase_write_task() doesn't make sense.
> Then, it should rollback that helper and call x86_fsbase_write_cpu() only
> from ptrace. Same for gsbase. Sounds okay?
>

I think x86_fsbase_write_task() makes plenty of sense, but I think
that callers need to be aware that the effect of writing a nonzero
fsbase *and* a nonzero fsindex is bizarre on non-FSGSBASE systems.  So
that code should go in the callers.  The oddities involved have little
to do with whether the caller is writing to current or to something
else.

Arguably the code should be entirely split out into the code that
writes current (arch_prctl()) and the code that writes a stopped task
(ptrace).  I don't think there are any code paths that genuinely can
write either.

--Andy

--Andy

  reply	other threads:[~2018-10-24 19:43 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-18 23:08 [PATCH v6 0/8] x86: infrastructure to enable FSGSBASE Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 1/8] x86/arch_prctl/64: Make ptrace read FS/GS base accurately Chang S. Bae
2018-10-08  9:54   ` [tip:x86/asm] x86/fsgsbase/64: Fix ptrace() to read the " tip-bot for Andy Lutomirski
2018-10-08  9:59   ` [tip:x86/asm] x86/segments: Introduce the 'CPUNODE' naming to better document the segment limit CPU/node NR trick tip-bot for Ingo Molnar
2018-10-08  9:59   ` [tip:x86/asm] x86/fsgsbase/64: Clean up various details tip-bot for Ingo Molnar
2018-09-18 23:08 ` [PATCH v6 2/8] x86/fsgsbase/64: Introduce FS/GS base helper functions Chang S. Bae
2018-10-08  9:55   ` [tip:x86/asm] " tip-bot for Chang S. Bae
2018-10-24 19:01   ` [regression in -rc1] Re: [PATCH v6 2/8] " Andy Lutomirski
2018-10-24 19:13     ` Bae, Chang Seok
2018-10-24 19:22       ` Andy Lutomirski
2018-10-24 19:29         ` Bae, Chang Seok
2018-10-24 19:43           ` Andy Lutomirski [this message]
2018-10-24 22:50             ` Bae, Chang Seok
2018-10-25 22:37     ` Andy Lutomirski
2018-09-18 23:08 ` [PATCH v6 3/8] x86/fsgsbase/64: Make ptrace use correct FS/GS base helpers Chang S. Bae
2018-10-08  9:56   ` [tip:x86/asm] x86/fsgsbase/64: Make ptrace use the new " tip-bot for Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 4/8] x86/fsgsbase/64: Use FS/GS base helpers in core dump Chang S. Bae
2018-10-08  9:56   ` [tip:x86/asm] x86/fsgsbase/64: Convert the ELF core dump code to the new FSGSBASE helpers tip-bot for Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 5/8] x86/fsgsbase/64: Factor out load FS/GS segments from __switch_to() Chang S. Bae
2018-10-08  9:57   ` [tip:x86/asm] x86/fsgsbase/64: Factor out FS/GS segment loading " tip-bot for Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 6/8] x86/segments/64: Rename PER_CPU segment to CPU_NUMBER Chang S. Bae
2018-10-08  9:57   ` [tip:x86/asm] x86/segments/64: Rename the GDT PER_CPU entry " tip-bot for Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 7/8] x86/vdso: Introduce helper functions for CPU and node number Chang S. Bae
2018-10-08  9:58   ` [tip:x86/asm] " tip-bot for Chang S. Bae
2018-09-18 23:08 ` [PATCH v6 8/8] x86/vdso: Move out the CPU initialization Chang S. Bae
2018-10-08  8:36   ` Ingo Molnar
2018-10-08  9:58   ` [tip:x86/asm] x86/vdso: Initialize the CPU/node NR segment descriptor earlier tip-bot for Chang S. Bae

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to='CALCETrV=dJ-3CGM3oB7xgNYsKE_Tbpu5i7H4qwKpuNmEsjyL_A@mail.gmail.com' \
    --to=luto@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=chang.seok.bae@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=markus.t.metzger@intel.com \
    --cc=mingo@kernel.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=tglx@linutronix.de \
    --subject='Re: [regression in -rc1] Re: [PATCH v6 2/8] x86/fsgsbase/64: Introduce FS/GS base helper functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).