Re: [GIT PULL v1 16/17] 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] Missing KVM dirty page tracking in fast path for mapped adapter indicators breaks live migration.
- [High] Atomicity violation when pinning the summary page fails leaves the guest hardware state inconsistent.
- [High] Host-side denial of service vulnerability via unprivileged userspace-reachable WARN_ON_ONCE.
--

commit 65f7550eaa09f7cd9feb26fff5319f4be6fecb0c
Author: Douglas Freimuth <freimuth@linux.ibm.com>

KVM: s390: Enable adapter_indicators_set to use mapped pages

This patch optimizes the s390 adapter_indicators_set function to use
long-term mapped pages when available. If pages are mapped, it processes
the interrupt on a fast path; otherwise, it falls back to the slow path
pinning.

Message-ID: <20260604192755.203143-3-freimuth@linux.ibm.com>

> 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)
>  {
>  	unsigned long bit;
>  	int summary_set, idx;
> -	struct page *ind_page, *summary_page;
> +	struct s390_map_info *ind_info, *summary_info;
>  	void *map;
> +	struct page *ind_page, *summary_page;
> +	unsigned long flags;
>  
> -	ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
> -	if (!ind_page)
> -		return -1;
> -	summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
> -	if (!summary_page) {
> +	ind_page = NULL;
> +
> +	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);
> +		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);
> -		return -1;
> +	} 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]
When updating the mapped indicator page in this fast path branch, does this
skip calling mark_page_dirty() and set_page_dirty_lock()? Could skipping
the dirty page logging cause the destination VM to resume with stale memory
state during live migration?

> +	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))

[Severity: High]
Since summary_addr is provided by userspace, can an invalid address
intentionally trigger this WARN_ON_ONCE()? Should this return an error
instead of asserting to avoid spamming the host kernel log?

> +			return -1;

[Severity: High]
If pin_map_page() fails for the summary page and returns -1 here, does this
leave the indicator bit already set in guest memory without its corresponding
summary bit? The original code pinned both pages before modifying memory.

> +		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 fast path, does this skip calling mark_page_dirty()
when updating the summary bit?

>  
> -	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/20260615124243.187614-1-imbrenda@linux.ibm.com?part=16