[PATCH 00/25] objtool: UACCESS validation v4

[PATCH 00/25] objtool: UACCESS validation v4

[Peter Zijlstra]

Teach objtool to validate the UACCESS (SMAP, PAN) rules which are currently
unenforced and (therefore obviously) violated.

UACCESS sections should be small; we want to limit the amount of code that can
touch userspace. Furthermore, UACCESS state isn't scheduled, this means that
anything that directly calls into the scheduler will result in random code
running with UACCESS enabled and possibly getting back into the UACCESS region
with UACCESS disabled and causing faults.

Forbid any CALL/RET while UACCESS is enabled; but provide a few exceptions.

This builds x86_64-allmodconfig and lots of x86_64-randconfig clean.

Changes since -v3:

 - removed a bunch of functions from the UACCESS-safe list
   due to the removal of CONFIG_KASAN_EXTRA=y.

 - hopefully addressed all the feedback from Josh

 - realized objtool doesn't cover x86_32

 - some added additional annotations/fixes: kcov, signal

 - retains the DF check for now, Linus, do you (still) think it is worth doing
   that DF check?

---
 arch/x86/Kconfig                           |   2 +
 arch/x86/ia32/ia32_signal.c                |  29 ++-
 arch/x86/include/asm/alternative-asm.h     |  11 +
 arch/x86/include/asm/alternative.h         |  10 +
 arch/x86/include/asm/asm.h                 |  24 --
 arch/x86/include/asm/nospec-branch.h       |  28 +-
 arch/x86/include/asm/smap.h                |  37 ++-
 arch/x86/include/asm/uaccess.h             |   5 +-
 arch/x86/include/asm/uaccess_64.h          |   3 -
 arch/x86/include/asm/xen/hypercall.h       |  24 +-
 arch/x86/kernel/signal.c                   |  29 ++-
 arch/x86/lib/copy_user_64.S                |  48 ++++
 arch/x86/lib/memcpy_64.S                   |   3 +-
 arch/x86/lib/usercopy_64.c                 |  20 --
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   6 +-
 include/linux/compiler.h                   |   2 +-
 include/linux/uaccess.h                    |   2 +
 kernel/Makefile                            |   1 +
 kernel/trace/trace_branch.c                |   4 +
 lib/Makefile                               |   1 +
 lib/ubsan.c                                |   4 +
 mm/kasan/Makefile                          |   3 +
 mm/kasan/common.c                          |  10 +
 mm/kasan/report.c                          |   3 +-
 scripts/Makefile.build                     |   3 +
 tools/objtool/arch.h                       |   8 +-
 tools/objtool/arch/x86/decode.c            |  21 +-
 tools/objtool/builtin-check.c              |   4 +-
 tools/objtool/builtin.h                    |   2 +-
 tools/objtool/check.c                      | 400 ++++++++++++++++++++++-------
 tools/objtool/check.h                      |   4 +-
 tools/objtool/elf.c                        |  15 +-
 tools/objtool/elf.h                        |   3 +-
 tools/objtool/special.c                    |  18 ++
 tools/objtool/special.h                    |   1 +
 tools/objtool/warn.h                       |   8 +
 36 files changed, 584 insertions(+), 212 deletions(-)

[PATCH 01/25] x86: Make SMAP 64-bit only

[Peter Zijlstra]

We rely on objtool to verify AC=1 doesn't escape. However there is no
objtool support for x86_32, and thus we cannot guarantee the
correctness of the 32bit code.

Also; if you're running 32bit kernels on hardware with SMAP (which all
should have LM support afaik) you're doing it wrong anyway.

XXX: we could do the PUSHF/POPF thing in __switch_to_asm() on x86_32,
but why bother.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1853,6 +1853,8 @@ config ARCH_RANDOM
 
 config X86_SMAP
 	def_bool y
+	# Note: we rely on objtool to validate AC=1 doesn't escape
+	depends on HAVE_STACK_VALIDATION
 	prompt "Supervisor Mode Access Prevention" if EXPERT
 	---help---
 	  Supervisor Mode Access Prevention (SMAP) is a security

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Linus Torvalds]

On Mon, Mar 18, 2019 at 8:54 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> We rely on objtool to verify AC=1 doesn't escape. However there is no
> objtool support for x86_32, and thus we cannot guarantee the
> correctness of the 32bit code.

Absolutely not.

This is just crazy. We had working SMAP long before objtool, and we
will have it regardless of objtool.

This is like saying "ok, I don't have an oxygen sensor, so I don't
know that the air I'm breathing is sufficient to maintain life, so
I'll just stop breathing".

So no way in hell do we make SMAP go away on 32-bit for no sane reason
what-so-ever.

Besides, the x86-64 objtool coverage will cover 99% of all 32-bit code
too, simply because we share it. In fact, it will cover most of the
code for other architectures too.

                  Linus

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Peter Zijlstra]

On Mon, Mar 18, 2019 at 09:58:20AM -0700, Linus Torvalds wrote:
> On Mon, Mar 18, 2019 at 8:54 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > We rely on objtool to verify AC=1 doesn't escape. However there is no
> > objtool support for x86_32, and thus we cannot guarantee the
> > correctness of the 32bit code.
> 
> Absolutely not.
> 
> This is just crazy. We had working SMAP long before objtool, and we
> will have it regardless of objtool.

Well, 'working', because as shown with this work, there's been a number
of bugs around this.

Anyway, since you mentioned it, I checked, about 16 months: Broadwell
was Oct'14, objtool was Feb'16.

> This is like saying "ok, I don't have an oxygen sensor, so I don't
> know that the air I'm breathing is sufficient to maintain life, so
> I'll just stop breathing".

With PTI, booting a 32bit kernel on a Broadwell or later will already
complain about it being sub-optimal for missing out on PCID. But sure;
if you want to enable this..

> So no way in hell do we make SMAP go away on 32-bit for no sane reason
> what-so-ever.

then I'd sleep much better if we make 32bit context switch EFLAGS.
Because, yes, x86_64 objtool validates a lot of the x86_32 code too, but
unless we have 100% coverage, there's always the chance an AC=1 escapes.

---
 arch/x86/entry/entry_32.S        | 2 ++
 arch/x86/include/asm/switch_to.h | 1 +
 arch/x86/kernel/process_32.c     | 5 +++++
 3 files changed, 8 insertions(+)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d309f30cf7af..5fc76b755510 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -650,6 +650,7 @@ ENTRY(__switch_to_asm)
 	pushl	%ebx
 	pushl	%edi
 	pushl	%esi
+	pushfl
 
 	/* switch stack */
 	movl	%esp, TASK_threadsp(%eax)
@@ -672,6 +673,7 @@ ENTRY(__switch_to_asm)
 #endif
 
 	/* restore callee-saved registers */
+	popfl
 	popl	%esi
 	popl	%edi
 	popl	%ebx
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 7cf1a270d891..18a4b6890fa8 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -46,6 +46,7 @@ struct inactive_task_frame {
 	unsigned long r13;
 	unsigned long r12;
 #else
+	unsigned long flags;
 	unsigned long si;
 	unsigned long di;
 #endif
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index e471d8e6f0b2..d28e9a74b736 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -127,6 +127,11 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	struct task_struct *tsk;
 	int err;
 
+	/*
+	 * We start a new task with the RESET value of EFLAGS, in particular we
+	 * context switch with IRQs disabled.
+	 */
+	frame->flags = X86_EFLAGS_FIXED;
 	frame->bp = 0;
 	frame->ret_addr = (unsigned long) ret_from_fork;
 	p->thread.sp = (unsigned long) fork_frame;

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Peter Zijlstra]

On Mon, Mar 18, 2019 at 06:36:57PM +0100, Peter Zijlstra wrote:
> then I'd sleep much better if we make 32bit context switch EFLAGS.
> Because, yes, x86_64 objtool validates a lot of the x86_32 code too, but
> unless we have 100% coverage, there's always the chance an AC=1 escapes.

How about I do a patch that schedules EFLAGS for both 32bit and 64bit,
mark this for backporting to infinity.

And then at the end, after the objtool-ac bits land, I do a patch
removing the EFLAGS scheduling for x86_64.

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Linus Torvalds]

On Mon, Mar 18, 2019 at 10:51 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> How about I do a patch that schedules EFLAGS for both 32bit and 64bit,
> mark this for backporting to infinity.
>
> And then at the end, after the objtool-ac bits land, I do a patch
> removing the EFLAGS scheduling for x86_64.

Sounds sane to me.

And we can make it AC-conditional if it's actually shown to be visible
from a performance standpoint.

But iirc pushf/popf isn't really that expensive - in fact I think it's
pretty cheap when system flags don't change. Which would be the common
case unless you'd also make the popf do the irq restore part and
simply make finish_lock_switch() re-enable irq's by doing an
irqrestore?

I think popf is like 20 cycles or something (and pushf is just single
cycles). Probably not worth worrying about in the task switch context.

                 Linus

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[hpa]

On March 18, 2019 11:10:22 AM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Mon, Mar 18, 2019 at 10:51 AM Peter Zijlstra <peterz@infradead.org>
>wrote:
>>
>> How about I do a patch that schedules EFLAGS for both 32bit and
>64bit,
>> mark this for backporting to infinity.
>>
>> And then at the end, after the objtool-ac bits land, I do a patch
>> removing the EFLAGS scheduling for x86_64.
>
>Sounds sane to me.
>
>And we can make it AC-conditional if it's actually shown to be visible
>from a performance standpoint.
>
>But iirc pushf/popf isn't really that expensive - in fact I think it's
>pretty cheap when system flags don't change. Which would be the common
>case unless you'd also make the popf do the irq restore part and
>simply make finish_lock_switch() re-enable irq's by doing an
>irqrestore?
>
>I think popf is like 20 cycles or something (and pushf is just single
>cycles). Probably not worth worrying about in the task switch context.
>
>                 Linus

Yes, the problem isn't scheduling per se but the risk of hiding problems.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Denys Vlasenko]

On 3/18/19 7:10 PM, Linus Torvalds wrote:
> On Mon, Mar 18, 2019 at 10:51 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> How about I do a patch that schedules EFLAGS for both 32bit and 64bit,
>> mark this for backporting to infinity.
>>
>> And then at the end, after the objtool-ac bits land, I do a patch
>> removing the EFLAGS scheduling for x86_64.
> 
> Sounds sane to me.
> 
> And we can make it AC-conditional if it's actually shown to be visible
> from a performance standpoint.
> 
> But iirc pushf/popf isn't really that expensive - in fact I think it's
> pretty cheap when system flags don't change.

I did not see evidence of this. In my testing,
POPF is always ~20 cycles, even if popped flags are identical to current
state of flags.

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[hpa]

On March 21, 2019 10:25:05 AM PDT, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>On 3/18/19 7:10 PM, Linus Torvalds wrote:
>> On Mon, Mar 18, 2019 at 10:51 AM Peter Zijlstra
><peterz@infradead.org> wrote:
>>>
>>> How about I do a patch that schedules EFLAGS for both 32bit and
>64bit,
>>> mark this for backporting to infinity.
>>>
>>> And then at the end, after the objtool-ac bits land, I do a patch
>>> removing the EFLAGS scheduling for x86_64.
>> 
>> Sounds sane to me.
>> 
>> And we can make it AC-conditional if it's actually shown to be
>visible
>> from a performance standpoint.
>> 
>> But iirc pushf/popf isn't really that expensive - in fact I think
>it's
>> pretty cheap when system flags don't change.
>
>I did not see evidence of this. In my testing,
>POPF is always ~20 cycles, even if popped flags are identical to
>current
>state of flags.

I think you will find that if you change system flags it is much slower.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Peter Zijlstra]

On Thu, Mar 21, 2019 at 11:18:05AM -0700, hpa@zytor.com wrote:
> On March 21, 2019 10:25:05 AM PDT, Denys Vlasenko <dvlasenk@redhat.com> wrote:

> >I did not see evidence of this. In my testing,
> >POPF is always ~20 cycles, even if popped flags are identical to
> >current state of flags.
> 
> I think you will find that if you change system flags it is much slower.

So with all the patches in this series applied, only x86_32 will suffer
this, and I don't think anybody still considers that a performance
critical platform.

That said, we could do something terrible like:

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -673,7 +673,8 @@ ENTRY(__switch_to_asm)
 #endif
 
        /* restore callee-saved registers */
-       popfl
+       ALTERNATIVE "popl %esi", \
+                   "popfl", X86_FEATURE_SMAP
        popl    %esi
        popl    %edi
        popl    %ebx

And then you only pay the POPF penalty when you run a 32bit kernel on a
SMAP enabled CPU, and we have a very good solution in that code: run a
64bit kernel.

Re: [PATCH 01/25] x86: Make SMAP 64-bit only

[Linus Torvalds]

On Thu, Mar 21, 2019 at 10:25 AM Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> >
> > But iirc pushf/popf isn't really that expensive - in fact I think it's
> > pretty cheap when system flags don't change.
>
> I did not see evidence of this. In my testing,
> POPF is always ~20 cycles, even if popped flags are identical to current
> state of flags.

It may have been an artifact on just some older CPU's. I have this
distinct memory of popf that changed IF being more expensive, but
maybe that was the Pentium 4 days.

Or maybe it's just that my memory is garbage.

               Linus

[PATCH 01/25] sched/x86: Save [ER]FLAGS on context switch

[Peter Zijlstra]

New patch #1

---

Subject: sched/x86: Save [ER]FLAGS on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Feb 14 10:30:52 CET 2019

Effectively reverts commit:

  2c7577a75837 ("sched/x86_64: Don't save flags on context switch")

Specifically because SMAP uses FLAGS.AC which invalidates the claim
that the kernel has clean flags.

In particular; while preemption from interrupt return is fine (the
IRET frame on the exception stack contains FLAGS) it breaks any code
that does synchonous scheduling, including preempt_enable().

This has become a significant issue ever since commit:

  5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")

provided for means of having 'normal' C code between STAC / CLAC,
exposing the FLAGS.AC state. So far this hasn't led to trouble,
however fix it before it comes apart.

