KVM: arm64: Stop the hyp trace clock worker on disable
Fuad Tabba
reviewerPATCH 1/1 KVM: arm64: Stop the hyp trace clock worker on disable
When disabling, hyp_trace_clock_enable() cancels the clock work but then falls through into the enable path, which re-schedules it and marks the clock running again, so the worker never stops. Return once the work is cancelled.
--- arch/arm64/kvm/hyp_trace.c | 1 + 1 file changed, 1 insertion(+)
arch/arm64/kvm/hyp_trace.c +1 -0
@@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enablif (!enable) {cancel_delayed_work_sync (&hyp_clock->work); hyp_clock->running = false; +return ; }ktime_get_snapshot_id (CLOCK_BOOTTIME , &snap);
base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
Signature · 1 line
2.39.5
Quoted 9 lines · Fuad Tabba
When disabling,
hyp_trace_clock_enable()cancels the clock work but then falls through into the enable path, which re-schedules it and marks the clock running again, so the worker never stops. Return once the work is cancelled.Fixes b22888917fa41 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")Reported-by Sashiko <[email protected]>Signed-off-by Fuad Tabba <[email protected]>
Done here already https://lore.kernel.org/all/[email protected]/
Quoted 21 lines
--- arch/arm64/kvm/hyp_trace.c | 1 + 1 file changed, 1 insertion(+)arch/arm64/kvm/hyp_trace.c +1
-0@@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enablif (!enable) {cancel_delayed_work_sync (&hyp_clock->work); hyp_clock->running = false; +return ; }ktime_get_snapshot_id (CLOCK_BOOTTIME , &snap);base-commit:
1590cf0329716306e948a8fc29f1d3ee87d3989fSignature · 1 line
2.39.5
Quoted 6 lines · Vincent Donnefort
Quoted 5 lines · Fuad Tabba
When disabling,
hyp_trace_clock_enable()cancels the clock work but then falls through into the enable path, which re-schedules it and marks the clock running again, so the worker never stops. Return once the work is cancelled.
Quoted 6 lines
Quoted 4 lines
Fixes b22888917fa41 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")Reported-by Sashiko <[email protected]>Signed-off-by Fuad Tabba <[email protected]>Done here already https://lore.kernel.org/all/[email protected]/
sorry I meant here:
https://lore.kernel.org/all/[email protected]/
Quoted 22 lines
Quoted 22 lines
--- arch/arm64/kvm/hyp_trace.c | 1 + 1 file changed, 1 insertion(+)arch/arm64/kvm/hyp_trace.c +1
-0@@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enablif (!enable) {cancel_delayed_work_sync (&hyp_clock->work); hyp_clock->running = false; +return ; }ktime_get_snapshot_id (CLOCK_BOOTTIME , &snap);base-commit:
1590cf0329716306e948a8fc29f1d3ee87d3989fSignature · 1 line
2.39.5
Fuad Tabba
reviewerQuoted 13 lines · Vincent Donnefort
Quoted 11 lines · Vincent Donnefort
Quoted 9 lines · Fuad Tabba
When disabling,
hyp_trace_clock_enable()cancels the clock work but then falls through into the enable path, which re-schedules it and marks the clock running again, so the worker never stops. Return once the work is cancelled.Fixes b22888917fa41 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")Reported-by Sashiko <[email protected]>Signed-off-by Fuad Tabba <[email protected]>
Quoted 5 lines
Quoted 1 line
Done here already https://lore.kernel.org/all/[email protected]/
sorry I meant here:
Thanks, and sorry for the noise!
/fuad
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
--- arch/arm64/kvm/hyp_trace.c | 1 + 1 file changed, 1 insertion(+)arch/arm64/kvm/hyp_trace.c +1
-0@@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enablif (!enable) {cancel_delayed_work_sync (&hyp_clock->work); hyp_clock->running = false; +return ; }ktime_get_snapshot_id (CLOCK_BOOTTIME , &snap);base-commit:
1590cf0329716306e948a8fc29f1d3ee87d3989f--
2.39.5
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Use of uninitialized stack memory due to missing snap.valid check
- [High] Kernel panics due to multiple missing zero-checks in clock math
- [Medium] Resource leak if hypercall fails in hyp_trace_enable_tracing
- [Medium] Repeated initialization of delayed_work and completion objects --
--- Patch [1]: [PATCH] KVM: arm64: Stop the hyp trace clock worker on disable --- commit 036d5fc621f0803d25d344f6b28a01f0246f7e82
Author: Fuad Tabba <[email protected]>
KVM: arm64: Stop the hyp trace clock worker on disable
When disabling, hyp_trace_clock_enable() cancels the clock work but then falls through into the enable path, which re-schedules it and marks the clock running again, so the worker never stops. Return once the work is cancelled.
arch/arm64/kvm/hyp_trace.c static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl · line 122
Quoted 12 lines
arch/arm64/kvm/hyp_trace.c +1
-0@@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enablif (!enable) {cancel_delayed_work_sync (&hyp_clock->work); hyp_clock->running = false; +return ; }ktime_get_snapshot_id (CLOCK_BOOTTIME , &snap);
[Severity: High]
This is a pre-existing issue, but the snapshot is read without checking if it is valid.
If timekeeping is suspended, ktime_get_snapshot_id() sets valid = false and returns without initializing systime or cycles.
Does this mean the kernel will consume uninitialized stack memory when accessing snap.systime and snap.cycles immediately after?
[Severity: Medium]
This is a pre-existing issue, but later in hyp_trace_clock_enable(), these initialization macros are called unconditionally:
init_completion(&hyp_clock->ready);
INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);Since these are called on a static global object on every enable cycle, does this violate the requirement to initialize such objects only once, potentially causing lockdep warnings or memory leaks?
[Severity: High]
This is a pre-existing issue, but there are multiple divisions in __hyp_clock_work() that do not check for zero:
ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
boot = ktime_to_ns(snap.systime);
...
rate = div64_u64(delta_cycles * NSEC_PER_SEC, delta_boot);
clocks_calc_mult_shift(&hyp_clock->mult, &hyp_clock->shift,
rate, NSEC_PER_SEC, CLOCK_MAX_CONVERSION_S);
/* Add a comfortable 50% margin */
hyp_clock->cyc_overflow64 = (U64_MAX / hyp_clock->mult) >> 1;First, as mentioned above, if ktime_get_snapshot_id() fails, the worker consumes the snapshot without checking validity.
If delta_boot becomes 0 (for example, from uninitialized stack memory, or time failing to advance), will div64_u64() panic?
Similarly, if delta_cycles is 0, rate becomes 0, which would cause clocks_calc_mult_shift() to execute a divide by zero. And if hyp_clock->mult becomes 0, the cyc_overflow64 calculation will panic. Could we add zero-checks here?
[Severity: Medium]
This is a pre-existing issue, but there might be a resource leak in hyp_trace_enable_tracing() if the hypercall fails:
hyp_trace_clock_enable(&hyp_clock, enable);
return kvm_call_hyp_nvhe(__tracing_enable, enable);The clock worker is started before the hypercall. If kvm_call_hyp_nvhe() fails when enabling tracing, the error is returned to the caller but the previously started delayed work is not stopped.
Does this mean the worker will continuously reschedule itself and leak CPU cycles since the error path does not clean it up?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1