Re: [PATCH 12/21] KVM: s390: vsie: Replace radix_tree with xarray addr_to_page

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional overwrite and logic error in `get_vsie_page` due to `xa_store()` misuse.
- [Medium] Missing `xa_destroy()` causes memory leak of XArray internal nodes during VM teardown.
- [Medium] Incorrect XArray initialization drops memcg accounting and adds free-slot tracking overhead.
--

--- Patch [12]: [PATCH 12/21] KVM: s390: vsie: Replace radix_tree with xarray addr_to_page ---
commit b9607cfaff7f6d624df41b31c8e80cc0a315a2e3
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Replace radix_tree with xarray addr_to_page

This commit replaces the radix_tree implementation in the s390 vsie with
an xarray for addr_to_page. Changing to the xarray API directly relaxes some
of the locking obligations and provides more symmetry with the next vsie
changes in the series.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 4090b4019f215..1734215ae73a9 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
[ ... ]
> @@ -1574,14 +1569,13 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
>  			kvm->arch.vsie.next %= nr_vcpus;
>  		}
>  		if (vsie_page->scb_gpa != ULONG_MAX)
> -			radix_tree_delete(&kvm->arch.vsie.addr_to_page,
> -					  vsie_page->scb_gpa >> 9);
> +			xa_erase(&kvm->arch.vsie.addr_to_page, vsie_page->scb_gpa >> 9);
>  		/* Mark it as invalid until it resides in the tree. */
>  		vsie_page->scb_gpa = ULONG_MAX;
>  	}
>  
>  	/* Double use of the same address or allocation failure. */
> -	if (radix_tree_insert(&kvm->arch.vsie.addr_to_page, addr >> 9, vsie_page)) {
> +	if (xa_store(&kvm->arch.vsie.addr_to_page, addr >> 9, vsie_page, GFP_KERNEL)) {

[Severity: High]
Does this unintentionally change the error handling semantics by using
xa_store() instead of xa_insert()?

The original radix_tree_insert() correctly refused to overwrite existing
entries and returned an error (e.g., -EEXIST). However, xa_store()
unconditionally overwrites the entry and returns the previous pointer or an
error pointer on failure.

If an entry already exists, xa_store() returns the old vsie_page pointer,
which evaluates to true. The code incorrectly treats this as an error, calling
put_vsie_page() on the newly inserted page while returning NULL. The new page
remains in the xarray but its scb_gpa is left as ULONG_MAX, breaking lookups
for this address.

Also, should this be using GFP_KERNEL_ACCOUNT to maintain the accounting
properties of the original code?

>  		put_vsie_page(vsie_page);
>  		mutex_unlock(&kvm->arch.vsie.mutex);
>  		return NULL;
[ ... ]
> @@ -1660,7 +1654,7 @@ int kvm_s390_handle_vsie(struct kvm_vcpu *vcpu)
>  void kvm_s390_vsie_init(struct kvm *kvm)
>  {
>  	mutex_init(&kvm->arch.vsie.mutex);
> -	INIT_RADIX_TREE(&kvm->arch.vsie.addr_to_page, GFP_KERNEL_ACCOUNT);
> +	xa_init_flags(&kvm->arch.vsie.addr_to_page, XA_FLAGS_ALLOC);

[Severity: Medium]
Should this use XA_FLAGS_ACCOUNT (or 0) instead of XA_FLAGS_ALLOC?

The original code initialized the radix tree with GFP_KERNEL_ACCOUNT, ensuring
that memory allocated for internal nodes was accounted to the guest's memory
cgroup. Initializing with XA_FLAGS_ALLOC and storing with GFP_KERNEL drops this
accounting.

Additionally, XA_FLAGS_ALLOC enables internal free-slot tracking
(XA_FLAGS_TRACK_FREE) which is typically meant for ID allocators (like
xa_alloc()). Using this on an xarray indexed by sparse physical addresses adds
unnecessary overhead.

>  }
>  
>  /* Destroy the vsie data structures. To be called when a vm is destroyed. */
> @@ -1676,10 +1670,8 @@ void kvm_s390_vsie_destroy(struct kvm *kvm)
>  			if (vsie_page->gmap_cache.gmap)
>  				release_gmap_shadow(vsie_page);
>  		kvm->arch.vsie.pages[i] = NULL;
> -		/* free the radix tree entry */
>  		if (vsie_page->scb_gpa != ULONG_MAX)
> -			radix_tree_delete(&kvm->arch.vsie.addr_to_page,
> -					  vsie_page->scb_gpa >> 9);
> +			xa_erase(&kvm->arch.vsie.addr_to_page, vsie_page->scb_gpa >> 9);
>  		free_vsie_page(vsie_page);
>  	}
>  	kvm->arch.vsie.page_count = 0;

[Severity: Medium]
Will this leak the xarray internal nodes when tearing down a VM?

If any page was left with scb_gpa == ULONG_MAX (e.g., due to the xa_store()
issue mentioned above), xa_erase() is skipped for that entry.

Because the VM destruction path never calls
xa_destroy(&kvm->arch.vsie.addr_to_page), the xarray internal nodes allocated
for any remaining entries will be permanently leaked. Should xa_destroy() be
called here to guarantee all internal memory is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-vsie-sigpi-v1-0-ea9f12066408@linux.ibm.com?part=12