Re: [PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The function `__get_kvmclock_master_clock()` is missing a `return true;` statement on its success path, causing it to return an undefined boolean value.
--

--- Patch [7]: [PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock() ---
commit cb25993776f1881f5ff66de261984bd8992ceece
Author: Sean Christopherson <[email protected]>

KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

This commit refactors the kvmclock retrieval logic by extracting the master
clock fallback out of the inner helper and changing it to return a boolean.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b520928579129..7366d4602f89f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1667,8 +1673,6 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  				   &hv_clock.tsc_shift,
>  				   &hv_clock.tsc_to_system_mul);
>  		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> -	} else {
> -		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
>  	}
>  }

[Severity: High]
Does this function need to return true on the success path?

Since __get_kvmclock_master_clock() was changed to return a bool, it returns
false on the early exit paths, but appears to fall off the end of the
function without explicitly returning a value on success.

Since the return value here is undefined, could it evaluate to false in the
caller? If so, get_kvmclock() would incorrectly trigger the fallback logic:

get_kvmclock()
{
        if (!__get_kvmclock_master_clock(kvm, data))
                data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
}

This could overwrite the accurately calculated data->clock with the base
offset calculation, while leaving data->flags populated with master clock
flags.

>  
> @@ -1678,8 +1682,11 @@ static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  	unsigned seq;
>  
>  	do {
> +		data->flags = 0;
> +
>  		seq = read_seqcount_begin(&ka->pvclock_sc);
> -		__get_kvmclock(kvm, data);
> +		if (!__get_kvmclock_master_clock(kvm, data))
> +			data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
>  	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7