* [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling @ 2019-05-31 8:59 Jan Beulich 2019-05-31 8:59 ` [Xen-devel] " Jan Beulich ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 8:59 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne 1: guard top-of-stack reads 2: widen condition for logging top-of-stack The issue patch 2 fixes (a curious lack of an intermediate call stack entry) was observed in practice; patch 1 is a result of me just looking at the code (and if I have missed some aspect of why this isn't a problem in reality, that patch could be easily dropped). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling 2019-05-31 8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich @ 2019-05-31 8:59 ` Jan Beulich 2019-05-31 9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 8:59 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne 1: guard top-of-stack reads 2: widen condition for logging top-of-stack The issue patch 2 fixes (a curious lack of an intermediate call stack entry) was observed in practice; patch 1 is a result of me just looking at the code (and if I have missed some aspect of why this isn't a problem in reality, that patch could be easily dropped). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] x86/traps: guard top-of-stack reads 2019-05-31 8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 2019-05-31 8:59 ` [Xen-devel] " Jan Beulich @ 2019-05-31 9:17 ` Jan Beulich 2019-05-31 9:17 ` [Xen-devel] " Jan Beulich 2019-06-07 17:51 ` Andrew Cooper 2019-05-31 9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 2019-06-17 8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 3 siblings, 2 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 9:17 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Nothing (afaics) guarantees that the original frame's stack pointer points at readable memory. Avoid a (likely nested) crash by attaching exception recovery to the read (making it a single read at the same time). Don't even invoke _show_trace() in case of a non-readable top slot. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp static void show_trace(const struct cpu_user_regs *regs) { - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; printk("Xen call trace:\n"); + asm ( "1: mov %2,%0; 2:\n" + ".pushsection .fixup,\"ax\"\n" + "3: xor %k1,%k1; jmp 2b\n" + ".popsection\n" + _ASM_EXTABLE(1b, 3b) + : "+r" (tos), "+r" (sp) : "m" (*sp) ); + /* * If RIP looks sensible, or the top of the stack doesn't, print RIP at * the top of the stack trace. */ if ( is_active_kernel_text(regs->rip) || - !is_active_kernel_text(*sp) ) + !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); /* * Else RIP looks bad but the top of the stack looks good. Perhaps we @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else + else if ( sp ) { - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); sp++; } + if ( !sp ) + return; + _show_trace((unsigned long)sp, regs->rbp); printk("\n"); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH 1/2] x86/traps: guard top-of-stack reads 2019-05-31 9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich @ 2019-05-31 9:17 ` Jan Beulich 2019-06-07 17:51 ` Andrew Cooper 1 sibling, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 9:17 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Nothing (afaics) guarantees that the original frame's stack pointer points at readable memory. Avoid a (likely nested) crash by attaching exception recovery to the read (making it a single read at the same time). Don't even invoke _show_trace() in case of a non-readable top slot. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp static void show_trace(const struct cpu_user_regs *regs) { - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; printk("Xen call trace:\n"); + asm ( "1: mov %2,%0; 2:\n" + ".pushsection .fixup,\"ax\"\n" + "3: xor %k1,%k1; jmp 2b\n" + ".popsection\n" + _ASM_EXTABLE(1b, 3b) + : "+r" (tos), "+r" (sp) : "m" (*sp) ); + /* * If RIP looks sensible, or the top of the stack doesn't, print RIP at * the top of the stack trace. */ if ( is_active_kernel_text(regs->rip) || - !is_active_kernel_text(*sp) ) + !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); /* * Else RIP looks bad but the top of the stack looks good. Perhaps we @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else + else if ( sp ) { - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); sp++; } + if ( !sp ) + return; + _show_trace((unsigned long)sp, regs->rbp); printk("\n"); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] x86/traps: guard top-of-stack reads 2019-05-31 9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich 2019-05-31 9:17 ` [Xen-devel] " Jan Beulich @ 2019-06-07 17:51 ` Andrew Cooper 2019-06-11 9:57 ` Jan Beulich 1 sibling, 1 reply; 19+ messages in thread From: Andrew Cooper @ 2019-06-07 17:51 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: WeiLiu, Roger Pau Monne On 31/05/2019 10:17, Jan Beulich wrote: > Nothing (afaics) guarantees that the original frame's stack pointer > points at readable memory. Having hit just the scenario described here, the answer is "nothing". > Avoid a (likely nested) crash by attaching > exception recovery to the read (making it a single read at the same > time). Don't even invoke _show_trace() in case of a non-readable top > slot. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp > > static void show_trace(const struct cpu_user_regs *regs) > { > - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); > + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; > > printk("Xen call trace:\n"); > /* Probe the stack for readability. */ > + asm ( "1: mov %2,%0; 2:\n" > + ".pushsection .fixup,\"ax\"\n" > + "3: xor %k1,%k1; jmp 2b\n" Can we use some named parameters, so the asm can actually be followed? Also, you can't do this by zeroing sp, because it aliases with "sp was at zero and readable". A better option would be to get an explicit fault boolean out of the asm. > + ".popsection\n" > + _ASM_EXTABLE(1b, 3b) > + : "+r" (tos), "+r" (sp) : "m" (*sp) ); > + > /* > * If RIP looks sensible, or the top of the stack doesn't, print RIP at > * the top of the stack trace. > */ > if ( is_active_kernel_text(regs->rip) || > - !is_active_kernel_text(*sp) ) > + !is_active_kernel_text(tos) ) > printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); > /* > * Else RIP looks bad but the top of the stack looks good. Perhaps we > @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ > * return address; print it and skip past so _show_trace() doesn't print > * it again. > */ > - else > + else if ( sp ) > { > - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); > + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); > sp++; > } > > + if ( !sp ) > + return; Along with the alias mentioned above, this also has a boundary case when sp is -8, due to the sp++ above. It would probably be better to fit an else if ( fault ) { printk(" [Fault on access]\n"); return; } into the middle of the existing if/else. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] x86/traps: guard top-of-stack reads 2019-06-07 17:51 ` Andrew Cooper @ 2019-06-11 9:57 ` Jan Beulich 0 siblings, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-06-11 9:57 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, WeiLiu, Roger Pau Monne >>> On 07.06.19 at 19:51, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 10:17, Jan Beulich wrote: >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp >> >> static void show_trace(const struct cpu_user_regs *regs) >> { >> - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); >> + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; >> >> printk("Xen call trace:\n"); >> > > /* Probe the stack for readability. */ That's not an appropriate comment for this code fragment, at least not with my (non-native) understanding of "probe". To me the verb does not include reading actual data, yet that's what we do here. If anything is needed at all, then maybe "Guarded read of the stack top"? >> + asm ( "1: mov %2,%0; 2:\n" >> + ".pushsection .fixup,\"ax\"\n" >> + "3: xor %k1,%k1; jmp 2b\n" > > Can we use some named parameters, so the asm can actually be followed? Sure. I did consider doing so, but then thought the one here would be simple enough. > Also, you can't do this by zeroing sp, because it aliases with "sp was > at zero and readable". A better option would be to get an explicit > fault boolean out of the asm. Hmm, this was actually deliberate: A zero %rsp is a clear sign of the stack being bad, and better not getting dumped from. >> @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_ >> * return address; print it and skip past so _show_trace() doesn't print >> * it again. >> */ >> - else >> + else if ( sp ) >> { >> - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); >> + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); >> sp++; >> } >> >> + if ( !sp ) >> + return; > > Along with the alias mentioned above, this also has a boundary case when > sp is -8, due to the sp++ above. Hmm, yes, until the next patch. > It would probably be better to fit an > > else if ( fault ) > { > printk(" [Fault on access]\n"); > return; > } > > into the middle of the existing if/else. Well, okay, I'll add such a separate boolean then. I wanted to avoid further hampering readability of the asm()... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] x86/traps: widen condition for logging top-of-stack 2019-05-31 8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 2019-05-31 8:59 ` [Xen-devel] " Jan Beulich 2019-05-31 9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich @ 2019-05-31 9:22 ` Jan Beulich 2019-05-31 9:22 ` [Xen-devel] " Jan Beulich 2019-06-07 18:01 ` Andrew Cooper 2019-06-17 8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 3 siblings, 2 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 9:22 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, often for relatively simple leaf functions. (To give a specific example, the case I've run into this with is _pci_hide_device() and gcc 8. Interestingly the even more simple neighboring iommu_has_feature() does get a frame pointer set up, around just a single instruction. But this may be a result of the size-of-asm() effects discussed elsewhere.) Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. Also annotate non-frame-pointer-based stack trace entries with a question mark, to signal clearly that any one of them may not actually be part of the call stack. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -431,7 +431,7 @@ static void _show_trace(unsigned long sp { addr = *stack++; if ( is_active_kernel_text(addr) ) - printk(" [<%p>] %pS\n", _p(addr), _p(addr)); + printk(" [<%p>] ? %pS\n", _p(addr), _p(addr)); } } @@ -502,21 +502,25 @@ static void show_trace(const struct cpu_ if ( is_active_kernel_text(regs->rip) || !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); + + if ( !sp ) + return; + /* - * Else RIP looks bad but the top of the stack looks good. Perhaps we - * followed a wild function pointer? Lets assume the top of the stack is a + * If RIP looks bad or the top of the stack looks good, log the top of + * stack as well. Perhaps we followed a wild function pointer, or we're + * in a function without frame pointer, or in a function prologue before + * the frame pointer gets set up? Lets assume the top of the stack is a * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else if ( sp ) + if ( !is_active_kernel_text(regs->rip) || + is_active_kernel_text(tos) ) { - printk(" [<%p>] %pS\n", _p(tos), _p(tos)); + printk(" [<%p>] ? %pS\n", _p(tos), _p(tos)); sp++; } - if ( !sp ) - return; - _show_trace((unsigned long)sp, regs->rbp); printk("\n"); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH 2/2] x86/traps: widen condition for logging top-of-stack 2019-05-31 9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich @ 2019-05-31 9:22 ` Jan Beulich 2019-06-07 18:01 ` Andrew Cooper 1 sibling, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-05-31 9:22 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, often for relatively simple leaf functions. (To give a specific example, the case I've run into this with is _pci_hide_device() and gcc 8. Interestingly the even more simple neighboring iommu_has_feature() does get a frame pointer set up, around just a single instruction. But this may be a result of the size-of-asm() effects discussed elsewhere.) Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. Also annotate non-frame-pointer-based stack trace entries with a question mark, to signal clearly that any one of them may not actually be part of the call stack. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -431,7 +431,7 @@ static void _show_trace(unsigned long sp { addr = *stack++; if ( is_active_kernel_text(addr) ) - printk(" [<%p>] %pS\n", _p(addr), _p(addr)); + printk(" [<%p>] ? %pS\n", _p(addr), _p(addr)); } } @@ -502,21 +502,25 @@ static void show_trace(const struct cpu_ if ( is_active_kernel_text(regs->rip) || !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); + + if ( !sp ) + return; + /* - * Else RIP looks bad but the top of the stack looks good. Perhaps we - * followed a wild function pointer? Lets assume the top of the stack is a + * If RIP looks bad or the top of the stack looks good, log the top of + * stack as well. Perhaps we followed a wild function pointer, or we're + * in a function without frame pointer, or in a function prologue before + * the frame pointer gets set up? Lets assume the top of the stack is a * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else if ( sp ) + if ( !is_active_kernel_text(regs->rip) || + is_active_kernel_text(tos) ) { - printk(" [<%p>] %pS\n", _p(tos), _p(tos)); + printk(" [<%p>] ? %pS\n", _p(tos), _p(tos)); sp++; } - if ( !sp ) - return; - _show_trace((unsigned long)sp, regs->rbp); printk("\n"); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] x86/traps: widen condition for logging top-of-stack 2019-05-31 9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 2019-05-31 9:22 ` [Xen-devel] " Jan Beulich @ 2019-06-07 18:01 ` Andrew Cooper 2019-06-11 9:46 ` Jan Beulich 1 sibling, 1 reply; 19+ messages in thread From: Andrew Cooper @ 2019-06-07 18:01 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: WeiLiu, Roger Pau Monne On 31/05/2019 10:22, Jan Beulich wrote: > Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, > often for relatively simple leaf functions. (To give a specific example, > the case I've run into this with is _pci_hide_device() and gcc 8. > Interestingly the even more simple neighboring iommu_has_feature() does > get a frame pointer set up, around just a single instruction. But this > may be a result of the size-of-asm() effects discussed elsewhere.) > > Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. > > Also annotate non-frame-pointer-based stack trace entries with a > question mark, to signal clearly that any one of them may not actually > be part of the call stack. I very specifically didn't do that before, because it give the false impression that when a question mark isn't present, the logging line is accurate. This is not true for %rbp corruption, asm blocks which don't respect the frame pointer ABI (arguably also corruption), any fault raised from within an EFI call. Porting Xen to use objtool would be a definite improvement, but cannot guard against unexpected %rbp corruption and the EFI case. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] x86/traps: widen condition for logging top-of-stack 2019-06-07 18:01 ` Andrew Cooper @ 2019-06-11 9:46 ` Jan Beulich 0 siblings, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-06-11 9:46 UTC (permalink / raw) To: Andrew Cooper, xen-devel; +Cc: WeiLiu, Roger Pau Monne >>> On 07.06.19 at 20:01, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 10:22, Jan Beulich wrote: >> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, >> often for relatively simple leaf functions. (To give a specific example, >> the case I've run into this with is _pci_hide_device() and gcc 8. >> Interestingly the even more simple neighboring iommu_has_feature() does >> get a frame pointer set up, around just a single instruction. But this >> may be a result of the size-of-asm() effects discussed elsewhere.) >> >> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. >> >> Also annotate non-frame-pointer-based stack trace entries with a >> question mark, to signal clearly that any one of them may not actually >> be part of the call stack. > > I very specifically didn't do that before, because it give the false > impression that when a question mark isn't present, the logging line is > accurate. > > This is not true for %rbp corruption, asm blocks which don't respect the > frame pointer ABI (arguably also corruption), any fault raised from > within an EFI call. So what do you suggest instead? Somehow we should mark slots that are more guesses than actually derived. > Porting Xen to use objtool would be a definite improvement, but cannot > guard against unexpected %rbp corruption and the EFI case. I'm not sure about "definite", but I think I see your point. Personally I continue to believe that programmer (assembly code) and compiler (C code) attached unwind annotations are the better model. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling 2019-05-31 8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich ` (2 preceding siblings ...) 2019-05-31 9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich @ 2019-06-17 8:10 ` Jan Beulich 2019-06-17 8:12 ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich 2019-06-17 8:13 ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 3 siblings, 2 replies; 19+ messages in thread From: Jan Beulich @ 2019-06-17 8:10 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne 1: guard top-of-stack reads 2: widen condition for logging top-of-stack The issue patch 2 fixes (a curious lack of an intermediate call stack entry) was observed in practice; patch 1 is a result of me just looking at the code. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads 2019-06-17 8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich @ 2019-06-17 8:12 ` Jan Beulich 2019-07-02 17:47 ` Andrew Cooper 2019-06-17 8:13 ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 1 sibling, 1 reply; 19+ messages in thread From: Jan Beulich @ 2019-06-17 8:12 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Nothing (afaics) guarantees that the original frame's stack pointer points at readable memory. Avoid a (likely nested) crash by attaching exception recovery to the read (making it a single read at the same time). Don't even invoke _show_trace() in case of a non-readable top slot. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Name asm() arguments. Use explicit "fault" variable. --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -484,17 +484,31 @@ static void _show_trace(unsigned long sp static void show_trace(const struct cpu_user_regs *regs) { - unsigned long *sp = ESP_BEFORE_EXCEPTION(regs); + unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0; + bool fault = false; printk("Xen call trace:\n"); + /* Guarded read of the stack top. */ + asm ( "1: mov %[data], %[tos]; 2:\n" + ".pushsection .fixup,\"ax\"\n" + "3: movb $1, %[fault]; jmp 2b\n" + ".popsection\n" + _ASM_EXTABLE(1b, 3b) + : [tos] "+r" (tos), [fault] "+qm" (fault) : [data] "m" (*sp) ); + /* * If RIP looks sensible, or the top of the stack doesn't, print RIP at * the top of the stack trace. */ if ( is_active_kernel_text(regs->rip) || - !is_active_kernel_text(*sp) ) + !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); + else if ( fault ) + { + printk(" [Fault on access]\n"); + return; + } /* * Else RIP looks bad but the top of the stack looks good. Perhaps we * followed a wild function pointer? Lets assume the top of the stack is a @@ -503,7 +517,7 @@ static void show_trace(const struct cpu_ */ else { - printk(" [<%p>] %pS\n", _p(*sp), _p(*sp)); + printk(" [<%p>] %pS\n", _p(tos), _p(tos)); sp++; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads 2019-06-17 8:12 ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich @ 2019-07-02 17:47 ` Andrew Cooper 2019-07-03 7:10 ` Jan Beulich 0 siblings, 1 reply; 19+ messages in thread From: Andrew Cooper @ 2019-07-02 17:47 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: WeiLiu, Roger Pau Monne On 17/06/2019 09:12, Jan Beulich wrote: > Nothing (afaics) guarantees that the original frame's stack pointer I'd drop the (afaics), because the answer really is nothing. > points at readable memory. Avoid a (likely nested) crash by attaching > exception recovery to the read (making it a single read at the same > time). Don't even invoke _show_trace() in case of a non-readable top > slot. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads 2019-07-02 17:47 ` Andrew Cooper @ 2019-07-03 7:10 ` Jan Beulich 0 siblings, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-07-03 7:10 UTC (permalink / raw) To: Andrew Cooper, xen-devel; +Cc: WeiLiu, Roger Pau Monne On 02.07.2019 19:47, Andrew Cooper wrote: > On 17/06/2019 09:12, Jan Beulich wrote: >> Nothing (afaics) guarantees that the original frame's stack pointer > > I'd drop the (afaics), because the answer really is nothing. Well, okay, done. >> points at readable memory. Avoid a (likely nested) crash by attaching >> exception recovery to the read (making it a single read at the same >> time). Don't even invoke _show_trace() in case of a non-readable top >> slot. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. FTR - there's a quirk in here that I've left in place deliberately (should probably have mentioned it in a post-commit- message remark) which gets resolved by patch 2, and hence I'm likely going to wait with committing this such that both can go in at the same time. The issue is with the if/else-if/else chain here, which patch 2 makes into a series of plain if()-s. Handling this correctly right here would imo mean folding together both patches; anything else would at best result in clumsy intermediate code. Despite this quirk the change here is an improvement, just not as much of one as would be desirable. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack 2019-06-17 8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 2019-06-17 8:12 ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich @ 2019-06-17 8:13 ` Jan Beulich 2019-07-03 10:21 ` Andrew Cooper 1 sibling, 1 reply; 19+ messages in thread From: Jan Beulich @ 2019-06-17 8:13 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, WeiLiu, Roger Pau Monne Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, often for relatively simple leaf functions. (To give a specific example, the case I've run into this with is _pci_hide_device() and gcc 8. Interestingly the even more simple neighboring iommu_has_feature() does get a frame pointer set up, around just a single instruction. But this may be a result of the size-of-asm() effects discussed elsewhere.) Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. Also annotate non-frame-pointer-based stack trace entries with a question mark, to signal clearly that any one of them may not actually be part of the call stack. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Re-base over changes to earlier patch. --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -431,7 +431,7 @@ static void _show_trace(unsigned long sp { addr = *stack++; if ( is_active_kernel_text(addr) ) - printk(" [<%p>] %pS\n", _p(addr), _p(addr)); + printk(" [<%p>] ? %pS\n", _p(addr), _p(addr)); } } @@ -504,20 +504,25 @@ static void show_trace(const struct cpu_ if ( is_active_kernel_text(regs->rip) || !is_active_kernel_text(tos) ) printk(" [<%p>] %pS\n", _p(regs->rip), _p(regs->rip)); - else if ( fault ) + + if ( fault ) { printk(" [Fault on access]\n"); return; } + /* - * Else RIP looks bad but the top of the stack looks good. Perhaps we - * followed a wild function pointer? Lets assume the top of the stack is a + * If RIP looks bad or the top of the stack looks good, log the top of + * stack as well. Perhaps we followed a wild function pointer, or we're + * in a function without frame pointer, or in a function prologue before + * the frame pointer gets set up? Let's assume the top of the stack is a * return address; print it and skip past so _show_trace() doesn't print * it again. */ - else + if ( !is_active_kernel_text(regs->rip) || + is_active_kernel_text(tos) ) { - printk(" [<%p>] %pS\n", _p(tos), _p(tos)); + printk(" [<%p>] ? %pS\n", _p(tos), _p(tos)); sp++; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack 2019-06-17 8:13 ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich @ 2019-07-03 10:21 ` Andrew Cooper 2019-07-03 10:34 ` Jan Beulich 0 siblings, 1 reply; 19+ messages in thread From: Andrew Cooper @ 2019-07-03 10:21 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: WeiLiu, Roger Pau Monne On 17/06/2019 09:13, Jan Beulich wrote: > Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, > often for relatively simple leaf functions. Actually, the problem is more widespread than this. For every function, there is a non-zero quantity of time between the function starting and the frame pointer being set up. However, half of this time is spent with the old %rbp on the top of the stack, so won't benefit from these changes. > (To give a specific example, > the case I've run into this with is _pci_hide_device() and gcc 8. > Interestingly the even more simple neighboring iommu_has_feature() does > get a frame pointer set up, around just a single instruction. But this > may be a result of the size-of-asm() effects discussed elsewhere.) > > Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. This far, I'm happy with. > Also annotate non-frame-pointer-based stack trace entries with a > question mark, to signal clearly that any one of them may not actually > be part of the call stack. I'm still opposed to this. The introduction of ? does more harm than good IMO, because it simply can't be trusted. Stack traces are not guaranteed-accurate, even with frame pointers enabled. The only thing we can say for certain in any trace is where %rip points. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack 2019-07-03 10:21 ` Andrew Cooper @ 2019-07-03 10:34 ` Jan Beulich 2019-07-03 19:47 ` Andrew Cooper 0 siblings, 1 reply; 19+ messages in thread From: Jan Beulich @ 2019-07-03 10:34 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, WeiLiu, Roger Pau Monne On 03.07.2019 12:21, Andrew Cooper wrote: > On 17/06/2019 09:13, Jan Beulich wrote: >> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, >> often for relatively simple leaf functions. > > Actually, the problem is more widespread than this. For every function, > there is a non-zero quantity of time between the function starting and > the frame pointer being set up. > > However, half of this time is spent with the old %rbp on the top of the > stack, so won't benefit from these changes. I think the compiler typically pairs push %rbp and mov %rsp, %rbp, but this pair may not sit at the beginning of the function. And it's that other code that's prone to crash. The push %rbp may also fault (most notably due to stack overrun), but that would then still have the top of stack covered by the change here. The mov %rsp, %rbp, otoh, won't plausibly fault. IOW I think it's far more than "half of the time" that this change helps. >> (To give a specific example, >> the case I've run into this with is _pci_hide_device() and gcc 8. >> Interestingly the even more simple neighboring iommu_has_feature() does >> get a frame pointer set up, around just a single instruction. But this >> may be a result of the size-of-asm() effects discussed elsewhere.) >> >> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. > > This far, I'm happy with. > >> Also annotate non-frame-pointer-based stack trace entries with a >> question mark, to signal clearly that any one of them may not actually >> be part of the call stack. > > I'm still opposed to this. The introduction of ? does more harm than > good IMO, because it simply can't be trusted. > > Stack traces are not guaranteed-accurate, even with frame pointers > enabled. The only thing we can say for certain in any trace is where > %rip points. Yes, I realize you still don't like this. But similarly to the other patch set - on the v1 discussion here I was lacking feedback, and hence I eventually timed out and sent v2. The question is - what is your alternative proposal to distinguish the truly guessed entry logged here from the more reliable ones? And then similarly how to distinguish the less reliable ones produced by the !CONFIG_FRAME_POINTER variant of _show_trace() from their more reliable counterparts? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack 2019-07-03 10:34 ` Jan Beulich @ 2019-07-03 19:47 ` Andrew Cooper 2019-07-04 9:09 ` Jan Beulich 0 siblings, 1 reply; 19+ messages in thread From: Andrew Cooper @ 2019-07-03 19:47 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, WeiLiu, Roger Pau Monne On 03/07/2019 11:34, Jan Beulich wrote: > On 03.07.2019 12:21, Andrew Cooper wrote: >> On 17/06/2019 09:13, Jan Beulich wrote: >>> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer, >>> often for relatively simple leaf functions. >> Actually, the problem is more widespread than this. For every function, >> there is a non-zero quantity of time between the function starting and >> the frame pointer being set up. >> >> However, half of this time is spent with the old %rbp on the top of the >> stack, so won't benefit from these changes. > I think the compiler typically pairs push %rbp and mov %rsp, %rbp, > but this pair may not sit at the beginning of the function. And it's > that other code that's prone to crash. The push %rbp may also fault > (most notably due to stack overrun), but that would then still have > the top of stack covered by the change here. The mov %rsp, %rbp, > otoh, won't plausibly fault. IOW I think it's far more than "half of > the time" that this change helps. My statement wasn't meant as a criticism, but more of an observation. > >>> (To give a specific example, >>> the case I've run into this with is _pci_hide_device() and gcc 8. >>> Interestingly the even more simple neighboring iommu_has_feature() does >>> get a frame pointer set up, around just a single instruction. But this >>> may be a result of the size-of-asm() effects discussed elsewhere.) >>> >>> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid. >> This far, I'm happy with. >> >>> Also annotate non-frame-pointer-based stack trace entries with a >>> question mark, to signal clearly that any one of them may not actually >>> be part of the call stack. >> I'm still opposed to this. The introduction of ? does more harm than >> good IMO, because it simply can't be trusted. >> >> Stack traces are not guaranteed-accurate, even with frame pointers >> enabled. The only thing we can say for certain in any trace is where >> %rip points. > Yes, I realize you still don't like this. But similarly to the > other patch set - on the v1 discussion here I was lacking > feedback, and hence I eventually timed out and sent v2. The > question is - what is your alternative proposal to distinguish > the truly guessed entry logged here from the more reliable > ones? And then similarly how to distinguish the less reliable > ones produced by the !CONFIG_FRAME_POINTER variant of > _show_trace() from their more reliable counterparts? A crazy idea I've just had. Annotate all printed lines with a character identifying which source of information we used? We could have [r] for register state, [f] for "from frame pointer", and [s] for "from stack rubble". ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack 2019-07-03 19:47 ` Andrew Cooper @ 2019-07-04 9:09 ` Jan Beulich 0 siblings, 0 replies; 19+ messages in thread From: Jan Beulich @ 2019-07-04 9:09 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, WeiLiu, Roger Pau Monne On 03.07.2019 21:47, Andrew Cooper wrote: > On 03/07/2019 11:34, Jan Beulich wrote: >> On 03.07.2019 12:21, Andrew Cooper wrote: >>> I'm still opposed to this. The introduction of ? does more harm than >>> good IMO, because it simply can't be trusted. >>> >>> Stack traces are not guaranteed-accurate, even with frame pointers >>> enabled. The only thing we can say for certain in any trace is where >>> %rip points. >> Yes, I realize you still don't like this. But similarly to the >> other patch set - on the v1 discussion here I was lacking >> feedback, and hence I eventually timed out and sent v2. The >> question is - what is your alternative proposal to distinguish >> the truly guessed entry logged here from the more reliable >> ones? And then similarly how to distinguish the less reliable >> ones produced by the !CONFIG_FRAME_POINTER variant of >> _show_trace() from their more reliable counterparts? > > A crazy idea I've just had. Annotate all printed lines with a character > identifying which source of information we used? > > We could have [r] for register state, [f] for "from frame pointer", and > [s] for "from stack rubble". I'm fine with the fundamental idea, but I'm not overly happy with the second pair of (square) brackets that would appear. Two variants of what your proposal come to mind: 1) Use (like I did) '?' for "stack rubble" (as you call it), '*' for frame pointer based entries, and '!' for register ones. 2) Instead of the extra brackets, prefix a character along of what you've suggested (I'd use upper case ones though) immediately inside the already present brackets, followed e.g. by a colon as separator. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-07-04 9:13 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-31 8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 2019-05-31 8:59 ` [Xen-devel] " Jan Beulich 2019-05-31 9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich 2019-05-31 9:17 ` [Xen-devel] " Jan Beulich 2019-06-07 17:51 ` Andrew Cooper 2019-06-11 9:57 ` Jan Beulich 2019-05-31 9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 2019-05-31 9:22 ` [Xen-devel] " Jan Beulich 2019-06-07 18:01 ` Andrew Cooper 2019-06-11 9:46 ` Jan Beulich 2019-06-17 8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich 2019-06-17 8:12 ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich 2019-07-02 17:47 ` Andrew Cooper 2019-07-03 7:10 ` Jan Beulich 2019-06-17 8:13 ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich 2019-07-03 10:21 ` Andrew Cooper 2019-07-03 10:34 ` Jan Beulich 2019-07-03 19:47 ` Andrew Cooper 2019-07-04 9:09 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).