| Message ID | 5D074BA30200007800238B6C@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : x86/traps: improve show_trace()'s top-of-stack handling | expand |
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
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
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
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
--- 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++; }
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.