Re: [PATCH v3 2/4] x86/syscalls: Specific usage of verify_pre_usermode_state

On Tue, Mar 14, 2017 at 8:17 AM, Thomas Garnier [off-list ref] wrote:

On Tue, Mar 14, 2017 at 2:40 AM, H. Peter Anvin [off-list ref] wrote:

quoted

On 03/13/17 17:04, H. Peter Anvin wrote:

quoted

On 03/11/17 01:42, Ingo Molnar wrote:

quoted

quoted

+   /*
+    * Check user-mode state on fast path return, the same check is done
+    * under the slow path through syscall_return_slowpath.
+    */
+#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
+   call    verify_pre_usermode_state
+#else
+   /*
+    * Similar to set_fs(USER_DS) in verify_pre_usermode_state without a
+    * warning.
+    */
+   movq    PER_CPU_VAR(current_task), %rax
+   movq    $TASK_SIZE_MAX, %rcx
+   cmp     %rcx, TASK_addr_limit(%rax)
+   jz      1f
+   movq    %rcx, TASK_addr_limit(%rax)
+1:
+#endif
+

How about simply doing...

      movq    PER_CPU_VAR(current_task), %rax
      movq    $TASK_SIZE_MAX, %rcx
#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
      cmpq    %rcx, TASK_addr_limit(%rax)
      jne     syscall_return_slowpath
#else
      movq    %rcx, TASK_addr_limit(%rax)
#endif

... and let the slow path take care of BUG.  This should be much faster,
even with the BUG, and is simpler to boot.

In fact, we could even to the cmpq/jne unconditionally.  I'm guessing
the occasional branch mispredict will be offset by occasionally touching
a clean cacheline in the case of an unconditional store.

Since this is something that should never happen, performance doesn't
matter.

Ingo: Which approach do you favor? I want to keep the fast path as
fast as possible obviously.

Even though my name isn't Ingo, Linus keeps trying to get me to be the
actual maintainer of this file.  :)  How about (sorry about whitespace
damage):

#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
       movq    PER_CPU_VAR(current_task), %rax
       bt $63, TASK_addr_limit(%rax)
       jc     syscall_return_slowpath
#endif

Now the kernel is totally unchanged if the config option is off and
it's fast and simple if the option is on.