Fixes: 5b24a7a2aa20 ("Add 'unsafe' user access functions for batched accesses")
Acked-by: Andy Lutomirski <luto@amacapital.net>
Reported-by: Julien Thierry <julien.thierry@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_32.S        |    2 ++
 arch/x86/entry/entry_64.S        |    2 ++
 arch/x86/include/asm/switch_to.h |    1 +
 arch/x86/kernel/process_32.c     |    7 +++++++
 arch/x86/kernel/process_64.c     |    8 ++++++++
 5 files changed, 20 insertions(+)

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -650,6 +650,7 @@ ENTRY(__switch_to_asm)
 	pushl	%ebx
 	pushl	%edi
 	pushl	%esi
+	pushfl
 
 	/* switch stack */
 	movl	%esp, TASK_threadsp(%eax)
@@ -672,6 +673,7 @@ ENTRY(__switch_to_asm)
 #endif
 
 	/* restore callee-saved registers */
+	popfl
 	popl	%esi
 	popl	%edi
 	popl	%ebx
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -291,6 +291,7 @@ ENTRY(__switch_to_asm)
 	pushq	%r13
 	pushq	%r14
 	pushq	%r15
+	pushfq
 
 	/* switch stack */
 	movq	%rsp, TASK_threadsp(%rdi)
@@ -313,6 +314,7 @@ ENTRY(__switch_to_asm)
 #endif
 
 	/* restore callee-saved registers */
+	popfq
 	popq	%r15
 	popq	%r14
 	popq	%r13
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -40,6 +40,7 @@ asmlinkage void ret_from_fork(void);
  * order of the fields must match the code in __switch_to_asm().
  */
 struct inactive_task_frame {
+	unsigned long flags;
 #ifdef CONFIG_X86_64
 	unsigned long r15;
 	unsigned long r14;
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -127,6 +127,13 @@ int copy_thread_tls(unsigned long clone_
 	struct task_struct *tsk;
 	int err;
 
+	/*
+	 * For a new task use the RESET flags value since there is no before.
+	 * All the status flags are zero; DF and all the system flags must also
+	 * be 0, specifically IF must be 0 because we context switch to the new
+	 * task with interrupts disabled.
+	 */
+	frame->flags = X86_EFLAGS_FIXED;
 	frame->bp = 0;
 	frame->ret_addr = (unsigned long) ret_from_fork;
 	p->thread.sp = (unsigned long) fork_frame;
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -392,6 +392,14 @@ int copy_thread_tls(unsigned long clone_
 	childregs = task_pt_regs(p);
 	fork_frame = container_of(childregs, struct fork_frame, regs);
 	frame = &fork_frame->frame;
+
+	/*
+	 * For a new task use the RESET flags value since there is no before.
+	 * All the status flags are zero; DF and all the system flags must also
+	 * be 0, specifically IF must be 0 because we context switch to the new
+	 * task with interrupts disabled.
+	 */
+	frame->flags = X86_EFLAGS_FIXED;
 	frame->bp = 0;
 	frame->ret_addr = (unsigned long) ret_from_fork;
 	p->thread.sp = (unsigned long) fork_frame;

[PATCH 02/25] tracing: Improve "if" macro code generation

[Peter Zijlstra]

With CONFIG_PROFILE_ALL_BRANCHES, the "if" macro converts the
conditional to an array index.  This can cause GCC to create horrible
code.  When there are nested ifs, the generated code uses register
values to encode branching decisions.

Make it easier for GCC to optimize by keeping the conditional as a
conditional rather than converting it to an integer.  This shrinks the
generated code quite a bit, and also makes the code sane enough for
objtool to understand.

Cc: rostedt@goodmis.org
Cc: valentin.schneider@arm.com
Cc: luto@amacapital.net
Cc: brgerst@gmail.com
Cc: catalin.marinas@arm.com
Cc: mingo@kernel.org
Cc: dvlasenk@redhat.com
Cc: will.deacon@arm.com
Cc: julien.thierry@arm.com
Cc: torvalds@linux-foundation.org
Cc: dvyukov@google.com
Cc: bp@alien8.de
Cc: tglx@linutronix.de
Cc: james.morse@arm.com
Cc: luto@kernel.org
Cc: hpa@zytor.com
Reported-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20190307174802.46fmpysxyo35hh43@treble
---
 include/linux/compiler.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -67,7 +67,7 @@ void ftrace_likely_update(struct ftrace_
 				.line = __LINE__,			\
 			};						\
 		______r = !!(cond);					\
-		______f.miss_hit[______r]++;					\
+		______r ? ______f.miss_hit[1]++ : ______f.miss_hit[0]++;\
 		______r;						\
 	}))
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Mon, 18 Mar 2019 16:38:42 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> With CONFIG_PROFILE_ALL_BRANCHES, the "if" macro converts the
> conditional to an array index.  This can cause GCC to create horrible
> code.  When there are nested ifs, the generated code uses register
> values to encode branching decisions.
> 
> Make it easier for GCC to optimize by keeping the conditional as a
> conditional rather than converting it to an integer.  This shrinks the
> generated code quite a bit, and also makes the code sane enough for
> objtool to understand.
> 
> Cc: rostedt@goodmis.org
> Cc: valentin.schneider@arm.com
> Cc: luto@amacapital.net
> Cc: brgerst@gmail.com
> Cc: catalin.marinas@arm.com
> Cc: mingo@kernel.org
> Cc: dvlasenk@redhat.com
> Cc: will.deacon@arm.com
> Cc: julien.thierry@arm.com
> Cc: torvalds@linux-foundation.org
> Cc: dvyukov@google.com
> Cc: bp@alien8.de
> Cc: tglx@linutronix.de
> Cc: james.morse@arm.com
> Cc: luto@kernel.org
> Cc: hpa@zytor.com
> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> Link: https://lkml.kernel.org/r/20190307174802.46fmpysxyo35hh43@treble
> ---
>  include/linux/compiler.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -67,7 +67,7 @@ void ftrace_likely_update(struct ftrace_
>  				.line = __LINE__,			\
>  			};						\
>  		______r = !!(cond);					\
> -		______f.miss_hit[______r]++;					\
> +		______r ? ______f.miss_hit[1]++ : ______f.miss_hit[0]++;\
>  		______r;						\
>  	}))
>  #endif /* CONFIG_PROFILE_ALL_BRANCHES */
> 

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Josh Poimboeuf]

On Mon, Mar 18, 2019 at 04:38:42PM +0100, Peter Zijlstra wrote:
> With CONFIG_PROFILE_ALL_BRANCHES, the "if" macro converts the
> conditional to an array index.  This can cause GCC to create horrible
> code.  When there are nested ifs, the generated code uses register
> values to encode branching decisions.
> 
> Make it easier for GCC to optimize by keeping the conditional as a
> conditional rather than converting it to an integer.  This shrinks the
> generated code quite a bit, and also makes the code sane enough for
> objtool to understand.
> 
> Cc: rostedt@goodmis.org
> Cc: valentin.schneider@arm.com
> Cc: luto@amacapital.net
> Cc: brgerst@gmail.com
> Cc: catalin.marinas@arm.com
> Cc: mingo@kernel.org
> Cc: dvlasenk@redhat.com
> Cc: will.deacon@arm.com
> Cc: julien.thierry@arm.com
> Cc: torvalds@linux-foundation.org
> Cc: dvyukov@google.com
> Cc: bp@alien8.de
> Cc: tglx@linutronix.de
> Cc: james.morse@arm.com
> Cc: luto@kernel.org
> Cc: hpa@zytor.com
> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20190307174802.46fmpysxyo35hh43@treble

Missing "From" me.

-- 
Josh

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Peter Zijlstra]

On Mon, Mar 18, 2019 at 06:37:20PM -0500, Josh Poimboeuf wrote:
> On Mon, Mar 18, 2019 at 04:38:42PM +0100, Peter Zijlstra wrote:
> > With CONFIG_PROFILE_ALL_BRANCHES, the "if" macro converts the
> > conditional to an array index.  This can cause GCC to create horrible
> > code.  When there are nested ifs, the generated code uses register
> > values to encode branching decisions.
> > 
> > Make it easier for GCC to optimize by keeping the conditional as a
> > conditional rather than converting it to an integer.  This shrinks the
> > generated code quite a bit, and also makes the code sane enough for
> > objtool to understand.
> > 
> > Cc: rostedt@goodmis.org
> > Cc: valentin.schneider@arm.com
> > Cc: luto@amacapital.net
> > Cc: brgerst@gmail.com
> > Cc: catalin.marinas@arm.com
> > Cc: mingo@kernel.org
> > Cc: dvlasenk@redhat.com
> > Cc: will.deacon@arm.com
> > Cc: julien.thierry@arm.com
> > Cc: torvalds@linux-foundation.org
> > Cc: dvyukov@google.com
> > Cc: bp@alien8.de
> > Cc: tglx@linutronix.de
> > Cc: james.morse@arm.com
> > Cc: luto@kernel.org
> > Cc: hpa@zytor.com
> > Reported-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: https://lkml.kernel.org/r/20190307174802.46fmpysxyo35hh43@treble
> 
> Missing "From" me.

Right, sorry, quilt eats that and I sometimes forget to fix up :/

RE: [PATCH 02/25] tracing: Improve "if" macro code generation

[David Laight]

From: > Peter Zijlstra
> Sent: 18 March 2019 15:39
> 
> With CONFIG_PROFILE_ALL_BRANCHES, the "if" macro converts the
> conditional to an array index.  This can cause GCC to create horrible
> code.  When there are nested ifs, the generated code uses register
> values to encode branching decisions.
> 
> Make it easier for GCC to optimize by keeping the conditional as a
> conditional rather than converting it to an integer.  This shrinks the
> generated code quite a bit, and also makes the code sane enough for
> objtool to understand.
...
>  include/linux/compiler.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -67,7 +67,7 @@ void ftrace_likely_update(struct ftrace_
>  				.line = __LINE__,			\
>  			};						\
>  		______r = !!(cond);					\

	Is that (or maybe just the !!) needed any more??

> -		______f.miss_hit[______r]++;					\
> +		______r ? ______f.miss_hit[1]++ : ______f.miss_hit[0]++;\
>  		______r;						\
>  	}))
>  #endif /* CONFIG_PROFILE_ALL_BRANCHES */
> 

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Wed, Mar 20, 2019 at 4:17 AM David Laight <David.Laight@aculab.com> wrote:
>
> >               ______r = !!(cond);                                     \
>
>         Is that (or maybe just the !!) needed any more??

It is, because the 'cond' expression might not be an int, it could be
a test for a pointer being non-NULL, or an u64 being non-zero, and not
having the "!!" would mean that you'd get a warning or drop bits when
assigning to 'int'.

And you do need the new temporary variable to avoid double evaluation
the way that code is written.

That said, I do think the code is really ugly. We could:

 - avoid the temporary by just simplifying things.

 - do the '!!' just once in the parent macro.

 - Steven has this crazy model of "more underscores are better". They
aren't. They don't help if things nest anyway, but what does help is
meaningful names. Both when things don't nest, and when looking at
generated asm files.

 - ,, and finally, what _is_ better is to chop things up so that they
are smaller and make each macro do only one thing

So maybe do the patch something like the attached instead? Completely
untested, but it looks sane to me.

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1490 bytes --]

 include/linux/compiler.h | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 445348facea9..8aaf7cd026b0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -53,23 +53,24 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
  * "Define 'is'", Bill Clinton
  * "Define 'if'", Steven Rostedt
  */
-#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
-#define __trace_if(cond) \
-	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
-	({								\
-		int ______r;						\
-		static struct ftrace_branch_data			\
-			__aligned(4)					\
-			__section("_ftrace_branch")			\
-			______f = {					\
-				.func = __func__,			\
-				.file = __FILE__,			\
-				.line = __LINE__,			\
-			};						\
-		______r = !!(cond);					\
-		______f.miss_hit[______r]++;					\
-		______r;						\
-	}))
+#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
+
+#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
+
+#define __trace_if_value(cond) ({			\
+	static struct ftrace_branch_data		\
+		__aligned(4)				\
+		__section("_ftrace_branch")		\
+		__if_trace = {				\
+			.func = __func__,		\
+			.file = __FILE__,		\
+			.line = __LINE__,		\
+		};					\
+	(cond) ?					\
+		(__if_trace.miss_hit[1]++,1) :		\
+		(__if_trace.miss_hit[0]++,0);		\
+})
+
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */
 
 #else

RE: [PATCH 02/25] tracing: Improve "if" macro code generation

[David Laight]

From: Linus Torvalds 
> Sent: 20 March 2019 17:26
> To: David Laight
> On Wed, Mar 20, 2019 at 4:17 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > >               ______r = !!(cond);                                     \
> >
> >         Is that (or maybe just the !!) needed any more??
> 
> It is, because the 'cond' expression might not be an int, it could be
> a test for a pointer being non-NULL, or an u64 being non-zero, and not
> having the "!!" would mean that you'd get a warning or drop bits when
> assigning to 'int'.
> 
> And you do need the new temporary variable to avoid double evaluation
> the way that code is written.

As usual I'd opened my mouth before checking the full context :-)

>           ______r = !!(cond);                                     
> -		______f.miss_hit[______r]++;					\
> +		______r ? ______f.miss_hit[1]++ : ______f.miss_hit[0]++;\
>  		______r;						\

Actually you can avoid double evaluation by doing:

		(cond) ? (______f.miss_hit[1]++, 1) : (______f.miss_hit[0]++, 0)

With luck the compiler will move the increment to after the branch target.

for (_____ = ____; _____ < ______; _____++) :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Linus Torvalds]

On Wed, Mar 20, 2019 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
>
>
> Actually you can avoid double evaluation by doing:
>
>                 (cond) ? (______f.miss_hit[1]++, 1) : (______f.miss_hit[0]++, 0)

I don't think you looked at the patch in my attachment of the email
you replied to, did you?

That's exactly what it does, except it also gets rid of those
completely pointless and insane underscores, and makes the code more
legible in other ways too.

                 Linus

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Wed, 20 Mar 2019 10:26:17 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:


>  - Steven has this crazy model of "more underscores are better". They
> aren't. They don't help if things nest anyway, but what does help is
> meaningful names. Both when things don't nest, and when looking at
> generated asm files.

Note, at the time I had Ingo as a mentor ;-)

