Re: [regression in -rc1] Re: [PATCH v6 2/8] x86/fsgsbase/64: Introduce FS/GS base helper functions

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