Re: [PATCH v4 11/20] KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Ignored `xa_insert` errors in `vsie_page_init` break architectural double-use protection, cause XArray corruption, and expose a guest-triggerable host `WARN_ON_ONCE`.
- [High] Guest-triggerable `PGM_ADDRESSING` exceptions are incorrectly translated into a host `-ENOMEM` error, aborting the VM.
- [Medium] The intended lazy pinning optimization is completely defeated because the SCB is unconditionally unpinned at the end of every VSIE execution.
- [Medium] Non-atomic bitwise operations on `vsie_page->flags` violate documented atomic requirements and create data races.
- [Low] `SCB_ALIGNMENT_SHIFT` is not defined anywhere in the codebase, causing a build failure.
--

--- Patch [11]: [PATCH v4 11/20] KVM: s390: vsie: Lazily keep original scb pinned after vsie exit ---
commit 987277adb095f19cd7aeacd230e5c42d67de0027
Author: Christoph Schlameuss <[email protected]>
Subject: KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

This patch updates the nested virtualization code to cache the pinned
original SCB for a vsie_page. It introduces pinning state flags and delays
unpinning until the vsie_page is reused or the VM is destroyed.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index d2582519b3ecc..490c29b369cd0 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
[ ... ]
> @@ -760,14 +761,18 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  }
>  
>  /* unpin the scb provided by guest 2, marking it as dirty */
> -static void unpin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
> -		      gpa_t gpa)
> +static void unpin_scb(struct kvm *kvm, struct vsie_page *vsie_page)
>  {
> -	hpa_t hpa = virt_to_phys(vsie_page->scb_o);
> +	hpa_t hpa;
> +
> +	if (!test_bit(VSIE_PAGE_SCB_PINNED, &vsie_page->flags))
> +		return;
>  
> +	hpa = virt_to_phys(vsie_page->scb_o);
>  	if (hpa)
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
>  	vsie_page->scb_o = NULL;
> +	__clear_bit(VSIE_PAGE_SCB_PINNED, &vsie_page->flags);
>  }

[Severity: Medium]
Does this introduce a data race on the flags field? 

The vsie_page struct documentation states that flags must be modified
atomically. Since other threads might concurrently update the
VSIE_PAGE_IN_USE bit using atomic operations like test_and_set_bit() in
try_get_vsie_page(), using the non-atomic __clear_bit() here could
overwrite those concurrent changes.

>  /*
> @@ -776,19 +781,22 @@ static void unpin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
>   * Returns: - 0 if the scb was pinned.
>   *          - > 0 if control has to be given to guest 2
>   */
> -static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
> -		   gpa_t gpa)
> +static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	hpa_t hpa;
>  	int rc;
>  
> -	rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
> +	if (test_bit(VSIE_PAGE_SCB_PINNED, &vsie_page->flags))
> +		return 0;
> +
> +	rc = pin_guest_page(vcpu->kvm, vsie_page->scb_gpa, &hpa);
>  	if (rc) {
>  		rc = kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
>  		WARN_ON_ONCE(rc);
>  		return 1;
>  	}
>  	vsie_page->scb_o = phys_to_virt(hpa);
> +	__set_bit(VSIE_PAGE_SCB_PINNED, &vsie_page->flags);
>  	return 0;
>  }

[Severity: Medium]
Similar to the unpin_scb() function above, should this be an atomic
set_bit() to avoid racing with concurrent access to vsie_page->flags?

[ ... ]
> @@ -1529,17 +1537,45 @@ static struct vsie_page *alloc_vsie_page(struct kvm *kvm)
>  	return vsie_page;
>  }
>  
> +static int vsie_page_init(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page, unsigned long scb_gpa)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	int rc;
> +
> +	if (vsie_page->scb_gpa != ULONG_MAX)
> +		xa_erase(&kvm->arch.vsie.addr_to_page, vsie_page->scb_gpa >> SCB_ALIGNMENT_SHIFT);

[Severity: Low]
Is SCB_ALIGNMENT_SHIFT defined in this tree? 

It doesn't appear to be defined anywhere in the codebase, which would
cause a build failure here and in the xa_insert() and xa_load() calls
below.

> +	vsie_page->scb_gpa = scb_gpa;
> +	rc = pin_scb(vcpu, vsie_page);
> +	if (rc) {
> +		vsie_page->scb_gpa = ULONG_MAX;
> +		return -ENOMEM;
> +	}

[Severity: High]
When pin_scb() fails due to an invalid guest physical address, it
successfully queues a PGM_ADDRESSING exception for the guest and returns
1. 

By returning -ENOMEM here, are we incorrectly escalating a guest-level
addressing fault into a host-level memory allocation failure? This will
propagate up to the run loop and abort the VM instead of just delivering
the exception to the guest.

> +
> +	vsie_page->sca_gpa = read_scao(kvm, vsie_page->scb_o);
> +	WARN_ON_ONCE(xa_insert(&kvm->arch.vsie.addr_to_page, scb_gpa >> SCB_ALIGNMENT_SHIFT,
> +			       vsie_page, GFP_KERNEL_ACCOUNT));

[Severity: High]
Could ignoring the error from xa_insert() here bypass the architectural
double-use protection? 

If two nested vCPUs concurrently execute the SIE instruction with the same
guest SCB address, the second one will fail to insert. By wrapping this in
WARN_ON_ONCE and ignoring the failure, the guest can intentionally trigger
a host warning. 

Furthermore, the second vCPU proceeds with the duplicate vsie_page, and
when it is later reused, the xa_erase() at the top of vsie_page_init()
will erase the valid XArray entry belonging to the first vCPU.

> +
> +	return 0;
> +}
[ ... ]
> @@ -1552,53 +1588,69 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
[ ... ]
> +	rc = vsie_page_init(vcpu, vsie_page, addr);
> +	mutex_unlock(&kvm->arch.vsie.mutex);
> +	if (vsie_page_new)
> +		free_vsie_page(vsie_page_new);
> +	if (WARN_ON_ONCE(rc)) {
> +		unpin_scb(kvm, vsie_page);
> +		vsie_page->scb_gpa = ULONG_MAX;
>  		put_vsie_page(vsie_page);
> -		mutex_unlock(&kvm->arch.vsie.mutex);
> -		return NULL;
> +		return ERR_PTR(rc);
>  	}

[Severity: High]
Following up on the vsie_page_init() return value, this WARN_ON_ONCE will
be triggered if the guest provided an invalid SCB address. Does this
result in a guest triggerable host warning before returning the -ENOMEM
error?

[ ... ]
> @@ -1626,7 +1678,7 @@ int kvm_s390_handle_vsie(struct kvm_vcpu *vcpu)
[ ... ]
>  out_unshadow:
>  	unshadow_scb(vcpu, vsie_page);
>  out_unpin_scb:
> -	unpin_scb(vcpu, vsie_page, scb_addr);
> +	unpin_scb(vcpu->kvm, vsie_page);
>  out_put:
>  	put_vsie_page(vsie_page);

[Severity: Medium]
Will this unconditional call to unpin_scb() at the end of every nested SIE
execution defeat the lazy pinning optimization? 

Since every normal exit flows through out_unpin_scb, the
VSIE_PAGE_SCB_PINNED flag is cleared and the SCB is unpinned immediately,
bypassing the intended cache behavior.

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