Re: [PATCH 01/17] KVM: pfncache: Use atomic SRCU for readers instead of a rwlock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] kvm: concurrent MMU notifiers skip SRCU wait causing Use-After-Free
- [High] kvm: mmu_notifier_invalidate_range_start unconditionally sleeps, breaking OOM reaper
- [Low] kvm: compilation failure due to fabricated atomic SRCU API
--

commit 47eed0ef4a708a1117920edb2f9b64f3a7b2a8d4
Author: David Woodhouse <dwmw@xxxxxxxxxxxx>
KVM: pfncache: Use atomic SRCU for readers instead of a rwlock

This commit replaces the per-cache rwlock with a dedicated per-VM atomic
SRCU domain (kvm->gpc_srcu) for the read side in the KVM pfncache. Readers
now run under an atomic SRCU read lock, allowing them to execute in hardirq
and sched-out paths without spinning or failing.

Reported-by: syzbot+208f7f3e5f59c11aeb90@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=208f7f3e5f59c11aeb90
Reported-by: syzbot+919877893c9d28162dc2@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=919877893c9d28162dc2

> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -635,6 +635,13 @@ DEFINE_LOCK_GUARD_1(srcu, struct srcu_struct,
>  DECLARE_LOCK_GUARD_1_ATTRS(srcu, __acquires_shared(_T), __releases_shared(*(struct srcu_struct **)_T))
>  #define class_srcu_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(srcu, _T)
>  
> +DEFINE_LOCK_GUARD_1(srcu_atomic, struct srcu_struct,
> +		    _T->idx = srcu_read_lock_atomic(_T->lock),
> +		    srcu_read_unlock_atomic(_T->lock, _T->idx),
> +		    int idx)
> +DECLARE_LOCK_GUARD_1_ATTRS(srcu_atomic, __acquires_shared(_T), __releases_shared(*(struct srcu_struct **)_T))

[Severity: Low]
Does this code rely on a fabricated atomic SRCU API? 

The functions srcu_read_lock_atomic() and srcu_read_unlock_atomic() do not
exist in the Linux kernel tree, which will cause a build failure when
compiling.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1152,6 +1152,8 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>  		goto out_err_no_srcu;
>  	if (init_srcu_struct(&kvm->irq_srcu))
>  		goto out_err_no_irq_srcu;
> +	if (init_srcu_struct_atomic(&kvm->gpc_srcu))
> +		goto out_err_no_gpc_srcu;

[Severity: Low]
Is init_srcu_struct_atomic() a valid function? 

Using a non-existent API here will also result in a compilation error.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1719,18 +1719,18 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
>  {
>  	struct pvclock_vcpu_time_info *guest_hv_clock;
>  	struct pvclock_vcpu_time_info hv_clock;
> -	unsigned long flags;
> +	int idx;
>  
>  	memcpy(&hv_clock, ref_hv_clock, sizeof(hv_clock));
>  
> -	read_lock_irqsave(&gpc->lock, flags);
> +	idx = srcu_read_lock_atomic(&vcpu->kvm->gpc_srcu);

[Severity: Low]
Could this use of srcu_read_lock_atomic() break the build since the function
does not exist?

> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -26,35 +26,115 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,

[ ... ]

>  		unsigned long gpc_uhva;
>  		int old, new;
>  
> -		read_lock_irq(&gpc->lock);
> +		old = atomic_read_acquire(&gpc->state);
> +		if (!(old & (GPC_VALID | GPC_BECOMING_VALID)))
> +			continue;

[Severity: Critical]
Can this condition cause a use-after-free when concurrent MMU notifiers run?

If Thread A clears GPC_VALID (setting GPC_INVALIDATING), drops gpc_lock, and
blocks in synchronize_srcu_expedited(), a concurrent Thread B could read
gpc->state as GPC_INVALIDATING. 

Thread B would hit this check, skip the cache, and return immediately without
waiting for its own grace period. 

The core MM could then free the physical page while Thread A is still waiting
for KVM readers that are actively dereferencing the freed page.

[ ... ]

> +	 * (The one allocation in the SRCU grace-period machinery itself —
> +	 * the srcu_node tree on the one-time upgrade of a contended
> +	 * srcu_struct — is dealt with separately, by having srcu_gp_end()
> +	 * allocate with GFP_NOWAIT and fall back to a preallocated spare,
> +	 * so the grace period never blocks in reclaim either.)
> +	 */
> +	if (cleared)
> +		synchronize_srcu_expedited(&kvm->gpc_srcu);
> +
> +	/*

[Severity: High]
Does this code unconditionally sleep inside an MMU notifier callback?

The OOM reaper sets range->may_block to false and must not be blocked. Calling
synchronize_srcu_expedited() here violates that constraint and could lead to
deadlocks if the OOM reaper blocks on resources held by the dying process.

[ ... ]

> +	 * GPC_VALID precisely so that this path knows those readers may
> +	 * exist. Our own grace-period wait below necessarily begins after
> +	 * that clear, so waiting here covers them; only then may
> +	 * reader-visible fields (uhva, gpa, memslot, and later pfn/khva) be
> +	 * mutated or the old mapping torn down.
> +	 */
> +	must_drain = atomic_xchg(&gpc->state, 0) &
> +		     (GPC_VALID | GPC_INVALIDATING);
> +	if (must_drain)
> +		synchronize_srcu_expedited(&gpc->kvm->gpc_srcu);
>  
>  	old_pfn = gpc->pfn;

[Severity: High]
Are we using a sleeping synchronization function in the mutator path?

This appears to contradict the commit message's claim that readers are waited
on by a spinning atomic domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920211920.928306-1-dwmw2@xxxxxxxxxxxxx?part=1