Re: [PATCH v10 2/3] KVM: s390: Enable adapter_indicators_set to use mapped pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Host-userspace reachable WARN_ON_ONCE when pinning the summary page fails.
- [High] Broken atomicity: The indicator bit is set in guest memory before validating the summary page, leaving the guest in an inconsistent state on error.
- [High] The fast path for long-term mapped adapter indicators modifies guest memory without updating KVM's dirty page tracking, breaking live migration.
--

commit eab5dd8ef6ef5a366acb6ed92a72330259c39430
Author: Douglas Freimuth <freimuth@linux.ibm.com>
KVM: s390: Enable adapter_indicators_set to use mapped pages

This commit optimizes the s390 adapter_indicators_set function to use
long-term mapped pages when available. It introduces a fast path that operates
under a spinlock, falling back to the existing pin-and-map slow path if
the mapped pages are not found.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index d066a282271e2..b5304816aaa04 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
[ ... ]
> @@ -2892,41 +2892,80 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
[ ... ]
>  static int adapter_indicators_set(struct kvm *kvm,
>  				  struct s390_io_adapter *adapter,
>  				  struct kvm_s390_adapter_int *adapter_int)
>  {
[ ... ]
> +	spin_lock_irqsave(&adapter->maps_lock, flags);
> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> +	if (!ind_info) {
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +		ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
> +		if (!ind_page)
> +			return -1;
> +		idx = srcu_read_lock(&kvm->srcu);
> +		map = page_address(ind_page);
> +		bit = get_ind_bit(adapter_int->ind_addr,
> +				  adapter_int->ind_offset, adapter->swap);
> +		set_bit(bit, map);

[Severity: High]
If the indicator bit is set here, but the subsequent attempt to resolve or pin 
the summary page fails and the function returns -1, does this code leave the
indicator bit permanently set in guest memory?

Prior to this patch, both pages were successfully pinned before any bits were
mutated, ensuring an all-or-nothing update.

> +		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
> +		set_page_dirty_lock(ind_page);
> +		srcu_read_unlock(&kvm->srcu, idx);
> +		unpin_user_page(ind_page);
> +	} else {
> +		map = page_address(ind_info->page);
> +		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> +		set_bit(bit, map);
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);

[Severity: High]
By modifying the long-term mapped adapter indicator directly in the host 
virtual mapping, does this bypass KVM's dirty page tracking?

The slow path calls mark_page_dirty() and set_page_dirty_lock(), but the fast 
path skips them. Could this cause the migrated guest to lose interrupt 
delivery notifications if the updated indicator pages are not transferred to 
the destination VM?

> +	}
> +	spin_lock_irqsave(&adapter->maps_lock, flags);
> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> +	if (!summary_info) {
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
> +		if (WARN_ON_ONCE(!summary_page))
> +			return -1;

[Severity: High]
Since the summary address is derived from guest-controlled KVM routing entries 
and managed by host userspace, could a malicious or misbehaving userspace 
process trigger this warning by unmapping the memory or providing an invalid 
address?

The KVM subsystem guidelines note that asserts whose conditions can be driven 
by unprivileged host userspace are a host-side denial of service, especially 
if panic_on_warn is enabled. Is it better to just return -1 here without the 
warning?

> +		idx = srcu_read_lock(&kvm->srcu);
> +		map = page_address(summary_page);
> +		bit = get_ind_bit(adapter_int->summary_addr,
> +				  adapter_int->summary_offset, adapter->swap);
> +		summary_set = test_and_set_bit(bit, map);
> +		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> +		set_page_dirty_lock(summary_page);
> +		srcu_read_unlock(&kvm->srcu, idx);
> +		unpin_user_page(summary_page);
> +	} else {
> +		map = page_address(summary_info->page);
> +		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
> +				  adapter->swap);
> +		summary_set = test_and_set_bit(bit, map);
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);

[Severity: High]
Similar to the indicator page fast path above, does this code also bypass KVM
dirty page tracking for the summary page when a mapped info structure is found?

>  	}
>  
> -	idx = srcu_read_lock(&kvm->srcu);
> -	map = page_address(ind_page);
> -	bit = get_ind_bit(adapter_int->ind_addr,
> -			  adapter_int->ind_offset, adapter->swap);
> -	set_bit(bit, map);
> -	mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
> -	set_page_dirty_lock(ind_page);
> -	map = page_address(summary_page);
> -	bit = get_ind_bit(adapter_int->summary_addr,
> -			  adapter_int->summary_offset, adapter->swap);
> -	summary_set = test_and_set_bit(bit, map);
> -	mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> -	set_page_dirty_lock(summary_page);
> -	srcu_read_unlock(&kvm->srcu, idx);
> -
> -	unpin_user_page(ind_page);
> -	unpin_user_page(summary_page);
>  	return summary_set ? 0 : 1;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604192755.203143-1-freimuth@linux.ibm.com?part=2