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

[Vincent Donnefort <vdonnefort@google.com>]

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