Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unconditional NULL pointer dereference in ACPI suspend/resume and kexec paths due to nullified sched_clock save/restore hooks for VMware and Xen guests. --
commit 812ddcb2484da2fae1600ef7146674adbef2e760 Author: Sean Christopherson <[email protected]> x86/paravirt: Pass sched_clock save/restore helpers during registration Pass in a PV clock's save/restore helpers when configuring sched_clock instead of relying on each PV clock to manually set the save/restore hooks. In addition to bringing sanity to the code, this will allow gracefully "rejecting" a PV sched_clock, e.g. when running as a CoCo guest that has access to a "secure" TSC. No functional change intended. > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c > index b5cb66ca022b8..968de002975f9 100644 > --- a/arch/x86/kernel/cpu/vmware.c > +++ b/arch/x86/kernel/cpu/vmware.c > @@ -347,11 +347,8 @@ static void __init vmware_paravirt_ops_setup(void) > > vmware_cyc2ns_setup(); > > - if (vmw_sched_clock) { > - paravirt_set_sched_clock(vmware_sched_clock); > - x86_platform.save_sched_clock_state = NULL; > - x86_platform.restore_sched_clock_state = NULL; > - } > + if (vmw_sched_clock) > + paravirt_set_sched_clock(vmware_sched_clock, NULL, NULL); [ ... ] > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c > index b14c4ada89a3f..0114c63dfdd9a 100644 > --- a/arch/x86/kernel/tsc.c > +++ b/arch/x86/kernel/tsc.c > @@ -280,12 +280,15 @@ bool using_native_sched_clock(void) > return static_call_query(pv_sched_clock) == native_sched_clock; > } > > -void __paravirt_set_sched_clock(u64 (*func)(void), bool stable) > +void __paravirt_set_sched_clock(u64 (*func)(void), bool stable, > + void (*save)(void), void (*restore)(void)) > { > if (!stable) > clear_sched_clock_stable(); > > static_call_update(pv_sched_clock, func); > + x86_platform.save_sched_clock_state = save; > + x86_platform.restore_sched_clock_state = restore; Does assigning these function pointers without a null check cause a null pointer dereference during ACPI suspend/resume or kexec paths for VMware and Xen guests? When VMware and Xen pass NULL for these hooks (as formalized by this patch and introduced in an earlier patch in this series), they are stored directly in x86_platform. Then, unconditionally in arch/x86/power/cpu.c:save_processor_state(): void save_processor_state(void) { __save_processor_state(&saved_context); x86_platform.save_sched_clock_state(); } and similarly in __restore_processor_state(): x86_platform.restore_sched_clock_state(); Will these calls jump to a null pointer since the hooks are now NULL? Before this patch series, these platforms relied on the default tsc_save_sched_clock_state hooks, which safely returned early. Setting the hooks to NULL appears to convert this safe no-op into a regression that triggers a panic. Should __paravirt_set_sched_clock() substitute a dummy no-op function when NULL is passed, or should the caller sites in arch/x86/power/cpu.c explicitly check if the hooks are set before invocation? > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
