[PATCH] KVM: arm64: Fix hyp_trace clock disabling

[PATCH] KVM: arm64: Fix hyp_trace clock disabling

[Vincent Donnefort]

Fix the disable path in hyp_trace_clock_enable(), which fell through to
re-initialize and reschedule the clock after cancelling the work. Return
early instead.

While at it, cleanup hyp_trace_clock::lock which is unused and
hyp_trace_clock::running which is redundant: the trace_remote framework
already serializes calls to the callback enable_tracing.

Fixes: b22888917fa4 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
index 2411b4c32932..0b03117a38ec 100644
--- a/arch/arm64/kvm/hyp_trace.c
+++ b/arch/arm64/kvm/hyp_trace.c
@@ -37,8 +37,6 @@ static struct hyp_trace_clock {
 	u32			shift;
 	struct delayed_work	work;
 	struct completion	ready;
-	struct mutex		lock;
-	bool			running;
 } hyp_clock;
 
 static void __hyp_clock_work(struct work_struct *work)
@@ -110,12 +108,9 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
 {
 	struct system_time_snapshot snap;
 
-	if (hyp_clock->running == enable)
-		return;
-
 	if (!enable) {
 		cancel_delayed_work_sync(&hyp_clock->work);
-		hyp_clock->running = false;
+		return;
 	}
 
 	ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
@@ -128,7 +123,6 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
 	INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);
 	schedule_delayed_work(&hyp_clock->work, msecs_to_jiffies(CLOCK_INIT_MS));
 	wait_for_completion(&hyp_clock->ready);
-	hyp_clock->running = true;
 }
 
 /* Access to this struct within the trace_remote_callbacks are protected by the trace_remote lock */

base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.55.0.141.g00534a21ce-goog

Re: [PATCH] KVM: arm64: Fix hyp_trace clock disabling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing cleanup in the error path of `hyp_trace_enable_tracing()` leaks the clock synchronization delayed work. Furthermore, this patch introduces a workqueue corruption if tracing is subsequently re-enabled.
--

--- Patch [1]: [PATCH] KVM: arm64: Fix hyp_trace clock disabling ---
commit c37a59b3ffe1e163385bedc8f21496d8af6fbc61
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Fix hyp_trace clock disabling

This commit fixes the disable path in hyp_trace_clock_enable() by returning
early instead of re-initializing the clock. It also cleans up the unused
lock and redundant running state from the clock structure.

> diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> index 2411b4c32932c..0b03117a38ecd 100644
> --- a/arch/arm64/kvm/hyp_trace.c
> +++ b/arch/arm64/kvm/hyp_trace.c
> @@ -37,8 +37,6 @@ static struct hyp_trace_clock {
>  	u32			shift;
>  	struct delayed_work	work;
>  	struct completion	ready;
> -	struct mutex		lock;
> -	bool			running;
>  } hyp_clock;

[ ... ]

> @@ -110,12 +108,9 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
>  {
>  	struct system_time_snapshot snap;
>  
> -	if (hyp_clock->running == enable)
> -		return;
> -
>  	if (!enable) {
>  		cancel_delayed_work_sync(&hyp_clock->work);
> -		hyp_clock->running = false;
> +		return;
>  	}
>  
>  	ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
> @@ -128,7 +123,6 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
>  	INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);

[Severity: Medium]
Is it safe to remove the hyp_clock->running check above, considering how
errors are handled in hyp_trace_enable_tracing()?

Looking at hyp_trace_enable_tracing(), if the hypercall fails, the error
is propagated but the clock is not stopped:

static int hyp_trace_enable_tracing(bool enable, void *priv)
{
	hyp_trace_clock_enable(&hyp_clock, enable);

	return kvm_call_hyp_nvhe(__tracing_enable, enable);
}

If the hypercall returns an error when enable is true, the user might
try to enable tracing again. This would result in a second call to
hyp_trace_clock_enable() with enable as true.

