Re: [PATCH] KVM: arm64: Stop the hyp trace clock worker on disable

[sashiko-bot@kernel.org]

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 <fuad.tabba@linux.dev>

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.

Fixes: b22888917fa41 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260720172923.E4F161F000E9@smtp.kernel.org/
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>

> diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> index 2411b4c32932c..27367843fd211 100644
> --- a/arch/arm64/kvm/hyp_trace.c
> +++ b/arch/arm64/kvm/hyp_trace.c
> @@ -116,6 +116,7 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
>  	if (!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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720184850.1826630-1-fuad.tabba@linux.dev?part=1