Re: [PATCH v5 4/4] x86: prefer shadow stack for producing call traces

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/11/2026 1:09 pm, Jan Beulich wrote:
> Shadow stacks contain little more than return addresses, and they in
> particular allow precise call traces also with FRAME_POINTER=n:
>
> (XEN) Xen call trace:
> (XEN)    [<ffff82d04032d730>] R extable.c#search_one_extable+0x70/0x73
> (XEN)    [<ffff82d04032d802>] C search_exception_table+0xc2/0x177
> (XEN)    [<ffff82d040358378>] C traps.c#extable_fixup.isra.0+0x18/0x6c
> (XEN)    [<ffff82d040358e3b>] C do_invalid_op+0xab/0x106
> (XEN)    [<ffff82d040201d98>] C 
> x86_64/entry.S#handle_exception_saved+0x88/0xf4
> (XEN)    [<ffff82d07fffe044>] E ffff82d07fffe044
> (XEN)    [<ffff82d040412db0>] C stub_selftest+0xd0/0x168
> (XEN)    [<ffff82d0403508d6>] C setup.c#init_done+0x116/0x15a
>
> as opposed to this counterpart (earlier during the same boot, before CET
> is enabled):
>
> (XEN) Xen call trace:
> (XEN)    [<ffff82d04032d730>] R extable.c#search_one_extable+0x70/0x73
> (XEN)    [<ffff82d04032d802>] S search_exception_table+0xc2/0x177
> (XEN)    [<ffff82d040358378>] S traps.c#extable_fixup.isra.0+0x18/0x6c
> (XEN)    [<ffff82d040358e3b>] S do_invalid_op+0xab/0x106
> (XEN)    [<ffff82d040201d98>] S 
> x86_64/entry.S#handle_exception_saved+0x88/0xf4
> (XEN)    [<ffff82d040412db0>] S stub_selftest+0xd0/0x168
> (XEN)    [<ffff82d0403d0cc9>] S do_initcalls+0x29/0x38
> (XEN)    [<ffff82d04041adf2>] S __start_xen+0x1c72/0x2235
> (XEN)    [<ffff82d040288a57>] S __high_start+0xb7/0xc0
>
> (note the entirely missing entry for the stub itself [1]; sadly there are
> no stray entries there).
>
> [1] Arguably we could teach FRAME_POINTER=n traces to recognize stubs as
>     well. But not FRAME_POINTER=y ones. In fact, what's missing there
>     isn't the stub itself, but (of course) its immediate caller.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Sorry, but this commit message is still deceptive to read.  Please
reformat it as:

---8<---
Shadow stacks contain little more than return addresses, and they in
particular allow precise call traces also with FRAME_POINTER=n:

Compare a WARN() in stub_selftest() which runs twice on boot, first
without Shadow Stacks:

    (XEN) Xen call trace:
    ...
    (XEN)    [<ffff82d040288a57>] S __high_start+0xb7/0xc0

and later with Shadow Stacks:

    (XEN) Xen call trace:
    ...
    (XEN)    [<ffff82d0403508d6>] C setup.c#init_done+0x116/0x15a


Note that ...
---8<---

This way you are presenting the information in chronological order, as
well as stating which has CET enabled and which does not.

We probably ought to teach the emulation stubs about
CONFIG_FRAME_POINTER.  It should be consistent 


> ---
> While the 'E' for exception frames is probably okay, I'm not overly
> happy with the 'C' (for CET). I would have preferred 'S' (for shadow),
> but we use that character already.
>
> As an alternative to suppressing output for the top level exception
> frame, adding the new code ahead of the 'R' output line (and then also
> ahead of the stack top read) could be considered.
>
> Quite likely a number of other uses of is_active_kernel_text() also want
> amending with in_stub().
> ---
> v3: Correct "link to other shadow stack" check. Don't log a line for the
>     (impossible) PV case. Add example stack trace to description.
> v2: IS_ENABLED() -> #ifdef. Re-base.
>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -48,6 +48,7 @@
>  #include <asm/shared.h>
>  #include <asm/shstk.h>
>  #include <asm/smp.h>
> +#include <asm/stubs.h>
>  #include <asm/system.h>
>  #include <asm/traps.h>
>  #include <asm/uaccess.h>
> @@ -705,6 +706,13 @@ unsigned long get_stack_dump_bottom(unsi
>      }
>  }
>  
> +#ifdef CONFIG_XEN_SHSTK
> +static bool in_stub(unsigned long addr)
> +{
> +    return !((this_cpu(stubs.addr) ^ addr) >> STUB_BUF_SHIFT);
> +}
> +#endif