>  include/linux/compiler.h | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 445348facea9..8aaf7cd026b0 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -53,23 +53,24 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
>   * "Define 'is'", Bill Clinton
>   * "Define 'if'", Steven Rostedt
>   */
> -#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
> -#define __trace_if(cond) \
> -	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
> -	({								\
> -		int ______r;						\
> -		static struct ftrace_branch_data			\
> -			__aligned(4)					\
> -			__section("_ftrace_branch")			\
> -			______f = {					\
> -				.func = __func__,			\
> -				.file = __FILE__,			\
> -				.line = __LINE__,			\
> -			};						\
> -		______r = !!(cond);					\
> -		______f.miss_hit[______r]++;					\
> -		______r;						\
> -	}))
> +#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
> +
> +#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
> +
> +#define __trace_if_value(cond) ({			\
> +	static struct ftrace_branch_data		\
> +		__aligned(4)				\
> +		__section("_ftrace_branch")		\
> +		__if_trace = {				\
> +			.func = __func__,		\
> +			.file = __FILE__,		\
> +			.line = __LINE__,		\
> +		};					\
> +	(cond) ?					\
> +		(__if_trace.miss_hit[1]++,1) :		\
> +		(__if_trace.miss_hit[0]++,0);		\

Probably want to add a space between the comma and the numbers.

But other than that.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> +})
> +
>  #endif /* CONFIG_PROFILE_ALL_BRANCHES */
>  
>  #else

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Wed, 20 Mar 2019 10:26:17 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Mar 20, 2019 at 4:17 AM David Laight <David.Laight@aculab.com> wrote:
> >  
> > >               ______r = !!(cond);                                     \  
> >
> >         Is that (or maybe just the !!) needed any more??  
> 
> It is, because the 'cond' expression might not be an int, it could be
> a test for a pointer being non-NULL, or an u64 being non-zero, and not
> having the "!!" would mean that you'd get a warning or drop bits when
> assigning to 'int'.
> 
> And you do need the new temporary variable to avoid double evaluation
> the way that code is written.
> 
> That said, I do think the code is really ugly. We could:
> 
>  - avoid the temporary by just simplifying things.
> 
>  - do the '!!' just once in the parent macro.
> 
>  - Steven has this crazy model of "more underscores are better". They
> aren't. They don't help if things nest anyway, but what does help is
> meaningful names. Both when things don't nest, and when looking at
> generated asm files.
> 
>  - ,, and finally, what _is_ better is to chop things up so that they
> are smaller and make each macro do only one thing
> 
> So maybe do the patch something like the attached instead? Completely
> untested, but it looks sane to me.
> 

Linus,

This patch works. Can I get your Signed-off-by for it?

-- Steve

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Linus Torvalds]

On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> This patch works. Can I get your Signed-off-by for it?

Yes. Please write some kind of comprehensible commit log for it, but

   Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

for the patch itself.

             Linus

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Thu, 9 May 2019 09:51:59 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > This patch works. Can I get your Signed-off-by for it?  
> 
> Yes. Please write some kind of comprehensible commit log for it, but

How's this:

"Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
macro converts the conditional to an array index.  This can cause GCC
to create horrible code.  When there are nested ifs, the generated code
uses register values to encode branching decisions.

Josh Poimboeuf found that replacing the define "if" macro from using
the condition as an array index and incrementing the branch statics
with an if statement itself, reduced the asm complexity and shrinks the
generated code quite a bit.

But this can be simplified even further by replacing the internal if
statement with a ternary operator.

Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
"

> 
>    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Thanks,

-- Steve

> 
> for the patch itself.
> 

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Josh Poimboeuf]

On Thu, May 09, 2019 at 02:29:02PM -0400, Steven Rostedt wrote:
> On Thu, 9 May 2019 09:51:59 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > This patch works. Can I get your Signed-off-by for it?  
> > 
> > Yes. Please write some kind of comprehensible commit log for it, but
> 
> How's this:
> 
> "Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
> macro converts the conditional to an array index.  This can cause GCC
> to create horrible code.  When there are nested ifs, the generated code
> uses register values to encode branching decisions.
> 
> Josh Poimboeuf found that replacing the define "if" macro from using
> the condition as an array index and incrementing the branch statics
> with an if statement itself, reduced the asm complexity and shrinks the
> generated code quite a bit.
> 
> But this can be simplified even further by replacing the internal if
> statement with a ternary operator.
> 
> Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>

Actually, my original fix already went in:

  37686b1353cf ("tracing: Improve "if" macro code generation")

But it introduced a regression:

  https://lkml.kernel.org/r/201905040509.iqQ2CrOU%lkp@intel.com

which Linus' patch fixes for some reason.

-- 
Josh

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Josh Poimboeuf]

On Thu, May 09, 2019 at 01:45:31PM -0500, Josh Poimboeuf wrote:
> On Thu, May 09, 2019 at 02:29:02PM -0400, Steven Rostedt wrote:
> > On Thu, 9 May 2019 09:51:59 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > > On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > >
> > > > This patch works. Can I get your Signed-off-by for it?  
> > > 
> > > Yes. Please write some kind of comprehensible commit log for it, but
> > 
> > How's this:
> > 
> > "Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
> > macro converts the conditional to an array index.  This can cause GCC
> > to create horrible code.  When there are nested ifs, the generated code
> > uses register values to encode branching decisions.
> > 
> > Josh Poimboeuf found that replacing the define "if" macro from using
> > the condition as an array index and incrementing the branch statics
> > with an if statement itself, reduced the asm complexity and shrinks the
> > generated code quite a bit.
> > 
> > But this can be simplified even further by replacing the internal if
> > statement with a ternary operator.
> > 
> > Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Actually, my original fix already went in:
> 
>   37686b1353cf ("tracing: Improve "if" macro code generation")
> 
> But it introduced a regression:
> 
>   https://lkml.kernel.org/r/201905040509.iqQ2CrOU%lkp@intel.com
> 
> which Linus' patch fixes for some reason.

/me curses URL encoding

https://lkml.kernel.org/r/201905040509.iqQ2CrOU%25lkp@intel.com

-- 
Josh

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Randy Dunlap]

On 5/9/19 11:47 AM, Josh Poimboeuf wrote:
> On Thu, May 09, 2019 at 01:45:31PM -0500, Josh Poimboeuf wrote:
>> On Thu, May 09, 2019 at 02:29:02PM -0400, Steven Rostedt wrote:
>>> On Thu, 9 May 2019 09:51:59 -0700
>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>>>>>
>>>>> This patch works. Can I get your Signed-off-by for it?  
>>>>
>>>> Yes. Please write some kind of comprehensible commit log for it, but
>>>
>>> How's this:
>>>
>>> "Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
>>> macro converts the conditional to an array index.  This can cause GCC
>>> to create horrible code.  When there are nested ifs, the generated code
>>> uses register values to encode branching decisions.
>>>
>>> Josh Poimboeuf found that replacing the define "if" macro from using
>>> the condition as an array index and incrementing the branch statics
>>> with an if statement itself, reduced the asm complexity and shrinks the
>>> generated code quite a bit.
>>>
>>> But this can be simplified even further by replacing the internal if
>>> statement with a ternary operator.
>>>
>>> Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>
>> Actually, my original fix already went in:
>>
>>   37686b1353cf ("tracing: Improve "if" macro code generation")
>>
>> But it introduced a regression:
>>
>>   https://lkml.kernel.org/r/201905040509.iqQ2CrOU%lkp@intel.com
>>
>> which Linus' patch fixes for some reason.
> 
> /me curses URL encoding
> 
> https://lkml.kernel.org/r/201905040509.iqQ2CrOU%25lkp@intel.com
> 

Still fails for me.

-- 
~Randy

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Josh Poimboeuf]

On Thu, May 09, 2019 at 11:48:43AM -0700, Randy Dunlap wrote:
> On 5/9/19 11:47 AM, Josh Poimboeuf wrote:
> > On Thu, May 09, 2019 at 01:45:31PM -0500, Josh Poimboeuf wrote:
> >> On Thu, May 09, 2019 at 02:29:02PM -0400, Steven Rostedt wrote:
> >>> On Thu, 9 May 2019 09:51:59 -0700
> >>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>>
> >>>> On Thu, May 9, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >>>>>
> >>>>> This patch works. Can I get your Signed-off-by for it?  
> >>>>
> >>>> Yes. Please write some kind of comprehensible commit log for it, but
> >>>
> >>> How's this:
> >>>
> >>> "Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
> >>> macro converts the conditional to an array index.  This can cause GCC
> >>> to create horrible code.  When there are nested ifs, the generated code
> >>> uses register values to encode branching decisions.
> >>>
> >>> Josh Poimboeuf found that replacing the define "if" macro from using
> >>> the condition as an array index and incrementing the branch statics
> >>> with an if statement itself, reduced the asm complexity and shrinks the
> >>> generated code quite a bit.
> >>>
> >>> But this can be simplified even further by replacing the internal if
> >>> statement with a ternary operator.
> >>>
> >>> Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >>> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >>
> >> Actually, my original fix already went in:
> >>
> >>   37686b1353cf ("tracing: Improve "if" macro code generation")
> >>
> >> But it introduced a regression:
> >>
> >>   https://lkml.kernel.org/r/201905040509.iqQ2CrOU%lkp@intel.com
> >>
> >> which Linus' patch fixes for some reason.
> > 
> > /me curses URL encoding
> > 
> > https://lkml.kernel.org/r/201905040509.iqQ2CrOU%25lkp@intel.com
> > 
> 
> Still fails for me.

Sorry, another URL fail.  It wasn't on lkml.  I should actually try
clicking my own links.

https://lists.01.org/pipermail/kbuild-all/2019-May/060554.html

-- 
Josh

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Thu, 9 May 2019 13:45:31 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Actually, my original fix already went in:
> 
>   37686b1353cf ("tracing: Improve "if" macro code generation")
> 
> But it introduced a regression:
> 
>   https://lkml.kernel.org/r/201905040509.iqQ2CrOU%lkp@intel.com
> 
> which Linus' patch fixes for some reason.

Hmm, I'm still working on my pull request for the merge window, and if
this already went in, I could just add this, and let it conflict. I'm
sure Linus will have no problems in fixing up the conflicts.

I should change the subject, as it is the same ;-) Perhaps to:

 tracing: Clean up "if" macro

But it would be good to find out why this fixes the issue you see.
Perhaps its because we remove the internal if statement?

-- Steve

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Steven Rostedt]

On Thu, 9 May 2019 15:06:44 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Hmm, I'm still working on my pull request for the merge window, and if
> this already went in, I could just add this, and let it conflict. I'm
> sure Linus will have no problems in fixing up the conflicts.
> 
> I should change the subject, as it is the same ;-) Perhaps to:
> 
>  tracing: Clean up "if" macro
> 
> But it would be good to find out why this fixes the issue you see.
> Perhaps its because we remove the internal if statement?

I'm adding this to my tree, if that's alright with everyone. It will
conflict with your patch, but like I said, Linus should have no problem
fixing up the conflicts.

But it probably would probably still be good to know why this fixes the
issues you see.

-- Steve

From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH] tracing: Simplify "if" macro code

Peter Zijlstra noticed that with CONFIG_PROFILE_ALL_BRANCHES, the "if"
macro converts the conditional to an array index.  This can cause GCC
to create horrible code.  When there are nested ifs, the generated code
uses register values to encode branching decisions.

Josh Poimboeuf found that replacing the define "if" macro from using
the condition as an array index and incrementing the branch statics
with an if statement itself, reduced the asm complexity and shrinks the
generated code quite a bit.

But this can be simplified even further by replacing the internal if
statement with a ternary operator.

Link: https://lkml.kernel.org/r/20190307174802.46fmpysxyo35hh43@treble
Link: http://lkml.kernel.org/r/CAHk-=wiALN3jRuzARpwThN62iKd476Xj-uom+YnLZ4=eqcz7xQ@mail.gmail.com

Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/compiler.h | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 445348facea9..8aaf7cd026b0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -53,23 +53,24 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
  * "Define 'is'", Bill Clinton
  * "Define 'if'", Steven Rostedt
  */
-#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
-#define __trace_if(cond) \
-	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
-	({								\
-		int ______r;						\
-		static struct ftrace_branch_data			\
-			__aligned(4)					\
-			__section("_ftrace_branch")			\
-			______f = {					\
-				.func = __func__,			\
-				.file = __FILE__,			\
-				.line = __LINE__,			\
-			};						\
-		______r = !!(cond);					\
-		______f.miss_hit[______r]++;					\
-		______r;						\
-	}))
+#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
+
+#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
+
+#define __trace_if_value(cond) ({			\
+	static struct ftrace_branch_data		\
+		__aligned(4)				\
+		__section("_ftrace_branch")		\
+		__if_trace = {				\
+			.func = __func__,		\
+			.file = __FILE__,		\
+			.line = __LINE__,		\
+		};					\
+	(cond) ?					\
+		(__if_trace.miss_hit[1]++,1) :		\
+		(__if_trace.miss_hit[0]++,0);		\
+})
+
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */
 
 #else
-- 
2.20.1

Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Linus Torvalds]

On Thu, May 9, 2019 at 12:28 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> But it probably would probably still be good to know why this fixes the
> issues you see.

Looks like a compiler bug, plain and simple.

The simplified case really simplifies a lot for the internal IR, and
has a single conditional that then increments a constant location and
returns a constant value. That means that any branch following code in
the compiler has a much simpler setup, and doesn't need to try to
follow any chain of condirional branches with the same conditional to
generate sane code.

But the code generation problem is clearly a compiler bug, although
I'd not be surprised if it's also triggered by whatever horrid things
ASAN does internally to gcc state.

                 Linus

[PATCH 03/25] x86/ia32: Fix ia32_restore_sigcontext AC leak

[Peter Zijlstra]

Objtool spotted that we call native_load_gs_index() with AC set.
Re-arrange the code to avoid that.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 321fe5f5d0e9..4d5fcd47ab75 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -61,9 +61,8 @@
 } while (0)
 
 #define RELOAD_SEG(seg)		{		\
-	unsigned int pre = GET_SEG(seg);	\
+	unsigned int pre = (seg) | 3;		\
 	unsigned int cur = get_user_seg(seg);	\
-	pre |= 3;				\
 	if (pre != cur)				\
 		set_user_seg(seg, pre);		\
 }
