Re: [patch V2 08/17] x86/entry: Move syscall irq tracing to C code

[Andy Lutomirski]

On Wed, Oct 23, 2019 at 5:31 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> Interrupt state tracing can be safely done in C code. The few stack
> operations in assembly do not need to be covered.
>
> Remove the now pointless indirection via .Lsyscall_32_done and jump to
> swapgs_restore_regs_and_return_to_usermode directly.

This doesn't look right.

>  #define SYSCALL_EXIT_WORK_FLAGS                                \
> @@ -279,6 +282,9 @@ static void syscall_slow_exit_work(struc
>  {
>         struct thread_info *ti;
>
> +       /* User to kernel transition disabled interrupts. */
> +       trace_hardirqs_off();
> +

So you just traced IRQs off, but...

>         enter_from_user_mode();
>         local_irq_enable();

Now they're on and traced on again?

I also don't see how your patch handles the fastpath case.

How about the attached patch instead?
commit e154af0d7ff2d605f155f5aca059e3e835e426d4
Author: Andy Lutomirski <luto@xxxxxxxxxx>
Date:   Fri Aug 2 10:30:44 2019 -0700

    x86/entry: Move exit-to-usermode irqflag tracing to prepare_exit_to_usermode()
    
    prepare_exit_to_usermode() can easily handle irqflag tracing.  Move
    the logic there and remove it from the entry asm.
    
    Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index cc49380ef8ab..f4ce0cf2fb74 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -217,6 +217,13 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
 
 	user_enter_irqoff();
 
+	/*
+	 * The actual return to usermode will almost certainly turn IRQs on.
+	 * Trace it here to simplify the asm code.
+	 */
+	if (likely(regs->flags & X86_EFLAGS_IF))
+		trace_hardirqs_on();
+
 	mds_user_clear_cpu_buffers();
 }
 
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index f83ca5aa8b77..c703c29bebb1 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1062,7 +1062,6 @@ ENTRY(entry_INT80_32)
 	STACKLEAK_ERASE
 
 restore_all:
-	TRACE_IRQS_IRET
 	SWITCH_TO_ENTRY_STACK
 .Lrestore_all_notrace:
 	CHECK_AND_APPLY_ESPFIX
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 3abae80d4902..056419a0e76f 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -172,8 +172,6 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
 	movq	%rsp, %rsi
 	call	do_syscall_64		/* returns with IRQs disabled */
 
-	TRACE_IRQS_IRETQ		/* we're about to change IF */
-
 	/*
 	 * Try to use SYSRET instead of IRET if we're returning to
 	 * a completely clean 64-bit userspace context.  If we're not,
@@ -617,7 +615,6 @@ ret_from_intr:
 .Lretint_user:
 	mov	%rsp,%rdi
 	call	prepare_exit_to_usermode
-	TRACE_IRQS_IRETQ
 
 GLOBAL(swapgs_restore_regs_and_return_to_usermode)
 #ifdef CONFIG_DEBUG_ENTRY
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 435df637f392..3502f38bde01 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -254,7 +254,6 @@ sysret32_from_system_call:
 	 * stack. So let's erase the thread stack right now.
 	 */
 	STACKLEAK_ERASE
-	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
 	movq	RBX(%rsp), %rbx		/* pt_regs->rbx */
 	movq	RBP(%rsp), %rbp		/* pt_regs->rbp */
 	movq	EFLAGS(%rsp), %r11	/* pt_regs->flags (in r11) */
@@ -396,6 +395,5 @@ ENTRY(entry_INT80_compat)
 .Lsyscall_32_done:
 
 	/* Go back to user mode. */
-	TRACE_IRQS_ON
 	jmp	swapgs_restore_regs_and_return_to_usermode
 END(entry_INT80_compat)