static inline, or __maybe_unused.  This stub needs using elsewhere, and
has nothing to do with XEN_SHSTK beyond the first user.

> +
>  #if !defined(CONFIG_FRAME_POINTER)
>  
>  /*
> @@ -797,6 +805,49 @@ static void show_trace(const struct cpu_
>           !is_active_kernel_text(tos) )
>          printk("   [<%p>] R %pS\n", _p(regs->rip), _p(regs->rip));
>  
> +#ifdef CONFIG_XEN_SHSTK
> +    if ( rdssp() != SSP_NO_SHSTK )

Thinking about it, the entry condition wants to be regs->entry_ssp.

If shadow stacks where enabled at the time of taking the exception/etc,
that's good enough.

> +    {
> +        const unsigned long *ptr = _p(regs->entry_ssp);

Something Linux is gaining might be worth using here.

+struct fred_shstk_entry_frame {
+    u64 ssef_ssp;
+    u64 ssef_lip;
+    u64 ssef_cs;
+};

(Names subject to improvement, especially as this is not actually FRED
related, although cs does get extended with SL and WFE when FRED is
enabled.)

The logic below will render incorrectly on the IST stacks, I think.

In those cases, we'll get an entry_ssp pointing at the IST shstk with {
ssp, lip, cs, (maybe) sup token }, where what we want to do for
rendering the dump is to follow ssp first to get to the outer context
and then walk up.  This is different from rendering regs->rsp which
starts from the outer context.


> +        unsigned int n;
> +
> +        for ( n = 0; (unsigned long)ptr & (PAGE_SIZE - sizeof(*ptr)); ++n )
> +        {
> +            unsigned long val = *ptr;
> +
> +            if ( is_active_kernel_text(val) || in_stub(val) )
> +            {
> +                /* Normal return address entry.  */
> +                printk("   [<%p>] C %pS\n", _p(val), _p(val));
> +                ++ptr;
> +            }
> +            else if ( !((val ^ (unsigned long)ptr) >>
> +                        (PAGE_SHIFT + STACK_ORDER)) )
> +            {
> +                if ( val & (sizeof(val) - 1) )
> +                {
> +                    /* Most likely a supervisor token. */
> +                    break;
> +                }

Supervisors tokens only exist in !opt_fred cases.  But I don't see how
this is a valid check for one.  You're looking for non-zero in the
bottom 3 bits, but that's potentially ambiguous with cs and fails to
spot a non-busy token.

The distinguishing characteristic of a supervisor token is that it is
it's own address, so check (val ^ ptr) >> 1.

> +
> +                /*
> +                 * Ought to be a hypervisor interruption frame.  But don't
> +                 * (re)log the current frame's %rip.
> +                 */
> +                if ( n || ptr[1] != regs->rip )
> +                    printk("   [<%p>] E %pS\n", _p(ptr[1]), _p(ptr[1]));

Ah - this n is the thing covering the fact that you didn't move to the
outer context originally.  Please do that and drop this.

If we're going to start using E here, we should make the FRAME_POINTER
side match.  When printing something we've identified as a stub, we
ought to say "in stub" rather than leaving it as a raw number.

Not for this patch, but it might be nice to identify EFI ranges too,
because that's the only other case where we commonly get raw numbers.

> +                ptr = _p(val);
> +            }
> +            else /* Bogus. */
> +                break;
> +        }
> +
> +        /* Fall back to legacy stack trace if nothing was logged at all. */
> +        if ( n )
> +            return;

This comment wants to be after the return, not before it.

~Andrew

> +    }
> +#endif /* CONFIG_XEN_SHSTK */
> +
>      if ( fault )
>      {
>          printk("   [Fault on access]\n");
>