@@ -72,6 +71,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 				   struct sigcontext_32 __user *sc)
 {
 	unsigned int tmpflags, err = 0;
+	u16 gs, fs, es, ds;
 	void __user *buf;
 	u32 tmp;
 
@@ -79,16 +79,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 	current->restart_block.fn = do_no_restart_syscall;
 
 	get_user_try {
-		/*
-		 * Reload fs and gs if they have changed in the signal
-		 * handler.  This does not handle long fs/gs base changes in
-		 * the handler, but does not clobber them at least in the
-		 * normal case.
-		 */
-		RELOAD_SEG(gs);
-		RELOAD_SEG(fs);
-		RELOAD_SEG(ds);
-		RELOAD_SEG(es);
+		gs = GET_SEG(gs);
+		fs = GET_SEG(fs);
+		ds = GET_SEG(ds);
+		es = GET_SEG(es);
 
 		COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
 		COPY(dx); COPY(cx); COPY(ip); COPY(ax);
@@ -106,6 +100,17 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		buf = compat_ptr(tmp);
 	} get_user_catch(err);
 
+	/*
+	 * Reload fs and gs if they have changed in the signal
+	 * handler.  This does not handle long fs/gs base changes in
+	 * the handler, but does not clobber them at least in the
+	 * normal case.
+	 */
+	RELOAD_SEG(gs);
+	RELOAD_SEG(fs);
+	RELOAD_SEG(ds);
+	RELOAD_SEG(es);
+
 	err |= fpu__restore_sig(buf, 1);
 
 	force_iret();

[PATCH 04/25] i915,uaccess: Fix redundant CLAC

[Peter Zijlstra]

drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable

AKA. you don't need user_access_end() if user_access_begin() fails.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1606,6 +1606,7 @@ static int eb_copy_relocations(const str
 					     len)) {
 end_user:
 				user_access_end();
+end:
 				kvfree(relocs);
 				err = -EFAULT;
 				goto err;
@@ -1625,7 +1626,7 @@ static int eb_copy_relocations(const str
 		 * relocations were valid.
 		 */
 		if (!user_access_begin(urelocs, size))
-			goto end_user;
+			goto end;
 
 		for (copied = 0; copied < nreloc; copied++)
 			unsafe_put_user(-1,
@@ -2616,7 +2617,7 @@ i915_gem_execbuffer2_ioctl(struct drm_de
 		 * when we did the "copy_from_user()" above.
 		 */
 		if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list)))
-			goto end_user;
+			goto end;
 
 		for (i = 0; i < args->buffer_count; i++) {
 			if (!(exec2_list[i].offset & UPDATE))
@@ -2630,6 +2631,7 @@ i915_gem_execbuffer2_ioctl(struct drm_de
 		}
 end_user:
 		user_access_end();
+end:;
 	}
 
 	args->flags &= ~__I915_EXEC_UNKNOWN_FLAGS;

[PATCH 05/25] x86/uaccess: Move copy_user_handle_tail into asm

[Peter Zijlstra]

By writing the function in asm we avoid cross object code flow and
objtool no longer gets confused about a 'stray' CLAC.

Also; the asm version is actually _simpler_.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/asm.h        |   24 -------------------
 arch/x86/include/asm/uaccess_64.h |    3 --
 arch/x86/lib/copy_user_64.S       |   48 ++++++++++++++++++++++++++++++++++++++
 arch/x86/lib/usercopy_64.c        |   20 ---------------
 4 files changed, 48 insertions(+), 47 deletions(-)

--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -148,30 +148,6 @@
 	_ASM_PTR (entry);					\
 	.popsection
 
-.macro ALIGN_DESTINATION
-	/* check for bad alignment of destination */
-	movl %edi,%ecx
-	andl $7,%ecx
-	jz 102f				/* already aligned */
-	subl $8,%ecx
-	negl %ecx
-	subl %ecx,%edx
-100:	movb (%rsi),%al
-101:	movb %al,(%rdi)
-	incq %rsi
-	incq %rdi
-	decl %ecx
-	jnz 100b
-102:
-	.section .fixup,"ax"
-103:	addl %ecx,%edx			/* ecx is zerorest also */
-	jmp copy_user_handle_tail
-	.previous
-
-	_ASM_EXTABLE_UA(100b, 103b)
-	_ASM_EXTABLE_UA(101b, 103b)
-	.endm
-
 #else
 # define _EXPAND_EXTABLE_HANDLE(x) #x
 # define _ASM_EXTABLE_HANDLE(from, to, handler)			\
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -208,9 +208,6 @@ __copy_from_user_flushcache(void *dst, c
 }
 
 unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len);
-
-unsigned long
 mcsafe_handle_tail(char *to, char *from, unsigned len);
 
 #endif /* _ASM_X86_UACCESS_64_H */
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -16,6 +16,30 @@
 #include <asm/smap.h>
 #include <asm/export.h>
 
+.macro ALIGN_DESTINATION
+	/* check for bad alignment of destination */
+	movl %edi,%ecx
+	andl $7,%ecx
+	jz 102f				/* already aligned */
+	subl $8,%ecx
+	negl %ecx
+	subl %ecx,%edx
+100:	movb (%rsi),%al
+101:	movb %al,(%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz 100b
+102:
+	.section .fixup,"ax"
+103:	addl %ecx,%edx			/* ecx is zerorest also */
+	jmp copy_user_handle_tail
+	.previous
+
+	_ASM_EXTABLE_UA(100b, 103b)
+	_ASM_EXTABLE_UA(101b, 103b)
+	.endm
+
 /*
  * copy_user_generic_unrolled - memory copy with exception handling.
  * This version is for CPUs like P4 that don't have efficient micro
@@ -194,6 +218,30 @@ ENDPROC(copy_user_enhanced_fast_string)
 EXPORT_SYMBOL(copy_user_enhanced_fast_string)
 
 /*
+ * Try to copy last bytes and clear the rest if needed.
+ * Since protection fault in copy_from/to_user is not a normal situation,
+ * it is not necessary to optimize tail handling.
+ *
+ * Input:
+ * rdi destination
+ * rsi source
+ * rdx count
+ *
+ * Output:
+ * eax uncopied bytes or 0 if successful.
+ */
+ALIGN;
+copy_user_handle_tail:
+	movl %edx,%ecx
+1:	rep movsb
+2:	mov %ecx,%eax
+	ASM_CLAC
+	ret
+
+	_ASM_EXTABLE_UA(1b, 2b)
+ENDPROC(copy_user_handle_tail)
+
+/*
  * copy_user_nocache - Uncached memory copy with exception handling
  * This will force destination out of cache for more performance.
  *
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -55,26 +55,6 @@ unsigned long clear_user(void __user *to
 EXPORT_SYMBOL(clear_user);
 
 /*
- * Try to copy last bytes and clear the rest if needed.
- * Since protection fault in copy_from/to_user is not a normal situation,
- * it is not necessary to optimize tail handling.
- */
-__visible unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len)
-{
-	for (; len; --len, to++) {
-		char c;
-
-		if (__get_user_nocheck(c, from++, sizeof(char)))
-			break;
-		if (__put_user_nocheck(c, to, sizeof(char)))
-			break;
-	}
-	clac();
-	return len;
-}
-
-/*
  * Similar to copy_user_handle_tail, probe for the write fault point,
  * but reuse __memcpy_mcsafe in case a new read error is encountered.
  * clac() is handled in _copy_to_iter_mcsafe().

[PATCH 06/25] x86/uaccess: Fix up the fixup

[Peter Zijlstra]

arch/x86/lib/memcpy_64.o: warning: objtool: .fixup+0x7: return with UACCESS enabled

While the code isn't wrong; it is tedious (if at all possible) to
figure out what function a particular chunk of .fixup belongs to.

This then confuses the objtool uaccess validation. Instead of
returning directly from the .fixup, jump back into the right function.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/memcpy_64.S |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -257,6 +257,7 @@ ENTRY(__memcpy_mcsafe)
 	/* Copy successful. Return zero */
 .L_done_memcpy_trap:
 	xorl %eax, %eax
+.L_done:
 	ret
 ENDPROC(__memcpy_mcsafe)
 EXPORT_SYMBOL_GPL(__memcpy_mcsafe)
@@ -273,7 +274,7 @@ EXPORT_SYMBOL_GPL(__memcpy_mcsafe)
 	addl	%edx, %ecx
 .E_trailing_bytes:
 	mov	%ecx, %eax
