LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: "Chang S. Bae" <chang.seok.bae@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Andi Kleen <ak@linux.intel.com>,
	Ravi Shankar <ravi.v.shankar@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry
Date: Mon, 25 Mar 2019 10:44:04 +0100 (CET)
Message-ID: <alpine.DEB.2.21.1903251003090.1798@nanos.tec.linutronix.de> (raw)
In-Reply-To: <1552680405-5265-9-git-send-email-chang.seok.bae@intel.com>

On Fri, 15 Mar 2019, Chang S. Bae wrote:

> The FSGSBASE instructions allow fast accesses on GSBASE.  Now, at the
> paranoid_entry, the per-CPU base value can be always copied to GSBASE.
> And the original GSBASE value will be restored at the exit.

Again you are describing WHAT but not the WHY.

> So far, GSBASE modification has not been directly allowed from userspace.
> So, swapping GSBASE has been conditionally executed according to the
> kernel-enforced convention that a negative GSBASE indicates a kernel value.
> But when FSGSBASE is enabled, userspace can put an arbitrary value in
> GSBASE. The change will secure a correct GSBASE value with FSGSBASE.

So that's some WHY, but it should be explained _BEFORE_ explaining the
change. This changelog style is as bad as top posting. Why?

  1) FSGSBASE is fast

  2) Copy GSBASE always on paranoid exit and restore on entry

  3) Explain the context

No. You want to explain context first and then explain why this needs a
change when FSGSBASE is enabled and how that change looks like at the
conceptual level.

> Also, factor out the RDMSR-based GSBASE read into a new macro,
> READ_MSR_GSBASE.

This new macro is related to this change in what way? None AFAICT. I'm fine
with the macro itself, but the benefit for a single usage site is dubious.

Adding this macro and using it should be done with a separate patch before
this one, so this patch becomes simpler to review.

>  	/*
> @@ -1178,9 +1185,38 @@ ENTRY(paranoid_entry)
>  	 * This is also why CS (stashed in the "iret frame" by the
>  	 * hardware at entry) can not be used: this may be a return
>  	 * to kernel code, but with a user CR3 value.
> +	 *
> +	 * As long as this PTI macro doesn't depend on kernel GSBASE,
> +	 * we can do it early. This is because FIND_PERCPU_BASE
> +	 * references data in kernel space.

It's not about 'can do it early'. The FSGSBASE handling requires that the
kernel page tables are switched in.

And for review and bisectability sake moving the CR3 switch in front of the
GS handling should be done as a separate preparatory patch.

>  	 */
>  	SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14
>  
> +	/*
> +	 * Read GSBASE by RDGSBASE. Kernel GSBASE is found
> +	 * from the per-CPU offset table with a CPU NR.

That CPU NR comes out of thin air, right? This code is complex enough by
itself and does not need further confusion by comments which need a crystal
ball for decoding.

> +	 */

Sigh. I can't see how that comment explains the ALTERNATIVE jump.

> +	ALTERNATIVE "jmp .Lparanoid_entry_no_fsgsbase",	"",\
> +		X86_FEATURE_FSGSBASE

Please separate the above from the below with a new line for readability
sake.

> +	rdgsbase	%rbx
> +	FIND_PERCPU_BASE	%rax
> +	wrgsbase	%rax

So this really should be wrapped in a macro like:

   	SAVE_AND_SET_GSBASE	%rbx, %rax

which makes it entirely clear what this is about.

> +	ret
> +
  
> @@ -1194,12 +1230,21 @@ END(paranoid_entry)
>   * be complicated.  Fortunately, we there's no good reason
>   * to try to handle preemption here.
>   *
> - * On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
> + * On entry,
> + *	With FSGSBASE,
> + *		%rbx is original GSBASE that needs to be restored on the exit
> + *	Without that,
> + * 		%ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
>   */
>  ENTRY(paranoid_exit)
>  	UNWIND_HINT_REGS
>  	DISABLE_INTERRUPTS(CLBR_ANY)
>  	TRACE_IRQS_OFF_DEBUG
> +	ALTERNATIVE "jmp .Lparanoid_exit_no_fsgsbase",	"nop",\
> +		X86_FEATURE_FSGSBASE
> +	wrgsbase	%rbx
> +	jmp	.Lparanoid_exit_no_swapgs;

