Re: [PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> When __inject_sigp_stop() is called for a Stop and Store Status
> operation, if the vCPU is running, the interrupt is marked as pending
> and the status is stored by the thread performing the KVM_RUN IOCTL.
>
> If the vCPU is already stopped, the status is stored immediately.
>
> Storing the status means writing into userspace, which might fault, and
> __inject_sigp_stop() is called from do_inject_vcpu() which in turn is
> always called holding a spinlock, which is obviously an issue.
>
> Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
> adding a bool flag to indicate whether a store status is needed. The
> callers of do_inject_vcpu() are modified to pass the pointer to the
> bool flag; whenever a Store Status operation is needed, the callers can
> now perform it outside the spinlock.
>
> Opportunistically refactor kvm_s390_set_irq_state() to use
> scoped_guard() and __free().
>
> Signed-off-by: Claudio Imbrenda <[email protected]>
> ---
>  arch/s390/kvm/interrupt.c | 70 +++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 8e4b88bce31f..6940f4d354e5 100644

[...]

> @@ -3188,31 +3192,33 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	if (copy_from_user((void *) buf, irqstate, len)) {
> -		r = -EFAULT;
> -		goto out_free;
> -	}
> +	if (copy_from_user((void *)buf, irqstate, len))
> +		return -EFAULT;
>  
> -	/*
> -	 * Don't allow setting the interrupt state
> -	 * when there are already interrupts pending
> -	 */
> -	spin_lock(&li->lock);
> -	if (li->pending_irqs) {
> -		r = -EBUSY;
> -		goto out_unlock;
> -	}
> +	scoped_guard(spinlock, &li->lock) {
> +		/*
> +		 * Don't allow setting the interrupt state
> +		 * when there are already interrupts pending
> +		 */
> +		if (li->pending_irqs)
> +			return -EBUSY;
>  
> -	for (n = 0; n < len / sizeof(*buf); n++) {
> -		r = do_inject_vcpu(vcpu, &buf[n]);
> -		if (r)
> -			break;
> +		for (n = 0; n < len / sizeof(*buf); n++) {
> +			tmp = false;
> +			r = do_inject_vcpu(vcpu, &buf[n], &tmp);
> +			if (r == -EWOULDBLOCK && tmp) {
> +				storestatus = true;
> +				r = 0;
> +			}
> +			if (r)
> +				break;
> +		}
>  	}
>  
> -out_unlock:
> -	spin_unlock(&li->lock);
> -out_free:
> -	vfree(buf);
> +	if (storestatus) {
> +		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);

I assume we do not care about loosing n = -EFAULT when we are already on the
error path here with r != 0. But are there cases in which we would not want to
call kvm_s390_store_status_unloaded() at all here when one of the later
do_inject_vcpu() calls failed with a specific error?

> +		return r ? r : n;
> +	}
>  
>  	return r;
>  }