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

[sashiko-bot@kernel.org]

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