RE: [PATCH v2] [LBR] Dump LBRs on Exception

[Berthier, Emmanuel]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
> Sent: Thursday, November 27, 2014 10:56 PM
> To: Thomas Gleixner
> Cc: Berthier, Emmanuel; H. Peter Anvin; X86 ML; Jarzmik, Robert; LKML
> Subject: Re: [PATCH v2] [LBR] Dump LBRs on Exception
> 
> On Thu, Nov 27, 2014 at 1:22 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> wrote:
> >>  /*
> >>   * Exception entry points.
> >>   */
> >> @@ -1063,6 +1103,8 @@ ENTRY(\sym)
> >>       subq $ORIG_RAX-R15, %rsp
> >>       CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
> >>
> >> +     STOP_LBR
> >
> > We really cannot do this unconditionally for every exception. This
> > wants to be conditional, i.e.
> >
> >        .if \stop_lbr
> >        cond_stop_lbr
> >        .endif
> >
> > So we can select which exceptions actually get that treatment.
> > do_page_fault is probably the only one which is interesting here.
> >
> > Now looking at your macro maze, I really wonder whether we can do it a
> > little bit less convoluted. We need to push/pop registers. error_entry
> > saves the registers already and has a (admitedly convoluted)
> > kernel/user space check. But we might be able to do something sane
> > there. Cc'ing Andy as he is the master of that universe.
> >
> 
> Can one of you give me some context as to what this code is intended to do?
> I haven't followed the thread.
> 
> In particular, knowing why this needs to be in asm instead of in C would be
> nice, because asm in entry_64.S has an amazing ability to have little bugs
> hiding for years.
> 
> There's also the caveat that, especialy for the IST exceptions, you're running
> in a weird context in which lots of things that are usually safe are verboten.
> Page faults can be tricky too, though.
> 
> --Andy

Welcome Andy.
The global purpose of this patch is to disable/enable LBR during exception handling and dump them later in the Panic process in order to get a small instruction trace which could help in case of stack corruption by example.
This has to be done at the very early stage of exception handling as LBR contains few records (8 or 16) and we do not want to flush useful ones (those before the exception), so this code should avoid executing any jump/branch/call before stopping the LBR.

The proposed patch regarding asm code is as follow:

 diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S index df088bb..f39cded 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1035,6 +1035,46 @@ apicinterrupt IRQ_WORK_VECTOR \
 	irq_work_interrupt smp_irq_work_interrupt  #endif
 
+.macro STOP_LBR
+#ifdef CONFIG_LBR_DUMP_ON_EXCEPTION
+	testl $3,CS+8(%rsp)		/* Kernel Space? */
+	jz 1f
+	testl $1, lbr_dump_on_exception
+	jz 1f
+	push %rax
+	push %rcx
+	push %rdx
+	movl $MSR_IA32_DEBUGCTLMSR, %ecx
+	rdmsr
+	and $~1, %eax			/* Disable LBR recording */
+	wrmsr
+	pop %rdx
+	pop %rcx
+	pop %rax
+1:
+#endif
+.endm
+
+.macro START_LBR
+#ifdef CONFIG_LBR_DUMP_ON_EXCEPTION
+	testl $3,CS+8(%rsp)		/* Kernel Space? */
+	jz 1f
+	testl $1, lbr_dump_on_exception
+	jz 1f
+	push %rax
+	push %rcx
+	push %rdx
+	movl $MSR_IA32_DEBUGCTLMSR, %ecx
+	rdmsr
+	or $1, %eax			/* Enable LBR recording */
+	wrmsr
+	pop %rdx
+	pop %rcx
+	pop %rax
+1:
+#endif
+.endm
+
 /*
  * Exception entry points.
  */
@@ -1063,6 +1103,8 @@ ENTRY(\sym)
 	subq $ORIG_RAX-R15, %rsp
 	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 
+	STOP_LBR
+
 	.if \paranoid
 	call save_paranoid
 	.else
@@ -1094,6 +1136,8 @@ ENTRY(\sym)
 
 	call \do_sym
 
+	START_LBR
+
 	.if \shift_ist != -1
 	addq $EXCEPTION_STKSZ, INIT_TSS_IST(\shift_ist)
 	.endif
--
1.7.9.5

Thanks,
Emmanuel.
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå