Re: [PATCH v6 13/21] KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The guest physical address (GPA) of the SCB is clobbered before unpinning, causing KVM to lose dirty page tracking and corrupt nested guest state during live migration.
--

commit d9924cf112f75fbd55bca14e9ac133f8ed7fdde0
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

This patch changes the original SIE control block (SCB) to be lazily unpinned
only on reuse for a different SCB. This bounds the number of pinned SCBs by
the number of vCPUs and reduces memory pressure while extending pin and unpin
methods to track status.

> diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
> index c42e2df4c0abd..cdce4b3b25251 100644
> --- a/arch/s390/kvm/s390/vsie.c
> +++ b/arch/s390/kvm/s390/vsie.c
[ ... ]
> @@ -774,14 +775,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 *kvm, 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(kvm, gpa, hpa);
> +		unpin_guest_page(kvm, vsie_page->scb_gpa, hpa);

[Severity: High]
Does this code rely on scb_gpa to correctly mark the page dirty?

When reusing a vsie_page for a different SCB in get_vsie_page(), scb_gpa is
set to ULONG_MAX before unpin_scb() is called. This causes unpin_guest_page()
to fail to find a valid memslot and silently drops the dirty page log. Can
this result in stale nested guest state being migrated during live migration?

>  	vsie_page->scb_o = NULL;
> +	clear_bit(VSIE_PAGE_SCB_PINNED, &vsie_page->flags);
>  }
[ ... ]
> @@ -1575,51 +1608,79 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
[ ... ]
>  	} else {
>  		/* reuse an existing entry that belongs to nobody */
>  		while (true) {
> -			vsie_page = kvm->arch.vsie.pages[kvm->arch.vsie.next];
> +			pages_idx = kvm->arch.vsie.next;
> +			kvm->arch.vsie.next++;
> +			kvm->arch.vsie.next %= kvm->arch.vsie.page_count;
> +			vsie_page = kvm->arch.vsie.pages[pages_idx];
>  			if (try_get_vsie_page(vsie_page))
>  				break;
> -			kvm->arch.vsie.next++;
> -			kvm->arch.vsie.next %= nr_vcpus;
>  		}
>  		if (vsie_page->scb_gpa != ULONG_MAX)
>  			xa_erase(&kvm->arch.vsie.addr_to_page,
>  				 vsie_page->scb_gpa >> SCB_ALIGNMENT_SHIFT);
>  		/* Mark it as invalid until it resides in the tree. */
>  		vsie_page->scb_gpa = ULONG_MAX;
> +
> +		unpin_scb(kvm, vsie_page);

[Severity: High]
Since unpin_scb() now relies on vsie_page->scb_gpa to mark the page dirty,
but we just set scb_gpa to ULONG_MAX immediately above, will the page actually
be marked dirty correctly?

>  	}
>  
> -	/* Double use of the same address or allocation failure. */
> -	if (xa_insert(&kvm->arch.vsie.addr_to_page, addr >> SCB_ALIGNMENT_SHIFT, vsie_page,
> -		      GFP_KERNEL_ACCOUNT)) {
> -		put_vsie_page(vsie_page);
> -		mutex_unlock(&kvm->arch.vsie.mutex);
> -		return NULL;
> +	rc = init_vsie_page(vcpu, vsie_page, addr);
> +	if (!rc) {
> +		rc = xa_insert(&kvm->arch.vsie.addr_to_page, addr >> SCB_ALIGNMENT_SHIFT, vsie_page,
> +			       GFP_KERNEL_ACCOUNT);
> +		if (rc == -EBUSY)
> +			rc = -EAGAIN;
>  	}
> -	vsie_page->scb_gpa = addr;
> +
>  	mutex_unlock(&kvm->arch.vsie.mutex);
> +	if (vsie_page_new)
> +		free_vsie_page(vsie_page_new);
> +	if (rc) {
> +		vsie_page->scb_gpa = ULONG_MAX;
> +		put_vsie_page(vsie_page);
> +		return rc;

[Severity: High]
In this error path, the page is left pinned but its GPA is wiped. Doesn't
this mean the next time the page is reused or destroyed, it will suffer from
the same missing dirty page tracking failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-vsie-sigpi-v6-0-8020bb53be52@linux.ibm.com?part=13