-	ret
+	jmp	.L_done
 
 	/*
 	 * For write fault handling, given the destination is unaligned,

[PATCH 07/25] x86/nospec,objtool: Introduce ANNOTATE_IGNORE_ALTERNATIVE

[Peter Zijlstra]

To facillitate other usage of ignoring alternatives; rename
ANNOTATE_NOSPEC_IGNORE to ANNOTATE_IGNORE_ALTERNATIVE.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/alternative-asm.h |   11 +++++++++++
 arch/x86/include/asm/alternative.h     |   10 ++++++++++
 arch/x86/include/asm/nospec-branch.h   |   28 +++++++++-------------------
 tools/objtool/check.c                  |    8 ++++----
 4 files changed, 34 insertions(+), 23 deletions(-)

--- a/arch/x86/include/asm/alternative-asm.h
+++ b/arch/x86/include/asm/alternative-asm.h
@@ -20,6 +20,17 @@
 #endif
 
 /*
+ * objtool annotation to ignore the alternatives and only consider the original
+ * instruction(s).
+ */
+.macro ANNOTATE_IGNORE_ALTERNATIVE
+	.Lannotate_\@:
+	.pushsection .discard.ignore_alts
+	.long .Lannotate_\@ - .
+	.popsection
+.endm
+
+/*
  * Issue one struct alt_instr descriptor entry (need to put it into
  * the section .altinstructions, see below). This entry contains
  * enough information for the alternatives patching code to patch an
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -45,6 +45,16 @@
 #define LOCK_PREFIX ""
 #endif
 
+/*
+ * objtool annotation to ignore the alternatives and only consider the original
+ * instruction(s).
+ */
+#define ANNOTATE_IGNORE_ALTERNATIVE				\
+	"999:\n\t"						\
+	".pushsection .discard.ignore_alts\n\t"			\
+	".long 999b - .\n\t"					\
+	".popsection\n\t"
+
 struct alt_instr {
 	s32 instr_offset;	/* original instruction */
 	s32 repl_offset;	/* offset to replacement instruction */
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -11,6 +11,15 @@
 #include <asm/msr-index.h>
 
 /*
+ * This should be used immediately before a retpoline alternative. It tells
+ * objtool where the retpolines are so that it can make sense of the control
+ * flow by just reading the original instruction(s) and ignoring the
+ * alternatives.
+ */
+#define ANNOTATE_NOSPEC_ALTERNATIVE \
+	ANNOTATE_IGNORE_ALTERNATIVE
+
+/*
  * Fill the CPU return stack buffer.
  *
  * Each entry in the RSB, if used for a speculative 'ret', contains an
@@ -57,19 +66,6 @@
 #ifdef __ASSEMBLY__
 
 /*
- * This should be used immediately before a retpoline alternative.  It tells
- * objtool where the retpolines are so that it can make sense of the control
- * flow by just reading the original instruction(s) and ignoring the
- * alternatives.
- */
-.macro ANNOTATE_NOSPEC_ALTERNATIVE
-	.Lannotate_\@:
-	.pushsection .discard.nospec
-	.long .Lannotate_\@ - .
-	.popsection
-.endm
-
-/*
  * This should be used immediately before an indirect jump/call. It tells
  * objtool the subsequent indirect jump/call is vouched safe for retpoline
  * builds.
@@ -152,12 +148,6 @@
 
 #else /* __ASSEMBLY__ */
 
-#define ANNOTATE_NOSPEC_ALTERNATIVE				\
-	"999:\n\t"						\
-	".pushsection .discard.nospec\n\t"			\
-	".long 999b - .\n\t"					\
-	".popsection\n\t"
-
 #define ANNOTATE_RETPOLINE_SAFE					\
 	"999:\n\t"						\
 	".pushsection .discard.retpoline_safe\n\t"		\
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -457,13 +457,13 @@ static void add_ignores(struct objtool_f
  * But it at least allows objtool to understand the control flow *around* the
  * retpoline.
  */
-static int add_nospec_ignores(struct objtool_file *file)
+static int add_ignore_alternatives(struct objtool_file *file)
 {
 	struct section *sec;
 	struct rela *rela;
 	struct instruction *insn;
 
-	sec = find_section_by_name(file->elf, ".rela.discard.nospec");
+	sec = find_section_by_name(file->elf, ".rela.discard.ignore_alts");
 	if (!sec)
 		return 0;
 
@@ -475,7 +475,7 @@ static int add_nospec_ignores(struct obj
 
 		insn = find_insn(file, rela->sym->sec, rela->addend);
 		if (!insn) {
-			WARN("bad .discard.nospec entry");
+			WARN("bad .discard.ignore_alts entry");
 			return -1;
 		}
 
@@ -1239,7 +1239,7 @@ static int decode_sections(struct objtoo
 
 	add_ignores(file);
 
-	ret = add_nospec_ignores(file);
+	ret = add_ignore_alternatives(file);
 	if (ret)
 		return ret;
 

[PATCH 08/25] x86/uaccess,xen: Suppress SMAP warnings

[Peter Zijlstra]

drivers/xen/privcmd.o: warning: objtool: privcmd_ioctl()+0x1414: call to hypercall_page() with UACCESS enabled

Some Xen hypercalls allow parameter buffers in user land, so they need
to set AC=1. Avoid the warning for those cases.

Cc: andrew.cooper3@citrix.com
Reviewed-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/xen/hypercall.h |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -214,6 +214,22 @@ xen_single_call(unsigned int call,
 	return (long)__res;
 }
 
+static __always_inline void __xen_stac(void)
+{
+	/*
+	 * Suppress objtool seeing the STAC/CLAC and getting confused about it
+	 * calling random code with AC=1.
+	 */
+	asm volatile(ANNOTATE_IGNORE_ALTERNATIVE
+		     ASM_STAC ::: "memory", "flags");
+}
+
+static __always_inline void __xen_clac(void)
+{
+	asm volatile(ANNOTATE_IGNORE_ALTERNATIVE
+		     ASM_CLAC ::: "memory", "flags");
+}
+
 static inline long
 privcmd_call(unsigned int call,
 	     unsigned long a1, unsigned long a2,
@@ -222,9 +238,9 @@ privcmd_call(unsigned int call,
 {
 	long res;
 
-	stac();
+	__xen_stac();
 	res = xen_single_call(call, a1, a2, a3, a4, a5);
-	clac();
+	__xen_clac();
 
 	return res;
 }
@@ -421,9 +437,9 @@ HYPERVISOR_dm_op(
 	domid_t dom, unsigned int nr_bufs, struct xen_dm_op_buf *bufs)
 {
 	int ret;
-	stac();
+	__xen_stac();
 	ret = _hypercall3(int, dm_op, dom, nr_bufs, bufs);
-	clac();
+	__xen_clac();
 	return ret;
 }
 

[PATCH 09/25] x86/uaccess: Always inline user_access_begin()

[Peter Zijlstra]

If GCC out-of-lines it, the STAC and CLAC are in different fuctions
and objtool gets upset.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/uaccess.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -706,7 +706,7 @@ extern struct movsl_mask {
  * checking before using them, but you have to surround them with the
  * user_access_begin/end() pair.
  */
-static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
+static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
 {
 	if (unlikely(!access_ok(ptr,len)))
 		return 0;

[PATCH 10/25] x86/uaccess,signal: Fix AC=1 bloat

[Peter Zijlstra]

Occasionally GCC is less agressive with inlining and the following is
observed:

arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x3cc: call to force_valid_ss.isra.5() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x384: call to frame_uc_flags.isra.0() with UACCESS enabled

Cure this by moving this code out of the AC=1 region, since it really
isn't needed for the user access.

Reviewed-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/signal.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -132,16 +132,6 @@ static int restore_sigcontext(struct pt_
 		COPY_SEG_CPL3(cs);
 		COPY_SEG_CPL3(ss);
 
-#ifdef CONFIG_X86_64
-		/*
-		 * Fix up SS if needed for the benefit of old DOSEMU and
-		 * CRIU.
-		 */
-		if (unlikely(!(uc_flags & UC_STRICT_RESTORE_SS) &&
-			     user_64bit_mode(regs)))
-			force_valid_ss(regs);
-#endif
-
 		get_user_ex(tmpflags, &sc->flags);
 		regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
 		regs->orig_ax = -1;		/* disable syscall checks */
@@ -150,6 +140,15 @@ static int restore_sigcontext(struct pt_
 		buf = (void __user *)buf_val;
 	} get_user_catch(err);
 
+#ifdef CONFIG_X86_64
+	/*
+	 * Fix up SS if needed for the benefit of old DOSEMU and
+	 * CRIU.
+	 */
+	if (unlikely(!(uc_flags & UC_STRICT_RESTORE_SS) && user_64bit_mode(regs)))
+		force_valid_ss(regs);
+#endif
+
 	err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32));
 
 	force_iret();
@@ -461,6 +460,7 @@ static int __setup_rt_frame(int sig, str
 {
 	struct rt_sigframe __user *frame;
 	void __user *fp = NULL;
+	unsigned long uc_flags;
 	int err = 0;
 
 	frame = get_sigframe(&ksig->ka, regs, sizeof(struct rt_sigframe), &fp);
@@ -473,9 +473,11 @@ static int __setup_rt_frame(int sig, str
 			return -EFAULT;
 	}
 
+	uc_flags = frame_uc_flags(regs);
+
 	put_user_try {
 		/* Create the ucontext.  */
-		put_user_ex(frame_uc_flags(regs), &frame->uc.uc_flags);
+		put_user_ex(uc_flags, &frame->uc.uc_flags);
 		put_user_ex(0, &frame->uc.uc_link);
 		save_altstack_ex(&frame->uc.uc_stack, regs->sp);
 
@@ -541,6 +543,7 @@ static int x32_setup_rt_frame(struct ksi
 {
 #ifdef CONFIG_X86_X32_ABI
 	struct rt_sigframe_x32 __user *frame;
+	unsigned long uc_flags;
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
@@ -555,9 +558,11 @@ static int x32_setup_rt_frame(struct ksi
 			return -EFAULT;
 	}
 
+	uc_flags = frame_uc_flags(regs);
+
 	put_user_try {
 		/* Create the ucontext.  */
-		put_user_ex(frame_uc_flags(regs), &frame->uc.uc_flags);
+		put_user_ex(uc_flags, &frame->uc.uc_flags);
 		put_user_ex(0, &frame->uc.uc_link);
 		compat_save_altstack_ex(&frame->uc.uc_stack, regs->sp);
 		put_user_ex(0, &frame->uc.uc__pad0);

[PATCH 11/25] x86/uaccess: Introduce user_access_{save,restore}()

[Peter Zijlstra]

Introduce common helpers for when we need to safely suspend a
uaccess section; for instance to generate a {KA,UB}SAN report.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/smap.h    |   20 ++++++++++++++++++++
 arch/x86/include/asm/uaccess.h |    3 +++
 include/linux/uaccess.h        |    2 ++
 3 files changed, 25 insertions(+)

--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -58,6 +58,23 @@ static __always_inline void stac(void)
 	alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
 }
 
+static __always_inline unsigned long smap_save(void)
+{
+	unsigned long flags;
+
+	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __stringify(__ASM_CLAC),
+				  X86_FEATURE_SMAP)
+		      : "=rm" (flags) : : "memory", "cc");
+
+	return flags;
+}
+
+static __always_inline void smap_restore(unsigned long flags)
+{
+	asm volatile (ALTERNATIVE("", "push %0; popf", X86_FEATURE_SMAP)
+		      : : "g" (flags) : "memory", "cc");
+}
+
 /* These macros can be used in asm() statements */
 #define ASM_CLAC \
 	ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
@@ -69,6 +86,9 @@ static __always_inline void stac(void)
 static inline void clac(void) { }
 static inline void stac(void) { }
 
+static inline unsigned long smap_save(void) { return 0; }
+static inline void smap_restore(unsigned long flags) { }
+
 #define ASM_CLAC
 #define ASM_STAC
 
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -716,6 +716,9 @@ static __must_check __always_inline bool
 #define user_access_begin(a,b)	user_access_begin(a,b)
 #define user_access_end()	__uaccess_end()
 
+#define user_access_save()	smap_save()
+#define user_access_restore(x)	smap_restore(x)
+
 #define unsafe_put_user(x, ptr, label)	\
 	__put_user_size((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), label)
 
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -268,6 +268,8 @@ extern long strncpy_from_unsafe(char *ds
 #define user_access_end() do { } while (0)
 #define unsafe_get_user(x, ptr, err) do { if (unlikely(__get_user(x, ptr))) goto err; } while (0)
 #define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0)
+static inline unsigned long user_access_save(void) { return 0UL; }
+static void user_access_restore(unsigned long flags) { }
 #endif
 
 #ifdef CONFIG_HARDENED_USERCOPY

[PATCH 12/25] x86/smap: Ditch __stringify()

[Peter Zijlstra]

Linus noticed all users of __ASM_STAC/__ASM_CLAC are with
__stringify(). Just make them a string.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/smap.h |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -13,13 +13,12 @@
 #ifndef _ASM_X86_SMAP_H
 #define _ASM_X86_SMAP_H
 
-#include <linux/stringify.h>
 #include <asm/nops.h>
 #include <asm/cpufeatures.h>
 
 /* "Raw" instruction opcodes */
-#define __ASM_CLAC	.byte 0x0f,0x01,0xca
-#define __ASM_STAC	.byte 0x0f,0x01,0xcb
+#define __ASM_CLAC	".byte 0x0f,0x01,0xca"
+#define __ASM_STAC	".byte 0x0f,0x01,0xcb"
 
 #ifdef __ASSEMBLY__
 
@@ -28,10 +27,10 @@
 #ifdef CONFIG_X86_SMAP
 
 #define ASM_CLAC \
-	ALTERNATIVE "", __stringify(__ASM_CLAC), X86_FEATURE_SMAP
+	ALTERNATIVE "", __ASM_CLAC, X86_FEATURE_SMAP
 
 #define ASM_STAC \
-	ALTERNATIVE "", __stringify(__ASM_STAC), X86_FEATURE_SMAP
+	ALTERNATIVE "", __ASM_STAC, X86_FEATURE_SMAP
 
 #else /* CONFIG_X86_SMAP */
 
@@ -49,20 +48,20 @@
 static __always_inline void clac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
-	alternative("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP);
+	alternative("", __ASM_CLAC, X86_FEATURE_SMAP);
 }
 
 static __always_inline void stac(void)
 {
 	/* Note: a barrier is implicit in alternative() */
-	alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
+	alternative("", __ASM_STAC, X86_FEATURE_SMAP);
 }
 
 static __always_inline unsigned long smap_save(void)
 {
 	unsigned long flags;
 
-	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __stringify(__ASM_CLAC),
+	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __ASM_CLAC,
 				  X86_FEATURE_SMAP)
 		      : "=rm" (flags) : : "memory", "cc");
 
@@ -77,9 +76,9 @@ static __always_inline void smap_restore
 
 /* These macros can be used in asm() statements */
 #define ASM_CLAC \
-	ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
+	ALTERNATIVE("", __ASM_CLAC, X86_FEATURE_SMAP)
 #define ASM_STAC \
-	ALTERNATIVE("", __stringify(__ASM_STAC), X86_FEATURE_SMAP)
+	ALTERNATIVE("", __ASM_STAC, X86_FEATURE_SMAP)
 
 #else /* CONFIG_X86_SMAP */
 

[PATCH 13/25] x86/uaccess,kasan: Fix KASAN vs SMAP

[Peter Zijlstra]

KASAN inserts extra code for every LOAD/STORE emitted by te compiler.
Much of this code is simple and safe to run with AC=1, however the
kasan_report() function, called on error, is most certainly not safe
to call with AC=1.

Therefore wrap kasan_report() in user_access_{save,restore}; which for
x86 SMAP, saves/restores EFLAGS and clears AC before calling the real
function.

Also ensure all the functions are without __fentry__ hook. The
function tracer is also not safe.

Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 mm/kasan/Makefile |    3 +++
 mm/kasan/common.c |   10 ++++++++++
 mm/kasan/report.c |    3 +--
 3 files changed, 14 insertions(+), 2 deletions(-)

--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -2,11 +2,13 @@
 KASAN_SANITIZE := n
 UBSAN_SANITIZE_common.o := n
 UBSAN_SANITIZE_generic.o := n
+UBSAN_SANITIZE_generic_report.o := n
 UBSAN_SANITIZE_tags.o := n
 KCOV_INSTRUMENT := n
 
 CFLAGS_REMOVE_common.o = -pg
 CFLAGS_REMOVE_generic.o = -pg
+CFLAGS_REMOVE_generic_report.o = -pg
 CFLAGS_REMOVE_tags.o = -pg
 
 # Function splitter causes unnecessary splits in __asan_load1/__asan_store1
@@ -14,6 +16,7 @@ CFLAGS_REMOVE_tags.o = -pg
 
 CFLAGS_common.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 CFLAGS_generic.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
+CFLAGS_generic_report.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 
 obj-$(CONFIG_KASAN) := common.o init.o report.o
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -34,6 +34,7 @@
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 
 #include "kasan.h"
 #include "../slab.h"
@@ -612,6 +613,15 @@ void kasan_free_shadow(const struct vm_s
 		vfree(kasan_mem_to_shadow(vm->addr));
 }
 
+extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
+
+void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
+{
+	unsigned long flags = user_access_save();
+	__kasan_report(addr, size, is_write, ip);
+	user_access_restore(flags);
+}
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 static bool shadow_mapped(unsigned long addr)
 {
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,8 +281,7 @@ void kasan_report_invalid_free(void *obj
 	end_report(&flags);
 }
 
-void kasan_report(unsigned long addr, size_t size,
-		bool is_write, unsigned long ip)
+void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
 {
 	struct kasan_access_info info;
 	void *tagged_addr;

[PATCH 14/25] x86/uaccess,ubsan: Fix UBSAN vs SMAP

[Peter Zijlstra]

UBSAN can insert extra code in random locations; including AC=1
sections. Typically this code is not safe and needs wrapping.

So far, only __ubsan_handle_type_mismatch* have been observed in AC=1
sections and therefore only those are annotated.

Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 lib/Makefile |    1 +
 lib/ubsan.c  |    4 ++++
 2 files changed, 5 insertions(+)

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -263,6 +263,7 @@ obj-$(CONFIG_UCS2_STRING) += ucs2_string
 obj-$(CONFIG_UBSAN) += ubsan.o
 
 UBSAN_SANITIZE_ubsan.o := n
+CFLAGS_ubsan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 
 obj-$(CONFIG_SBITMAP) += sbitmap.o
 
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/sched.h>
+#include <linux/uaccess.h>
 
 #include "ubsan.h"
 
@@ -313,6 +314,7 @@ static void handle_object_size_mismatch(
 static void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
 				unsigned long ptr)
 {
+	unsigned long flags = user_access_save();
 
 	if (!ptr)
 		handle_null_ptr_deref(data);
@@ -320,6 +322,8 @@ static void ubsan_type_mismatch_common(s
 		handle_misaligned_access(data, ptr);
 	else
 		handle_object_size_mismatch(data, ptr);
+
+	user_access_restore(flags);
 }
 
 void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,

[PATCH 15/25] x86/uaccess,ftrace: Fix ftrace_likely_update() vs SMAP

[Peter Zijlstra]

For CONFIG_TRACE_BRANCH_PROFILING the likely/unlikely things get
overloaded and generate callouts to this code, and thus also when
AC=1.

Make it safe.

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/trace/trace_branch.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/kernel/trace/trace_branch.c
+++ b/kernel/trace/trace_branch.c
@@ -205,6 +205,8 @@ void trace_likely_condition(struct ftrac
 void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 			  int expect, int is_constant)
 {
+	unsigned long flags = user_access_save();
+
 	/* A constant is always correct */
 	if (is_constant) {
 		f->constant++;
@@ -223,6 +225,8 @@ void ftrace_likely_update(struct ftrace_
 		f->data.correct++;
 	else
 		f->data.incorrect++;
+
+	user_access_restore(flags);
 }
 EXPORT_SYMBOL(ftrace_likely_update);
 

[PATCH 16/25] x86/uaccess,kcov: Disable stack protector

[Peter Zijlstra]

kernel/kcov.o: warning: objtool: write_comp_data()+0x138: call to __stack_chk_fail() with UACCESS enabled
kernel/kcov.o: warning: objtool: __sanitizer_cov_trace_pc()+0xd9: call to __stack_chk_fail() with UACCESS enabled

All the other instrumentation (KASAN,UBSAN) also have stack protector
disabled.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/Makefile |    1 +
 1 file changed, 1 insertion(+)

--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -30,6 +30,7 @@ KCOV_INSTRUMENT_extable.o := n
 # Don't self-instrument.
 KCOV_INSTRUMENT_kcov.o := n
 KASAN_SANITIZE_kcov.o := n
+CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 
 # cond_syscall is currently not LTO compatible
 CFLAGS_sys_ni.o = $(DISABLE_LTO)

[PATCH 17/25] objtool: Set insn->func for alternatives

[Peter Zijlstra]

In preparation of function attributes, we need each instruction to
have a valid link back to its function.

Therefore make sure we set the function association for alternative
instruction sequences; they are, after all, still part of the function.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |    1 +
 1 file changed, 1 insertion(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -695,6 +695,7 @@ static int handle_group_alt(struct objto
 		last_new_insn = insn;
 
 		insn->ignore = orig_insn->ignore_alts;
+		insn->func = orig_insn->func;
 
 		if (insn->type != INSN_JUMP_CONDITIONAL &&
 		    insn->type != INSN_JUMP_UNCONDITIONAL)

[PATCH 18/25] objtool: Handle function aliases

[Peter Zijlstra]

Function aliases result in different symbols for the same set of
instructions; track a canonical symbol so there is a unique point of
access.

This again prepares the way for function attributes. And in particular
the need for aliases comes from how KASAN uses them.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/elf.c |   15 +++++++++++----
 tools/objtool/elf.h |    2 +-
 2 files changed, 12 insertions(+), 5 deletions(-)

--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -219,7 +219,7 @@ static int read_sections(struct elf *elf
 static int read_symbols(struct elf *elf)
 {
 	struct section *symtab, *sec;
-	struct symbol *sym, *pfunc;
+	struct symbol *sym, *pfunc, *alias;
 	struct list_head *entry, *tmp;
 	int symbols_nr, i;
 	char *coldstr;
@@ -239,6 +239,7 @@ static int read_symbols(struct elf *elf)
 			return -1;
 		}
 		memset(sym, 0, sizeof(*sym));
+		alias = sym;
 
 		sym->idx = i;
 
@@ -288,11 +289,17 @@ static int read_symbols(struct elf *elf)
 				break;
 			}
 
-			if (sym->offset == s->offset && sym->len >= s->len) {
-				entry = tmp;
-				break;
+			if (sym->offset == s->offset) {
+				if (sym->len == s->len && alias == sym)
+					alias = s;
+
+				if (sym->len >= s->len) {
+					entry = tmp;
+					break;
+				}
 			}
 		}
+		sym->alias = alias;
 		list_add(&sym->list, entry);
 		hash_add(sym->sec->symbol_hash, &sym->hash, sym->idx);
 	}
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -61,7 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
-	struct symbol *pfunc, *cfunc;
+	struct symbol *pfunc, *cfunc, *alias;
 };
 
 struct rela {

[PATCH 19/25] objtool: Rewrite add_ignores()

[Peter Zijlstra]

The whole add_ignores() thing was wildly weird; rewrite it according
to 'modern' ways.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   51 +++++++++++++++++++-------------------------------
 tools/objtool/check.h |    1 
 2 files changed, 20 insertions(+), 32 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -105,29 +105,6 @@ static struct instruction *next_insn_sam
 	     insn = next_insn_same_sec(file, insn))
 
 /*
- * Check if the function has been manually whitelisted with the
- * STACK_FRAME_NON_STANDARD macro, or if it should be automatically whitelisted
- * due to its use of a context switching instruction.
- */
-static bool ignore_func(struct objtool_file *file, struct symbol *func)
-{
-	struct rela *rela;
-
-	/* check for STACK_FRAME_NON_STANDARD */
-	if (file->whitelist && file->whitelist->rela)
-		list_for_each_entry(rela, &file->whitelist->rela->rela_list, list) {
-			if (rela->sym->type == STT_SECTION &&
-			    rela->sym->sec == func->sec &&
-			    rela->addend == func->offset)
-				return true;
-			if (rela->sym->type == STT_FUNC && rela->sym == func)
-				return true;
-		}
-
-	return false;
-}
-
-/*
  * This checks to see if the given function is a "noreturn" function.
  *
  * For global functions which are outside the scope of this object file, we
@@ -436,18 +413,31 @@ static void add_ignores(struct objtool_f
 	struct instruction *insn;
 	struct section *sec;
 	struct symbol *func;
+	struct rela *rela;
 
-	for_each_sec(file, sec) {
-		list_for_each_entry(func, &sec->symbol_list, list) {
-			if (func->type != STT_FUNC)
-				continue;
+	sec = find_section_by_name(file->elf, ".rela.discard.func_stack_frame_non_standard");
+	if (!sec)
+		return;
 
-			if (!ignore_func(file, func))
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		switch (rela->sym->type) {
+		case STT_FUNC:
+			func = rela->sym;
+			break;
+
+		case STT_SECTION:
+			func = find_symbol_by_offset(rela->sym->sec, rela->addend);
+			if (!func || func->type != STT_FUNC)
 				continue;
+			break;
 
-			func_for_each_insn_all(file, func, insn)
-				insn->ignore = true;
+		default:
+			WARN("unexpected relocation symbol type in %s: %d", sec->name, rela->sym->type);
+			continue;
 		}
+
+		func_for_each_insn_all(file, func, insn)
+			insn->ignore = true;
 	}
 }
 
@@ -2198,7 +2188,6 @@ int check(const char *_objname, bool orc
 
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
-	file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard");
 	file.c_file = find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -60,7 +60,6 @@ struct objtool_file {
 	struct elf *elf;
 	struct list_head insn_list;
 	DECLARE_HASHTABLE(insn_hash, 16);
-	struct section *whitelist;
 	bool ignore_unreachables, c_file, hints, rodata;
 };
 

[PATCH 20/25] objtool: Add --backtrace support

[Peter Zijlstra]

For when you want to know the path that reached your fail state.

$ ./objtool check --no-fp --backtrace arch/x86/lib/usercopy_64.o
arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x3: UACCESS disable without MEMOPs: __clear_user()
arch/x86/lib/usercopy_64.o: warning: objtool:   __clear_user()+0x3a: (alt)
arch/x86/lib/usercopy_64.o: warning: objtool:   __clear_user()+0x2e: (branch)
arch/x86/lib/usercopy_64.o: warning: objtool:   __clear_user()+0x18: (branch)
arch/x86/lib/usercopy_64.o: warning: objtool:   .altinstr_replacement+0xffffffffffffffff: (branch)
arch/x86/lib/usercopy_64.o: warning: objtool:   __clear_user()+0x5: (alt)
arch/x86/lib/usercopy_64.o: warning: objtool:   __clear_user()+0x0: <=== (func)

0000000000000000 <__clear_user>:
  0:   e8 00 00 00 00          callq  5 <__clear_user+0x5>
               1: R_X86_64_PLT32       __fentry__-0x4
  5:   90                      nop
  6:   90                      nop
  7:   90                      nop
  8:   48 89 f0                mov    %rsi,%rax
  b:   48 c1 ee 03             shr    $0x3,%rsi
  f:   83 e0 07                and    $0x7,%eax
 12:   48 89 f1                mov    %rsi,%rcx
 15:   48 85 c9                test   %rcx,%rcx
 18:   74 0f                   je     29 <__clear_user+0x29>
 1a:   48 c7 07 00 00 00 00    movq   $0x0,(%rdi)
 21:   48 83 c7 08             add    $0x8,%rdi
 25:   ff c9                   dec    %ecx
 27:   75 f1                   jne    1a <__clear_user+0x1a>
 29:   48 89 c1                mov    %rax,%rcx
 2c:   85 c9                   test   %ecx,%ecx
 2e:   74 0a                   je     3a <__clear_user+0x3a>
 30:   c6 07 00                movb   $0x0,(%rdi)
 33:   48 ff c7                inc    %rdi
 36:   ff c9                   dec    %ecx
 38:   75 f6                   jne    30 <__clear_user+0x30>
 3a:   90                      nop
 3b:   90                      nop
 3c:   90                      nop
 3d:   48 89 c8                mov    %rcx,%rax
 40:   c3                      retq


Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/builtin-check.c |    3 ++-
 tools/objtool/builtin.h       |    2 +-
 tools/objtool/check.c         |   18 ++++++++++++++----
 tools/objtool/warn.h          |    8 ++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable, retpoline, module;
+bool no_fp, no_unreachable, retpoline, module, backtrace;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -41,6 +41,7 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
 	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
 	OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"),
+	OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
 	OPT_END(),
 };
 
--- a/tools/objtool/builtin.h
+++ b/tools/objtool/builtin.h
@@ -20,7 +20,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline, module;
+extern bool no_fp, no_unreachable, retpoline, module, backtrace;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1885,8 +1885,11 @@ static int validate_branch(struct objtoo
 		if (!insn->ignore_alts) {
 			list_for_each_entry(alt, &insn->alts, list) {
 				ret = validate_branch(file, alt->insn, state);
-				if (ret)
-					return 1;
+				if (ret) {
+					if (backtrace)
+						BT_FUNC("(alt)", insn);
+					return ret;
+				}
 			}
 		}
 
@@ -1933,8 +1936,11 @@ static int validate_branch(struct objtoo
 			     insn->jump_dest->func->pfunc == func)) {
 				ret = validate_branch(file, insn->jump_dest,
 						      state);
-				if (ret)
-					return 1;
+				if (ret) {
+					if (backtrace)
+						BT_FUNC("(branch)", insn);
+					return ret;
+				}
 
 			} else if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("sibling call from callable instruction with modified stack frame",
@@ -2005,6 +2011,8 @@ static int validate_unwind_hints(struct
 	for_each_insn(file, insn) {
 		if (insn->hint && !insn->visited) {
 			ret = validate_branch(file, insn, state);
+			if (ret && backtrace)
+				BT_FUNC("<=== (hint)", insn);
 			warnings += ret;
 		}
 	}
@@ -2133,6 +2141,8 @@ static int validate_functions(struct obj
 				continue;
 
 			ret = validate_branch(file, insn, state);
+			if (ret && backtrace)
+				BT_FUNC("<=== (func)", insn);
 			warnings += ret;
 		}
 	}
--- a/tools/objtool/warn.h
+++ b/tools/objtool/warn.h
@@ -64,6 +64,14 @@ static inline char *offstr(struct sectio
 	free(_str);					\
 })
 
+#define BT_FUNC(format, insn, ...)			\
+({							\
+	struct instruction *_insn = (insn);		\
+	char *_str = offstr(_insn->sec, _insn->offset); \
+	WARN("  %s: " format, _str, ##__VA_ARGS__);	\
+	free(_str);					\
+})
+
 #define WARN_ELF(format, ...)				\
 	WARN(format ": %s", ##__VA_ARGS__, elf_errmsg(-1))
 

[PATCH 21/25] objtool: Rewrite alt->skip_orig

[Peter Zijlstra]

Really skip the original instruction flow, instead of letting it
continue with NOPs.

Since the alternative code flow already continues after the original
instructions, only the alt-original is skipped.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -31,6 +31,7 @@
 struct alternative {
 	struct list_head list;
 	struct instruction *insn;
+	bool skip_orig;
 };
 
 const char *objname;
@@ -623,9 +624,6 @@ static int add_call_destinations(struct
  *    conditionally jumps to the _end_ of the entry.  We have to modify these
  *    jumps' destinations to point back to .text rather than the end of the
  *    entry in .altinstr_replacement.
- *
- * 4. It has been requested that we don't validate the !POPCNT feature path
- *    which is a "very very small percentage of machines".
  */
 static int handle_group_alt(struct objtool_file *file,
 			    struct special_alt *special_alt,
@@ -641,9 +639,6 @@ static int handle_group_alt(struct objto
 		if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
 			break;
 
-		if (special_alt->skip_orig)
-			insn->type = INSN_NOP;
-
 		insn->alt_group = true;
 		last_orig_insn = insn;
 	}
@@ -808,6 +803,7 @@ static int add_special_section_alts(stru
 		}
 
 		alt->insn = new_insn;
+		alt->skip_orig = special_alt->skip_orig;
 		list_add_tail(&alt->list, &orig_insn->alts);
 
 		list_del(&special_alt->list);
@@ -1883,7 +1879,12 @@ static int validate_branch(struct objtoo
 		insn->visited = true;
 
 		if (!insn->ignore_alts) {
+			bool skip_orig = false;
+
 			list_for_each_entry(alt, &insn->alts, list) {
+				if (alt->skip_orig)
+					skip_orig = true;
+
 				ret = validate_branch(file, alt->insn, state);
 				if (ret) {
 					if (backtrace)
@@ -1891,6 +1892,9 @@ static int validate_branch(struct objtoo
 					return ret;
 				}
 			}
+
+			if (skip_orig)
+				return 0;
 		}
 
 		switch (insn->type) {

[PATCH 22/25] objtool: Fix sibling call detection

[Peter Zijlstra]

It turned out that we failed to detect some sibling calls;
specifically those without relocation records; like:

$ ./objdump-func.sh defconfig-build/mm/kasan/generic.o __asan_loadN
0000 0000000000000840 <__asan_loadN>:
0000  840:      48 8b 0c 24             mov    (%rsp),%rcx
0004  844:      31 d2                   xor    %edx,%edx
0006  846:      e9 45 fe ff ff          jmpq   690 <check_memory_region>

So extend the cross-function jump to also consider those that are not
between known (or newly detected) parent/child functions, as
sibling-cals when they jump to the start of the function.

The second part of that condition is to deal with random jumps to the
middle of other function, as can be found in
arch/x86/lib/copy_user_64.S for example.

This then (with later patches applied) makes the above recognise the
sibling call:

mm/kasan/generic.o: warning: objtool: __asan_loadN()+0x6: call to check_memory_region() with UACCESS enabled

Also make sure to set insn->call_dest for sibling calls so we can know
who we're calling. This is useful information when printing validation
warnings later.


Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   86 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 31 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -515,7 +515,8 @@ static int add_jump_destinations(struct
 			continue;
 		} else {
 			/* sibling call */
-			insn->jump_dest = 0;
+			insn->call_dest = rela->sym;
+			insn->jump_dest = NULL;
 			continue;
 		}
 
@@ -537,25 +538,38 @@ static int add_jump_destinations(struct
 		}
 
 		/*
-		 * For GCC 8+, create parent/child links for any cold
-		 * subfunctions.  This is _mostly_ redundant with a similar
-		 * initialization in read_symbols().
-		 *
-		 * If a function has aliases, we want the *first* such function
-		 * in the symbol table to be the subfunction's parent.  In that
-		 * case we overwrite the initialization done in read_symbols().
-		 *
-		 * However this code can't completely replace the
-		 * read_symbols() code because this doesn't detect the case
-		 * where the parent function's only reference to a subfunction
-		 * is through a switch table.
+		 * Cross-function jump.
 		 */
 		if (insn->func && insn->jump_dest->func &&
-		    insn->func != insn->jump_dest->func &&
-		    !strstr(insn->func->name, ".cold.") &&
-		    strstr(insn->jump_dest->func->name, ".cold.")) {
-			insn->func->cfunc = insn->jump_dest->func;
-			insn->jump_dest->func->pfunc = insn->func;
+		    insn->func != insn->jump_dest->func) {
+
+			/*
+			 * For GCC 8+, create parent/child links for any cold
+			 * subfunctions.  This is _mostly_ redundant with a
+			 * similar initialization in read_symbols().
+			 *
+			 * If a function has aliases, we want the *first* such
+			 * function in the symbol table to be the subfunction's
+			 * parent.  In that case we overwrite the
+			 * initialization done in read_symbols().
+			 *
+			 * However this code can't completely replace the
+			 * read_symbols() code because this doesn't detect the
+			 * case where the parent function's only reference to a
+			 * subfunction is through a switch table.
+			 */
+			if (!strstr(insn->func->name, ".cold.") &&
+			    strstr(insn->jump_dest->func->name, ".cold.")) {
+				insn->func->cfunc = insn->jump_dest->func;
+				insn->jump_dest->func->pfunc = insn->func;
+
+			} else if (insn->jump_dest->func->pfunc != insn->func->pfunc &&
+				   insn->jump_dest->offset == insn->jump_dest->func->offset) {
+
+				/* sibling class */
+				insn->call_dest = insn->jump_dest->func;
+				insn->jump_dest = NULL;
+			}
 		}
 	}
 
@@ -1785,6 +1799,17 @@ static bool insn_state_match(struct inst
 	return false;
 }
 
+static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
+{
+	if (has_modified_stack_frame(state)) {
+		WARN_FUNC("sibling call from callable instruction with modified stack frame",
+				insn->sec, insn->offset);
+		return 1;
+	}
+
+	return 0;
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -1935,9 +1960,14 @@ static int validate_branch(struct objtoo
 
 		case INSN_JUMP_CONDITIONAL:
 		case INSN_JUMP_UNCONDITIONAL:
-			if (insn->jump_dest &&
-			    (!func || !insn->jump_dest->func ||
-			     insn->jump_dest->func->pfunc == func)) {
+			if (func && !insn->jump_dest) {
+				ret = validate_sibling_call(insn, &state);
+				if (ret)
+					return ret;
+
+			} else if (insn->jump_dest &&
+				   (!func || !insn->jump_dest->func ||
+				    insn->jump_dest->func->pfunc == func)) {
 				ret = validate_branch(file, insn->jump_dest,
 						      state);
 				if (ret) {
@@ -1945,11 +1975,6 @@ static int validate_branch(struct objtoo
 						BT_FUNC("(branch)", insn);
 					return ret;
 				}
-
-			} else if (func && has_modified_stack_frame(&state)) {
-				WARN_FUNC("sibling call from callable instruction with modified stack frame",
-					  sec, insn->offset);
-				return 1;
 			}
 
 			if (insn->type == INSN_JUMP_UNCONDITIONAL)
@@ -1958,11 +1983,10 @@ static int validate_branch(struct objtoo
 			break;
 
 		case INSN_JUMP_DYNAMIC:
-			if (func && list_empty(&insn->alts) &&
-			    has_modified_stack_frame(&state)) {
-				WARN_FUNC("sibling call from callable instruction with modified stack frame",
-					  sec, insn->offset);
-				return 1;
+			if (func && list_empty(&insn->alts)) {
+				ret = validate_sibling_call(insn, &state);
+				if (ret)
+					return ret;
 			}
 
 			return 0;

[PATCH 23/25] objtool: Add UACCESS validation

[Peter Zijlstra]

It is important that UACCESS regions are as small as possible;
furthermore the UACCESS state is not scheduled, so doing anything that
might directly call into the scheduler will cause random code to be
ran with UACCESS enabled.

Teach objtool too track UACCESS state and warn about any CALL made
while UACCESS is enabled. This very much includes the __fentry__()
and __preempt_schedule() calls.

Note that exceptions _do_ save/restore the UACCESS state, and therefore
they can drive preemption. This also means that all exception handlers
must have an otherwise redundant UACCESS disable instruction;
therefore ignore this warning for !STT_FUNC code (exception handlers
are not normal functions).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 scripts/Makefile.build          |    3 
 tools/objtool/arch.h            |    4 
 tools/objtool/arch/x86/decode.c |    9 +-
 tools/objtool/builtin-check.c   |    3 
 tools/objtool/builtin.h         |    2 
 tools/objtool/check.c           |  169 +++++++++++++++++++++++++++++++++++++---
 tools/objtool/check.h           |    2 
 tools/objtool/elf.h             |    1 
 tools/objtool/special.c         |   18 ++++
 tools/objtool/special.h         |    1 
 10 files changed, 196 insertions(+), 16 deletions(-)

--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -225,6 +225,9 @@ endif
 ifdef CONFIG_RETPOLINE
   objtool_args += --retpoline
 endif
+ifdef CONFIG_X86_SMAP
+  objtool_args += --uaccess
+endif
 
 # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -33,7 +33,9 @@
 #define INSN_STACK		8
 #define INSN_BUG		9
 #define INSN_NOP		10
-#define INSN_OTHER		11
+#define INSN_STAC		11
+#define INSN_CLAC		12
+#define INSN_OTHER		13
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,14 @@ int arch_decode_instruction(struct elf *
 
 	case 0x0f:
 
-		if (op2 >= 0x80 && op2 <= 0x8f) {
+		if (op2 == 0x01) {
+
+			if (modrm == 0xca)
+				*type = INSN_CLAC;
+			else if (modrm == 0xcb)
+				*type = INSN_STAC;
+
+		} else if (op2 >= 0x80 && op2 <= 0x8f) {
 
 			*type = INSN_JUMP_CONDITIONAL;
 
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable, retpoline, module, backtrace;
+bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -42,6 +42,7 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
 	OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"),
 	OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
+	OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
 	OPT_END(),
 };
 
--- a/tools/objtool/builtin.h
+++ b/tools/objtool/builtin.h
@@ -20,7 +20,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline, module, backtrace;
+extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -443,6 +443,82 @@ static void add_ignores(struct objtool_f
 }
 
 /*
+ * This is a whitelist of functions that is allowed to be called with AC set.
+ * The list is meant to be minimal and only contains compiler instrumentation
+ * ABI and a few functions used to implement *_{to,from}_user() functions.
+ *
+ * These functions must not directly change AC, but may PUSHF/POPF.
+ */
+static const char *uaccess_safe_builtin[] = {
+	/* KASAN */
+	"kasan_report",
+	"check_memory_region",
+	/* KASAN out-of-line */
+	"__asan_loadN_noabort",
+	"__asan_load1_noabort",
+	"__asan_load2_noabort",
+	"__asan_load4_noabort",
+	"__asan_load8_noabort",
+	"__asan_load16_noabort",
+	"__asan_storeN_noabort",
+	"__asan_store1_noabort",
+	"__asan_store2_noabort",
+	"__asan_store4_noabort",
+	"__asan_store8_noabort",
+	"__asan_store16_noabort",
+	/* KASAN in-line */
+	"__asan_report_load_n_noabort",
+	"__asan_report_load1_noabort",
+	"__asan_report_load2_noabort",
+	"__asan_report_load4_noabort",
+	"__asan_report_load8_noabort",
+	"__asan_report_load16_noabort",
+	"__asan_report_store_n_noabort",
+	"__asan_report_store1_noabort",
+	"__asan_report_store2_noabort",
+	"__asan_report_store4_noabort",
+	"__asan_report_store8_noabort",
+	"__asan_report_store16_noabort",
+	/* KCOV */
+	"write_comp_data",
+	"__sanitizer_cov_trace_pc",
+	"__sanitizer_cov_trace_const_cmp1",
+	"__sanitizer_cov_trace_const_cmp2",
+	"__sanitizer_cov_trace_const_cmp4",
+	"__sanitizer_cov_trace_const_cmp8",
+	"__sanitizer_cov_trace_cmp1",
+	"__sanitizer_cov_trace_cmp2",
+	"__sanitizer_cov_trace_cmp4",
+	"__sanitizer_cov_trace_cmp8",
+	/* UBSAN */
+	"ubsan_type_mismatch_common",
+	"__ubsan_handle_type_mismatch",
+	"__ubsan_handle_type_mismatch_v1",
+	/* misc */
+	"csum_partial_copy_generic",
+	"__memcpy_mcsafe",
+	"ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
+	NULL
+};
+
+static void add_uaccess_safe(struct objtool_file *file)
+{
+	struct symbol *func;
+	const char **name;
+
+	if (!uaccess)
+		return;
+
+	for (name = uaccess_safe_builtin; *name; name++) {
+		func = find_symbol_by_name(file->elf, *name);
+		if (!func)
+			continue;
+
+		func->alias->uaccess_safe = true;
+	}
+}
+
+/*
  * FIXME: For now, just ignore any alternatives which add retpolines.  This is
  * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
  * But it at least allows objtool to understand the control flow *around* the
@@ -818,6 +894,7 @@ static int add_special_section_alts(stru
 
 		alt->insn = new_insn;
 		alt->skip_orig = special_alt->skip_orig;
+		orig_insn->ignore_alts |= special_alt->skip_alt;
 		list_add_tail(&alt->list, &orig_insn->alts);
 
 		list_del(&special_alt->list);
@@ -1239,6 +1316,7 @@ static int decode_sections(struct objtoo
 		return ret;
 
 	add_ignores(file);
+	add_uaccess_safe(file);
 
 	ret = add_ignore_alternatives(file);
 	if (ret)
@@ -1799,6 +1877,33 @@ static bool insn_state_match(struct inst
 	return false;
 }
 
+static inline bool func_uaccess_safe(struct symbol *func)
+{
+	if (func)
+		return func->alias->uaccess_safe;
+
+	return false;
+}
+
+static inline const char *insn_dest_name(struct instruction *insn)
+{
+	if (insn->call_dest)
+		return insn->call_dest->name;
+
+	return "{dynamic}";
+}
+
+static int validate_call(struct instruction *insn, struct insn_state *state)
+{
+	if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
+		WARN_FUNC("call to %s() with UACCESS enabled",
+				insn->sec, insn->offset, insn_dest_name(insn));
+		return 1;
+	}
+
+	return 0;
+}
+
 static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
 {
 	if (has_modified_stack_frame(state)) {
@@ -1807,7 +1912,7 @@ static int validate_sibling_call(struct
 		return 1;
 	}
 
-	return 0;
+	return validate_call(insn, state);
 }
 
 /*
@@ -1855,7 +1960,9 @@ static int validate_branch(struct objtoo
 			if (!insn->hint && !insn_state_match(insn, &state))
 				return 1;
 
-			return 0;
+			/* If we were here with AC=0, but now have AC=1, go again */
+			if (insn->state.uaccess || !state.uaccess)
+				return 0;
 		}
 
 		if (insn->hint) {
@@ -1925,6 +2032,16 @@ static int validate_branch(struct objtoo
 		switch (insn->type) {
 
 		case INSN_RETURN:
+			if (state.uaccess && !func_uaccess_safe(func)) {
+				WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
+				return 1;
+			}
+
+			if (!state.uaccess && func_uaccess_safe(func)) {
+				WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function", sec, insn->offset);
+				return 1;
+			}
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -1940,17 +2057,22 @@ static int validate_branch(struct objtoo
 			return 0;
 
 		case INSN_CALL:
-			if (is_fentry_call(insn))
-				break;
+		case INSN_CALL_DYNAMIC:
+			ret = validate_call(insn, &state);
+			if (ret)
+				return ret;
 
-			ret = dead_end_function(file, insn->call_dest);
-			if (ret == 1)
-				return 0;
-			if (ret == -1)
-				return 1;
+			if (insn->type == INSN_CALL) {
+				if (is_fentry_call(insn))
+					break;
+
+				ret = dead_end_function(file, insn->call_dest);
+				if (ret == 1)
+					return 0;
+				if (ret == -1)
+					return 1;
+			}
 
-			/* fallthrough */
-		case INSN_CALL_DYNAMIC:
 			if (!no_fp && func && !has_valid_stack_frame(&state)) {
 				WARN_FUNC("call without frame pointer save/setup",
 					  sec, insn->offset);
@@ -2005,6 +2127,29 @@ static int validate_branch(struct objtoo
 
 			break;
 
+		case INSN_STAC:
+			if (state.uaccess) {
+				WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+				return 1;
+			}
+
+			state.uaccess = true;
+			break;
+
+		case INSN_CLAC:
+			if (!state.uaccess && insn->func) {
+				WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+				return 1;
+			}
+
+			if (func_uaccess_safe(func)) {
+				WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+				return 1;
+			}
+
+			state.uaccess = false;
+			break;
+
 		default:
 			break;
 		}
@@ -2168,6 +2313,8 @@ static int validate_functions(struct obj
 			if (!insn || insn->ignore)
 				continue;
 
+			state.uaccess = func->alias->uaccess_safe;
+
 			ret = validate_branch(file, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (func)", insn);
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct insn_state {
 	int stack_size;
 	unsigned char type;
 	bool bp_scratch;
-	bool drap, end;
+	bool drap, end, uaccess;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@ struct symbol {
 	unsigned long offset;
 	unsigned int len;
 	struct symbol *pfunc, *cfunc, *alias;
+	bool uaccess_safe;
 };
 
 struct rela {
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -23,6 +23,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "builtin.h"
 #include "special.h"
 #include "warn.h"
 
@@ -42,6 +43,7 @@
 #define ALT_NEW_LEN_OFFSET	11
 
 #define X86_FEATURE_POPCNT (4*32+23)
+#define X86_FEATURE_SMAP   (9*32+20)
 
 struct special_entry {
 	const char *sec;
@@ -110,6 +112,22 @@ static int get_alt_entry(struct elf *elf
 		 */
 		if (feature == X86_FEATURE_POPCNT)
 			alt->skip_orig = true;
+
+		/*
+		 * If UACCESS validation is enabled; force that alternative;
+		 * otherwise force it the other way.
+		 *
+		 * What we want to avoid is having both the original and the
+		 * alternative code flow at the same time, in that case we can
+		 * find paths that see the STAC but take the NOP instead of
+		 * CLAC and the other way around.
+		 */
+		if (feature == X86_FEATURE_SMAP) {
+			if (uaccess)
+				alt->skip_orig = true;
+			else
+				alt->skip_alt = true;
+		}
 	}
 
 	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -26,6 +26,7 @@ struct special_alt {
 
 	bool group;
 	bool skip_orig;
+	bool skip_alt;
 	bool jump_or_nop;
 
 	struct section *orig_sec;

Re: [PATCH 23/25] objtool: Add UACCESS validation

[Josh Poimboeuf]

On Mon, Mar 18, 2019 at 04:39:03PM +0100, Peter Zijlstra wrote:
> +++ b/tools/objtool/check.c
> @@ -443,6 +443,82 @@ static void add_ignores(struct objtool_f
>  }
>  
>  /*
> + * This is a whitelist of functions that is allowed to be called with AC set.

s/is/are/

-- 
Josh

Re: [PATCH 23/25] objtool: Add UACCESS validation

[Peter Zijlstra]

On Mon, Mar 18, 2019 at 04:39:03PM +0100, Peter Zijlstra wrote:
> +static const char *uaccess_safe_builtin[] = {
> +	/* KASAN */
> +	"kasan_report",
> +	"check_memory_region",
> +	/* KASAN out-of-line */
> +	"__asan_loadN_noabort",
> +	"__asan_load1_noabort",
> +	"__asan_load2_noabort",
> +	"__asan_load4_noabort",
> +	"__asan_load8_noabort",
> +	"__asan_load16_noabort",
> +	"__asan_storeN_noabort",
> +	"__asan_store1_noabort",
> +	"__asan_store2_noabort",
> +	"__asan_store4_noabort",
> +	"__asan_store8_noabort",
> +	"__asan_store16_noabort",
> +	/* KASAN in-line */
> +	"__asan_report_load_n_noabort",
> +	"__asan_report_load1_noabort",
> +	"__asan_report_load2_noabort",
> +	"__asan_report_load4_noabort",
> +	"__asan_report_load8_noabort",
> +	"__asan_report_load16_noabort",
> +	"__asan_report_store_n_noabort",
> +	"__asan_report_store1_noabort",
> +	"__asan_report_store2_noabort",
> +	"__asan_report_store4_noabort",
> +	"__asan_report_store8_noabort",
> +	"__asan_report_store16_noabort",

So I was building the kernel with GCC-4.9 (for another issue) and that
got me:

arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x69: call to __asan_store8() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x3d: call to __asan_load8() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: __setup_rt_frame()+0x308: call to __asan_load8() with UACCESS enabled
lib/strncpy_from_user.o: warning: objtool: strncpy_from_user()+0x33d: call to __asan_store1() with UACCESS enabled
lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x19: call to __asan_load8() with UACCESS enabled
lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x19: call to __asan_load8() with UACCESS enabled

Can any of the KASAN folks enlighten me on this? Should I also whitelist
those symbols, and do I understand things correct that the difference
between * and *_noabort is absolutely nothing?

[PATCH 24/25] objtool: uaccess PUSHF/POPF support

[Peter Zijlstra]

Add PUSHF / POPF state.uaccess restore logic. This makes
user_access_save() / user_access_restore() 'work' with objtool.

XXX: should be merged with the previous patch such that KASAN
doesn't explode in between. Split for review.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch.h            |    2 ++
 tools/objtool/arch/x86/decode.c |    4 ++--
 tools/objtool/check.c           |   30 ++++++++++++++++++++++++++----
 tools/objtool/check.h           |    1 +
 4 files changed, 31 insertions(+), 6 deletions(-)

--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -43,6 +43,7 @@ enum op_dest_type {
 	OP_DEST_REG_INDIRECT,
 	OP_DEST_MEM,
 	OP_DEST_PUSH,
+	OP_DEST_PUSHF,
 	OP_DEST_LEAVE,
 };
 
@@ -57,6 +58,7 @@ enum op_src_type {
 	OP_SRC_REG_INDIRECT,
 	OP_SRC_CONST,
 	OP_SRC_POP,
+	OP_SRC_POPF,
 	OP_SRC_ADD,
 	OP_SRC_AND,
 };
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -357,13 +357,13 @@ int arch_decode_instruction(struct elf *
 		/* pushf */
 		*type = INSN_STACK;
 		op->src.type = OP_SRC_CONST;
-		op->dest.type = OP_DEST_PUSH;
+		op->dest.type = OP_DEST_PUSHF;
 		break;
 
 	case 0x9d:
 		/* popf */
 		*type = INSN_STACK;
-		op->src.type = OP_SRC_POP;
+		op->src.type = OP_SRC_POPF;
 		op->dest.type = OP_DEST_MEM;
 		break;
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1392,11 +1392,11 @@ static int update_insn_state_regs(struct
 		return 0;
 
 	/* push */
-	if (op->dest.type == OP_DEST_PUSH)
+	if (op->dest.type == OP_DEST_PUSH || op->dest.type == OP_DEST_PUSHF)
 		cfa->offset += 8;
 
 	/* pop */
-	if (op->src.type == OP_SRC_POP)
+	if (op->src.type == OP_SRC_POP || op->src.type == OP_SRC_POPF)
 		cfa->offset -= 8;
 
 	/* add immediate to sp */
@@ -1653,6 +1653,7 @@ static int update_insn_state(struct inst
 			break;
 
 		case OP_SRC_POP:
+		case OP_SRC_POPF:
 			if (!state->drap && op->dest.type == OP_DEST_REG &&
 			    op->dest.reg == cfa->base) {
 
@@ -1717,6 +1718,7 @@ static int update_insn_state(struct inst
 		break;
 
 	case OP_DEST_PUSH:
+	case OP_DEST_PUSHF:
 		state->stack_size += 8;
 		if (cfa->base == CFI_SP)
 			cfa->offset += 8;
@@ -1807,7 +1809,7 @@ static int update_insn_state(struct inst
 		break;
 
 	case OP_DEST_MEM:
-		if (op->src.type != OP_SRC_POP) {
+		if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) {
 			WARN_FUNC("unknown stack-related memory operation",
 				  insn->sec, insn->offset);
 			return -1;
@@ -2109,6 +2111,26 @@ static int validate_branch(struct objtoo
 			if (update_insn_state(insn, &state))
 				return 1;
 
+			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
+				if (!state.uaccess_stack) {
+					state.uaccess_stack = 1;
+				} else if (state.uaccess_stack >> 31) {
+					WARN_FUNC("PUSHF stack exhausted", sec, insn->offset);
+					return 1;
+				}
+				state.uaccess_stack <<= 1;
+				state.uaccess_stack  |= state.uaccess;
+			}
+
+			if (insn->stack_op.src.type == OP_SRC_POPF) {
+				if (state.uaccess_stack) {
+					state.uaccess = state.uaccess_stack & 1;
+					state.uaccess_stack >>= 1;
+					if (state.uaccess_stack == 1)
+						state.uaccess_stack = 0;
+				}
+			}
+
 			break;
 
 		case INSN_STAC:
@@ -2126,7 +2148,7 @@ static int validate_branch(struct objtoo
 				return 1;
 			}
 
-			if (func_uaccess_safe(func)) {
+			if (func_uaccess_safe(func) && !state.uaccess_stack) {
 				WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
 				return 1;
 			}
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -32,6 +32,7 @@ struct insn_state {
 	unsigned char type;
 	bool bp_scratch;
 	bool drap, end, uaccess;
+	unsigned int uaccess_stack;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
 };

[PATCH 25/25] objtool: Add Direction Flag validation

[Peter Zijlstra]

Having DF escape is BAD(tm).

Linus; you suggested this one, but since DF really is only used from
ASM and the failure case is fairly obvious, do we really need this?
OTOH the patch is fairly small and simple.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch.h            |    4 +++-
 tools/objtool/arch/x86/decode.c |    8 ++++++++
 tools/objtool/check.c           |   25 +++++++++++++++++++++++++
 tools/objtool/check.h           |    2 +-
 4 files changed, 37 insertions(+), 2 deletions(-)

--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -35,7 +35,9 @@
 #define INSN_NOP		10
 #define INSN_STAC		11
 #define INSN_CLAC		12
-#define INSN_OTHER		13
+#define INSN_STD		13
+#define INSN_CLD		14
+#define INSN_OTHER		15
 #define INSN_LAST		INSN_OTHER
 
 enum op_dest_type {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -451,6 +451,14 @@ int arch_decode_instruction(struct elf *
 		*type = INSN_CALL;
 		break;
 
+	case 0xfc:
+		*type = INSN_CLD;
+		break;
+
+	case 0xfd:
+		*type = INSN_STD;
+		break;
+
 	case 0xff:
 		if (modrm_reg == 2 || modrm_reg == 3)
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1903,6 +1903,12 @@ static int validate_call(struct instruct
 		return 1;
 	}
 
+	if (state->df) {
+		WARN_FUNC("call to %s() with DF set",
+				insn->sec, insn->offset, insn_dest_name(insn));
+		return 1;
+	}
+
 	return 0;
 }
 
@@ -2044,6 +2050,11 @@ static int validate_branch(struct objtoo
 				return 1;
 			}
 
+			if (state.df) {
+				WARN_FUNC("return with DF set", sec, insn->offset);
+				return 1;
+			}
+
 			if (func && has_modified_stack_frame(&state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
@@ -2172,6 +2183,20 @@ static int validate_branch(struct objtoo
 			state.uaccess = false;
 			break;
 
+		case INSN_STD:
+			if (state.df)
+				WARN_FUNC("recursive STD", sec, insn->offset);
+
+			state.df = true;
+			break;
+
+		case INSN_CLD:
+			if (!state.df && insn->func)
+				WARN_FUNC("redundant CLD", sec, insn->offset);
+
+			state.df = false;
+			break;
+
 		default:
 			break;
 		}
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct insn_state {
 	int stack_size;
 	unsigned char type;
 	bool bp_scratch;
-	bool drap, end, uaccess;
+	bool drap, end, uaccess, df;
 	unsigned int uaccess_stack;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];

Re: [PATCH 00/25] objtool: UACCESS validation v4

[Josh Poimboeuf]

On Mon, Mar 18, 2019 at 04:38:40PM +0100, Peter Zijlstra wrote:
> Teach objtool to validate the UACCESS (SMAP, PAN) rules which are currently
> unenforced and (therefore obviously) violated.
> 
> UACCESS sections should be small; we want to limit the amount of code that can
> touch userspace. Furthermore, UACCESS state isn't scheduled, this means that
> anything that directly calls into the scheduler will result in random code
> running with UACCESS enabled and possibly getting back into the UACCESS region
> with UACCESS disabled and causing faults.
> 
> Forbid any CALL/RET while UACCESS is enabled; but provide a few exceptions.
> 
> This builds x86_64-allmodconfig and lots of x86_64-randconfig clean.
> 
> Changes since -v3:
> 
>  - removed a bunch of functions from the UACCESS-safe list
>    due to the removal of CONFIG_KASAN_EXTRA=y.
> 
>  - hopefully addressed all the feedback from Josh
> 
>  - realized objtool doesn't cover x86_32
> 
>  - some added additional annotations/fixes: kcov, signal
> 
>  - retains the DF check for now, Linus, do you (still) think it is worth doing
>    that DF check?

I'm still not crazy about the DF thing, but otherwise everything looks
great.

For the objtool bits:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH 00/25] objtool: UACCESS validation v4

[Peter Zijlstra]

On Mon, Mar 18, 2019 at 06:57:07PM -0500, Josh Poimboeuf wrote:
> For the objtool bits:
> 
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

Thanks!

I'll go fold the PUSHF/POPF bits into the AC patch then.

[PATCH 26/25] sched/x86_64: Don't save flags on context switch

[Peter Zijlstra]

Subject: sched/x86_64: Don't save flags on context switch
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Mar 19 11:35:46 CET 2019

Now that we have objtool validating AC=1 state for all x86_64 code,
we can once again guarantee clean flags on schedule.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_64.S        |    2 --
 arch/x86/include/asm/switch_to.h |    2 +-
 arch/x86/kernel/process_64.c     |    7 -------
 3 files changed, 1 insertion(+), 10 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -291,7 +291,6 @@ ENTRY(__switch_to_asm)
 	pushq	%r13
 	pushq	%r14
 	pushq	%r15
-	pushfq
 
 	/* switch stack */
 	movq	%rsp, TASK_threadsp(%rdi)
@@ -314,7 +313,6 @@ ENTRY(__switch_to_asm)
 #endif
 
 	/* restore callee-saved registers */
-	popfq
 	popq	%r15
 	popq	%r14
 	popq	%r13
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -40,13 +40,13 @@ asmlinkage void ret_from_fork(void);
  * order of the fields must match the code in __switch_to_asm().
  */
 struct inactive_task_frame {
-	unsigned long flags;
 #ifdef CONFIG_X86_64
 	unsigned long r15;
 	unsigned long r14;
 	unsigned long r13;
 	unsigned long r12;
 #else
+	unsigned long flags;
 	unsigned long si;
 	unsigned long di;
 #endif
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -393,13 +393,6 @@ int copy_thread_tls(unsigned long clone_
 	fork_frame = container_of(childregs, struct fork_frame, regs);
 	frame = &fork_frame->frame;
 
-	/*
-	 * For a new task use the RESET flags value since there is no before.
-	 * All the status flags are zero; DF and all the system flags must also
-	 * be 0, specifically IF must be 0 because we context switch to the new
-	 * task with interrupts disabled.
-	 */
-	frame->flags = X86_EFLAGS_FIXED;
 	frame->bp = 0;
 	frame->ret_addr = (unsigned long) ret_from_fork;
 	p->thread.sp = (unsigned long) fork_frame;