Re: [PATCH 01/15] vdso: Move the kernel-side time update helpers to a new header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The kerneldoc for `vdso_time_update_begin()` still references the old function name `vdso_update_end()` instead of `vdso_time_update_end()`.

Pre-existing issues:
- [High] kvm_clock_sync() accesses KVM's vm_list locklessly during stop_machine(), which can race with VM creation/destruction and traverse a corrupted list.
--

commit a658f5b7a653ebec860c6e97f450e6c3c3e5c2a1
Author: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

vdso: Move the kernel-side time update helpers to a new header

The vdso/ header namespace is meant for symbols which are usable from
vDSO userspace code. However vdso_update_begin() and vdso_update_end()
are only usable from the kernel. Move them into a new header in the
linux/ namespace. Also rename them to make clear that they are only
for time-related data.

> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 2b989bebd220a..161628f6906fe 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -527,7 +528,7 @@ static int stp_sync_clock(void *data)
>  			cpu_relax();
>  		rc = 0;
>  		if (stp_info.todoff || stp_info.tmd != 2) {
> -			flags = vdso_update_begin();
> +			flags = vdso_time_update_begin();
>  			rc = chsc_sstpc(stp_page, STP_OP_SYNC, 0,
>  					&clock_delta);
>  			if (rc == 0) {
>  				sync->clock_delta = clock_delta;
>  				clock_sync_global(clock_delta);

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing severe
issue. Does stp_sync_clock() running via stop_machine() race with VM
creation or destruction and lead to list corruption?

If a task is preempted while modifying vm_list under kvm_lock during VM
creation or destruction (since kvm_lock is a mutex and leaves preemption
enabled), the list pointers could be left in an inconsistent state.

Because stp_sync_clock() runs via stop_machine(), it can preempt such tasks.
stp_sync_clock() calls clock_sync_global(), which triggers the epoch notifier
and causes kvm_clock_sync() to blindly traverse the corrupted vm_list
locklessly:

arch/s390/kvm/s390.c:kvm_clock_sync() {
    ...
    list_for_each_entry(kvm, &vm_list, vm_list) {
    ...
}

Can this lead to a host kernel panic or an infinite loop during lockless
traversal?

>  				rc = __store_stpinfo();
>  				if (rc == 0 && stp_info.tmd != 2)
>  					rc = -EAGAIN;
>  			}
> -			vdso_update_end(flags);
> +			vdso_time_update_end(flags);
>  		}
>  		sync->in_sync = rc ? -EAGAIN : 1;
>  		xchg(&first, 0);

[ ... ]

> diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
> index aa59919b8f2c2..12e205d86e7f3 100644
> --- a/kernel/time/vsyscall.c
> +++ b/kernel/time/vsyscall.c
> @@ -177,7 +178,7 @@ void vdso_time_update_aux(struct timekeeper *tk)
>  #endif
>  
>  /**
> - * vdso_update_begin - Start of a VDSO update section
> + * vdso_time_update_begin - Start of a VDSO update section
>   *
>   * Allows architecture code to safely update the architecture specific VDSO
>   * data. Disables interrupts, acquires timekeeper lock to serialize against
> @@ -188,7 +189,7 @@ void vdso_time_update_aux(struct timekeeper *tk)
>   * Returns: Saved interrupt flags which need to be handed in to
>   * vdso_update_end().
     ^^^^^^^^^^^^^^^
>   */
> -unsigned long vdso_update_begin(void)
> +unsigned long vdso_time_update_begin(void)

[Severity: Low]
Should the kerneldoc for vdso_time_update_begin() be updated to reference
vdso_time_update_end() instead of vdso_update_end()?

>  {
>  	struct vdso_time_data *vdata = vdso_k_time_data;
>  	unsigned long flags = timekeeper_lock_irqsave();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-vdso-asm-generic-vsyscall-h-v1-0-63ae856cc659@linutronix.de?part=1