| Message ID | 20210920172529.24932-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xen/trace: Fix leakage of uninitialised stack into the tracebuffer | expand |
Not a patch, but an RFC idea for one... It occurred to me that the cycles parameter from __trace_var() and friends is pointless, as the cycles bit is encoded in the top bit of the event field anyway, and passed in the adjacent parameter. Dropping the cycles parameter results in +85/-1029 (-944) net change. The common change in callers is e.g. from: lea 0x3c(%rsp),%rcx mov $0x4,%edx mov $0x1,%esi mov $0x10f002,%edi mov %ebp,0x3c(%rsp) callq ffff82d04022ea20 <__trace_var> to this: lea 0x3c(%rsp),%rdx mov $0x4,%esi mov $0x8010f002,%edi mov %ebp,0x3c(%rsp) callq ffff82d04022ea20 <__trace_var> Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a little bit unwieldy. Instead, I was thinking of implementing trace() (and a thin trace_time() wrapper) mirroring the "new API" in patch 14. Half of the trace_var() users should be __trace_var() already because of living inside a tb_init_done conditional, and the rest are actually opencoded TRACE() taking no extra data. Thoughts? ~Andrew
On 21.09.2021 22:08, Andrew Cooper wrote: > Not a patch, but an RFC idea for one... > > It occurred to me that the cycles parameter from __trace_var() and > friends is pointless, as the cycles bit is encoded in the top bit of the > event field anyway, and passed in the adjacent parameter. > > Dropping the cycles parameter results in +85/-1029 (-944) net change. > > The common change in callers is e.g. from: > > lea 0x3c(%rsp),%rcx > mov $0x4,%edx > mov $0x1,%esi > mov $0x10f002,%edi > mov %ebp,0x3c(%rsp) > callq ffff82d04022ea20 <__trace_var> > > to this: > > lea 0x3c(%rsp),%rdx > mov $0x4,%esi > mov $0x8010f002,%edi > mov %ebp,0x3c(%rsp) > callq ffff82d04022ea20 <__trace_var> > > > Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a > little bit unwieldy. > > Instead, I was thinking of implementing trace() (and a thin trace_time() > wrapper) mirroring the "new API" in patch 14. Half of the trace_var() > users should be __trace_var() already because of living inside a > tb_init_done conditional, and the rest are actually opencoded TRACE() > taking no extra data. > > Thoughts? Sounds quite reasonable to me. Jan
On 22/09/2021 08:03, Jan Beulich wrote: > On 21.09.2021 22:08, Andrew Cooper wrote: >> Not a patch, but an RFC idea for one... >> >> It occurred to me that the cycles parameter from __trace_var() and >> friends is pointless, as the cycles bit is encoded in the top bit of the >> event field anyway, and passed in the adjacent parameter. >> >> Dropping the cycles parameter results in +85/-1029 (-944) net change. >> >> The common change in callers is e.g. from: >> >> lea 0x3c(%rsp),%rcx >> mov $0x4,%edx >> mov $0x1,%esi >> mov $0x10f002,%edi >> mov %ebp,0x3c(%rsp) >> callq ffff82d04022ea20 <__trace_var> >> >> to this: >> >> lea 0x3c(%rsp),%rdx >> mov $0x4,%esi >> mov $0x8010f002,%edi >> mov %ebp,0x3c(%rsp) >> callq ffff82d04022ea20 <__trace_var> >> >> >> Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a >> little bit unwieldy. >> >> Instead, I was thinking of implementing trace() (and a thin trace_time() >> wrapper) mirroring the "new API" in patch 14. Half of the trace_var() >> users should be __trace_var() already because of living inside a >> tb_init_done conditional, and the rest are actually opencoded TRACE() >> taking no extra data. >> >> Thoughts? > Sounds quite reasonable to me. In which case, for v3 I'll reposition "new API" towards the beginning of the series, extend it with these two new APIs, then adjust all the cleanup patches to avoid double-patching most of the call-sites. ~Andrew
Patches 1-3 fix bugs causing uninitialised stack to leak into the trace buffers. Xentrace is a developer/debugging activity restricted to fully privileged entities, so the leaking of uninitialised stack contents is not a security concern here. Patches 4 and 5 are cleanup worthy of backporting, because their knock-on effects in release builds. Patches 6 and later are cleanup and probably not for backporting. They convert all trace records to using fixed types, and move some PV-specifics to only be built for PV && TRACEBUFFER, and removing stub files from all architectures. I have yet more cleanup in progress making most of the macros disappear, but this series is getting long enough already, (and taking time I don't really have). Andrew Cooper (12): xen/trace: Don't over-read trace objects xen/memory: Remove tail padding from TRC_MEM_* records xen/credit2: Remove tail padding from TRC_CSCHED2_* records x86/hvm: Reduce stack usage from HVMTRACE_ND() x86/hvm: Remove duplicate calls caused by tracing xen/credit2: Clean up trace handling xen/rt: Clean up trace handling xen/sched: Clean up trace handling xen/trace: Minor code cleanup x86/pv: Move x86/trace.c to x86/pv/trace.c xen/arch: Drop asm-*/trace.h x86/trace: Clean up trace handling tools/xentrace/formats | 4 + tools/xentrace/xenalyze.c | 12 +- xen/arch/x86/Makefile | 1 - xen/arch/x86/hvm/hpet.c | 15 +- xen/arch/x86/hvm/hvm.c | 5 +- xen/arch/x86/hvm/svm/svm.c | 8 +- xen/arch/x86/hvm/vlapic.c | 23 ++- xen/arch/x86/hvm/vmx/vmx.c | 9 +- xen/arch/x86/hvm/vpic.c | 9 +- xen/arch/x86/irq.c | 4 +- xen/arch/x86/mm/p2m-pod.c | 17 +- xen/arch/x86/mm/p2m-pt.c | 6 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/pv/Makefile | 1 + xen/arch/x86/pv/emul-inv-op.c | 2 +- xen/arch/x86/pv/emul-priv-op.c | 1 + xen/arch/x86/pv/ro-page-fault.c | 2 +- xen/arch/x86/pv/trace.c | 141 ++++++++++++++ xen/arch/x86/pv/traps.c | 2 +- xen/arch/x86/trace.c | 159 ---------------- xen/arch/x86/traps.c | 3 +- xen/common/memory.c | 4 +- xen/common/sched/core.c | 4 +- xen/common/sched/credit.c | 38 ++-- xen/common/sched/credit2.c | 344 +++++++++++++++++------------------ xen/common/sched/null.c | 42 +++-- xen/common/sched/rt.c | 121 ++++++------ xen/common/trace.c | 60 +++--- xen/include/asm-arm/trace.h | 12 -- xen/include/asm-x86/hvm/trace.h | 30 ++- xen/include/asm-x86/{ => pv}/trace.h | 8 +- xen/include/xen/trace.h | 13 +- 32 files changed, 537 insertions(+), 565 deletions(-) create mode 100644 xen/arch/x86/pv/trace.c delete mode 100644 xen/arch/x86/trace.c delete mode 100644 xen/include/asm-arm/trace.h rename xen/include/asm-x86/{ => pv}/trace.h (92%)