linux-kernel - Re: [RESEND PATCH V5 0/8] x86: infrastructure to enable FSGSBASE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180914092732.GA90840@gmail.com>
Date:   Fri, 14 Sep 2018 11:27:32 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Markus T Metzger <markus.t.metzger@...el.com>,
        Ravi Shankar <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH V5 0/8] x86: infrastructure to enable FSGSBASE


* Chang S. Bae <chang.seok.bae@...el.com> wrote:

> Resending the patchset that was posted before [6].
> 
> Given feedbacks from [1], it was suggested to separate two parts
> and to (re-)submit this patchset first.
> 
> To facilitate FSGSBASE, Andy's FS/GS base read fix is first
> ordered, then some helper functions and refactoring work
> are included. Cleanup for the vDSO initialization is
> for preparing per-CPU base finder that will be used in the
> paranoid entry, when FSGSBASE enabled.
> 
> Changes from V4 [5]:
> * Change patch ordering; putting the fix first before introducing
> the helper functions
> * Cleanup further for vDSO CPU initialization codes
> 
> Changes from V3 [4]:
> * Unify CPU number initialization
> 
> Changes from V2 [3]:
> * Bisect the CPU number initialization patch
> * Drop patches for introducing i386 CPU_NUMBER and switching
> write_rdtscp_aux() to use wrmsr_safe()
> 
> Changes from V1 [2]:
> * Rename the x86-64 CPU_NUMBER segment from PER_CPU
> * Add i386 CPU_NUMBER equivalent to x86-64 at GDT entry 23
> * Add additional helper function to store CPU number
> * Switch write_rdtscp_aux() to use wrmsr_safe()
> 
> [1] FSGSBASE patch set V2: 
>     https://lore.kernel.org/patchwork/cover/912063/
> [2] infrastructure for FSGSBASE 
>     V1: https://lore.kernel.org/patchwork/cover/913139/
> [3] V2: https://lore.kernel.org/patchwork/cover/913644/
> [4] V3: https://lore.kernel.org/patchwork/cover/949775/
> [5] V4: https://lore.kernel.org/patchwork/cover/951712/
> [6] V5: https://lore.kernel.org/patchwork/cover/956526/
> 
> Andy Lutomirski (1):
>   x86/arch_prctl/64: Make ptrace read FS/GS base accurately
> 
> Chang S. Bae (7):
>   x86/fsgsbase/64: Introduce FS/GS base helper functions
>   x86/fsgsbase/64: Make ptrace use FS/GS base helpers
>   x86/fsgsbase/64: Use FS/GS base helpers in core dump
>   x86/fsgsbase/64: Factor out load FS/GS segments from __switch_to
>   x86/segments/64: Rename PER_CPU segment to CPU_NUMBER
>   x86/vdso: Introduce helper functions for CPU and node number
>   x86/vdso: Move out the CPU initialization
> 
>  arch/x86/entry/vdso/vgetcpu.c   |   9 +-
>  arch/x86/entry/vdso/vma.c       |  38 +--------
>  arch/x86/include/asm/elf.h      |   6 +-
>  arch/x86/include/asm/fsgsbase.h |  47 +++++++++++
>  arch/x86/include/asm/segment.h  |  46 +++++++++-
>  arch/x86/include/asm/vgtod.h    |  26 ------
>  arch/x86/kernel/cpu/common.c    |  24 ++++++
>  arch/x86/kernel/process_64.c    | 183 +++++++++++++++++++++++++++++++---------
>  arch/x86/kernel/ptrace.c        |  28 ++----
>  9 files changed, 270 insertions(+), 137 deletions(-)
>  create mode 100644 arch/x86/include/asm/fsgsbase.h

Looks mostly good, here's a couple of details I noticed before I almost applied the series:


 - Please use a single, unified name-space for all the new FS/GS helpers, such as:

	x86_fsbase_read_cpu()
	x86_fsbase_read_task()
	x86_fsbase_write_cpu()
	x86_fsbase_write_cpu_inactive()
	x86_fsbase_write_task()

	x86_gsbase_read_cpu()
	x86_gsbase_read_task()
	x86_gsbase_write_task()

	x86_fsgsbase_load()		# was: load_fsgs()
	x86_fsgsbase_read_task()	# was: task_seg_base()

        etc.

 - Please port patch #7 to latest -tip, it's conflicting with:

     e78e5a91456f: x86/vdso: Fix lsl operand order

   The resolution is to preserve the modified code.

Also some style nits:

 - Please fix the inconsistent capitalization of 'CPU' in various places, comments, 
   descriptions, etc.

 - Comments should start with a capital letter, like sentences do. Right now there's a mixed 
   style of:

+               /*
+                * If performance here mattered, we could protect the LDT
+                * with RCU.  This is a slow path, though, so we can just
+                * take the mutex.
+                */

+       /*
+        * 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.
+        */


 - Please refer to functions in changelogs and comments with fn_name(), not just fn_name. For 
   example:

      x86/fsgsbase/64: Factor out load FS/GS segments from __switch_to

   But there's more examples.

 - Please read your own patches once again and fix typos in comments and changelogs, such as:

+        * correctly wrt barrier() and to keep gcc from cleverly


Thanks,

	Ingo