Re: [PATCH v2 9/9] KVM: s390: Fix _gaccess_shadow_fault()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Calling `gmap_link()` inside `_gaccess_do_shadow()` while holding `sg->parent->children_lock` causes a guaranteed lockdep splat and a recursive spinlock deadlock.
--

--- Patch [9]: [PATCH v2 9/9] KVM: s390: Fix _gaccess_shadow_fault() ---
commit 2773a2ded997afc2d9abf44ed48fd84a08d773f8
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix _gaccess_shadow_fault()

This commit addresses an issue where shadowed nested guest memory is not
present in the parent guest gmap, causing -ENOENT to be incorrectly
propagated to userspace. It fixes this by manually calling gmap_link()
on the missing memory of the nested guest.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1551,8 +1552,17 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
>  			return -EAGAIN;
>  	}
>  
> +retry:
>  	rc = dat_entry_walk(NULL, entries[LEVEL_MEM].gfn, sg->parent->asce, DAT_WALK_LEAF,
>  			    TABLE_TYPE_PAGE_TABLE, &host, &ptep_h);
> +	if (rc == -ENOENT) {
> +		slot = gfn_to_memslot(sg->kvm, entries[LEVEL_MEM].gfn);
> +		if (!slot)
> +			return PGM_ADDRESSING;
> +		rc = gmap_link(mc, sg->parent, entries + LEVEL_MEM, slot);

[Severity: Critical]
Does this introduce a recursive spinlock deadlock?

Earlier in _gaccess_do_shadow(), we enforce that the parent's children_lock
is already held:

	lockdep_assert_held(&sg->parent->children_lock);

However, the new call to gmap_link() passes sg->parent as the gmap argument.
If gmap_link() eventually triggers an unshadow event, the call chain reaches
gmap_handle_vsie_unshadow_event(), which attempts to unconditionally acquire
the same lock:

	scoped_guard(spinlock, &parent->children_lock)
		_gmap_handle_vsie_unshadow_event(parent, gfn);

Additionally, gmap_link() can lead to _gmap_ptep_xchg() which explicitly
asserts that the children_lock is not held:

	else
		lockdep_assert_not_held(&gmap->children_lock);

Could a nested guest trigger this missing mapping path and cause a host
kernel hang?

> +		if (!rc)
> +			goto retry;
> +	}
>  	if (rc)
>  		return rc;
>

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