Without the running check returning early, the code will fall through and
call INIT_DELAYED_WORK() on the already active hyp_clock->work item. Can
this corrupt the workqueue list pointers?

Even though the hypercall might currently be prevented from failing due
to the remote lock serialization, should we add a rollback to the error
path in hyp_trace_enable_tracing() before dropping the running state check?

>  	schedule_delayed_work(&hyp_clock->work, msecs_to_jiffies(CLOCK_INIT_MS));
>  	wait_for_completion(&hyp_clock->ready);
> -	hyp_clock->running = true;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714181010.2527225-1-vdonnefort@google.com?part=1

Re: [PATCH] KVM: arm64: Fix hyp_trace clock disabling

[Vincent Donnefort]

On Tue, Jul 14, 2026 at 06:25:42PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Missing cleanup in the error path of `hyp_trace_enable_tracing()` leaks the clock synchronization delayed work. Furthermore, this patch introduces a workqueue corruption if tracing is subsequently re-enabled.
> --
> 
> --- Patch [1]: [PATCH] KVM: arm64: Fix hyp_trace clock disabling ---
> commit c37a59b3ffe1e163385bedc8f21496d8af6fbc61
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> KVM: arm64: Fix hyp_trace clock disabling
> 
> This commit fixes the disable path in hyp_trace_clock_enable() by returning
> early instead of re-initializing the clock. It also cleans up the unused
> lock and redundant running state from the clock structure.
> 
> > diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> > index 2411b4c32932c..0b03117a38ecd 100644
> > --- a/arch/arm64/kvm/hyp_trace.c
> > +++ b/arch/arm64/kvm/hyp_trace.c
> > @@ -37,8 +37,6 @@ static struct hyp_trace_clock {
> >  	u32			shift;
> >  	struct delayed_work	work;
> >  	struct completion	ready;
> > -	struct mutex		lock;
> > -	bool			running;
> >  } hyp_clock;
> 
> [ ... ]
> 
> > @@ -110,12 +108,9 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
> >  {
> >  	struct system_time_snapshot snap;
> >  
> > -	if (hyp_clock->running == enable)
> > -		return;
> > -
> >  	if (!enable) {
> >  		cancel_delayed_work_sync(&hyp_clock->work);
> > -		hyp_clock->running = false;
> > +		return;
> >  	}
> >  
> >  	ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
> > @@ -128,7 +123,6 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
> >  	INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);
> 
> [Severity: Medium]
> Is it safe to remove the hyp_clock->running check above, considering how
> errors are handled in hyp_trace_enable_tracing()?
> 
> Looking at hyp_trace_enable_tracing(), if the hypercall fails, the error
> is propagated but the clock is not stopped:
> 
> static int hyp_trace_enable_tracing(bool enable, void *priv)
> {
> 	hyp_trace_clock_enable(&hyp_clock, enable);
> 
> 	return kvm_call_hyp_nvhe(__tracing_enable, enable);
> }
> 
> If the hypercall returns an error when enable is true, the user might
> try to enable tracing again. This would result in a second call to
> hyp_trace_clock_enable() with enable as true.

Sigh... Even though I don't believe this HVC can fail, it returns an error
nonetheless and I should handle the rollback properly here!

> 
> Without the running check returning early, the code will fall through and
> call INIT_DELAYED_WORK() on the already active hyp_clock->work item. Can
> this corrupt the workqueue list pointers?
> 
> Even though the hypercall might currently be prevented from failing due
> to the remote lock serialization, should we add a rollback to the error
> path in hyp_trace_enable_tracing() before dropping the running state check?
> 
> >  	schedule_delayed_work(&hyp_clock->work, msecs_to_jiffies(CLOCK_INIT_MS));
> >  	wait_for_completion(&hyp_clock->ready);
> > -	hyp_clock->running = true;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260714181010.2527225-1-vdonnefort@google.com?part=1