Re: [PATCH] KVM: s390: gmap: protect entire guest top-level table when shadowing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Pre-existing AB-BA deadlock between the gmap `children_lock` and the `kvm->mmu_lock`.
--

commit adfa2afc574e2132990a75c0f57245c13f93a6d8
Author: Naveed Khan <naveed@digiscrypt.com>

KVM: s390: gmap: protect entire guest top-level table when shadowing

This commit updates the top-level guest table protection to use the table
length rather than the designation type. This ensures that the entire table
is write-protected, preventing nested guests from modifying writable
trailing pages and retaining stale translations.

> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -1263,7 +1263,7 @@ static int gmap_protect_asce_top_level(struct kvm_s390_mmu_cache *mc, struct gma
>  	/* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */
>  	smp_rmb();
>  
> -	rc = kvm_s390_get_guest_pages(sg->kvm, context.f, asce.rsto, asce.dt + 1, false);
> +	rc = kvm_s390_get_guest_pages(sg->kvm, context.f, asce.rsto, asce.tl + 1, false);
>  	if (rc > 0)
>  		rc = -EFAULT;
>  	if (!rc)
>  		rc = _gmap_protect_asce_top_level(mc, sg, &context);

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a pre-existing AB-BA
deadlock between the gmap children_lock and kvm->mmu_lock here?

When unshadowing an existing real-space mapping, gmap_create_shadow() takes
the locks in this order:

arch/s390/kvm/gmap.c:gmap_create_shadow() {
	scoped_guard(spinlock, &parent->children_lock) {
		...
		scoped_guard(write_lock, &parent->kvm->mmu_lock) {
		...
}

Concurrently, when creating a virtual-space shadow, gmap_create_shadow()
releases children_lock and calls gmap_protect_asce_top_level(), which
eventually calls __gmap_protect_asce_top_level() where the locks are
taken in the reverse order:

arch/s390/kvm/gmap.c:__gmap_protect_asce_top_level() {
	guard(write_lock)(&sg->kvm->mmu_lock);
	...
	scoped_guard(spinlock, &parent->children_lock) {
	...
}

Since sg->kvm is identically parent->kvm, could this cause a deadlock
if one VCPU creates a real-space shadow while another concurrently creates a
virtual-space shadow for the same parent gmap?

>  	if (rc)
>  		kvm_s390_release_faultin_array(sg->kvm, context.f, true);
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178223359030.58041.119573473883113694@digiscrypt.com?part=1