Again. A few newlines would make it more readable.

This modifies the semantics of paranoid_entry and paranoid_exit. Looking at
the usage sites there is the following code in the nmi maze:

	/*
	 * Use paranoid_entry to handle SWAPGS, but no need to use paranoid_exit
	 * as we should not be calling schedule in NMI context.
	 * Even with normal interrupts enabled. An NMI should not be
	 * setting NEED_RESCHED or anything that normal interrupts and
	 * exceptions might do.
	 */
	call	paranoid_entry
	UNWIND_HINT_REGS

	/* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */
	movq	%rsp, %rdi
	movq	$-1, %rsi
	call	do_nmi

	/* Always restore stashed CR3 value (see paranoid_entry) */
	RESTORE_CR3 scratch_reg=%r15 save_reg=%r14

	testl	%ebx, %ebx			/* swapgs needed? */
	jnz	nmi_restore
nmi_swapgs:
	SWAPGS_UNSAFE_STACK
nmi_restore:
	POP_REGS

I might be missing something, but how is that supposed to work when
paranoid_entry uses FSGSBASE? I think it's broken, but if it's not then
there is a big fat comment missing explaining why.

Thanks,

	tglx






  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-15 20:06 [RESEND PATCH v6 00/12] x86: Enable FSGSBASE instructions Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 01/12] x86/fsgsbase/64: Add 'unsafe_fsgsbase' to enable CR4.FSGSBASE Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 02/12] kbuild: Raise the minimum required binutils version to 2.21 Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 03/12] x86/fsgsbase/64: Add intrinsics for FSGSBASE instructions Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions Chang S. Bae
2019-03-25 11:38   ` Thomas Gleixner
2019-03-25 12:46     ` Thomas Gleixner
2019-03-25 13:05       ` Thomas Gleixner
2019-03-26  0:38     ` Andi Kleen
2019-03-26 15:01       ` New feature/ABI review process [was Re: [RESEND PATCH v6 04/12] x86/fsgsbase/64:..] Thomas Gleixner
2019-03-26 22:56         ` Andi Kleen
2019-03-27 21:15           ` Thomas Gleixner
2019-03-15 20:06 ` [RESEND PATCH v6 05/12] x86/fsgsbase/64: Preserve FS/GS state in __switch_to() if FSGSBASE is on Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 06/12] x86/fsgsbase/64: When copying a thread, use the FSGSBASE instructions if available Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 07/12] x86/fsgsbase/64: Introduce the FIND_PERCPU_BASE macro Chang S. Bae
2019-03-25  9:02   ` Thomas Gleixner
2019-05-01 13:52     ` Bae, Chang Seok
2019-03-15 20:06 ` [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry Chang S. Bae
2019-03-25  9:44   ` Thomas Gleixner [this message]
2019-04-05  8:35     ` Thomas Gleixner
2019-04-05 13:50       ` Andy Lutomirski
2019-05-01 13:52         ` Bae, Chang Seok
2019-05-01 17:40           ` Andy Lutomirski
2019-05-01 18:01             ` Bae, Chang Seok
     [not found]               ` <7029A32B-958E-4C1E-8B5F-D49BA68E4755@intel.com>
2019-05-01 20:25                 ` Andy Lutomirski
2019-05-01 21:04                   ` Bae, Chang Seok
2019-05-02  0:29                     ` Andy Lutomirski
2019-05-06 22:56     ` Bae, Chang Seok
2019-03-15 20:06 ` [RESEND PATCH v6 09/12] selftests/x86/fsgsbase: Test WRGSBASE Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 10/12] x86/fsgsbase/64: Enable FSGSBASE by default and add a chicken bit Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 11/12] x86/elf: Enumerate kernel FSGSBASE capability in AT_HWCAP2 Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 12/12] x86/fsgsbase/64: Add documentation for FSGSBASE Chang S. Bae
2019-03-30 16:15   ` Randy Dunlap
2019-03-26  0:43 ` [RESEND PATCH v6 00/12] x86: Enable FSGSBASE instructions Andy Lutomirski

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=alpine.DEB.2.21.1903251003090.1798@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --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=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=ravi.v.shankar@intel.com \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.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/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git