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

[Claudio Imbrenda]

On Wed, 12 Aug 2026 14:59:36 +0200
"Christoph Schlameuss" <schlameuss@xxxxxxxxxxxxx> wrote:

> 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 <imbrenda@xxxxxxxxxxxxx>
> > ---
> >  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?

no, because the store status should have happened before the failed
interrupt injections.

The only tricky part is if store status fails, but the other injections
don't. In that case we should have aborted without injecting the other
interupts. On the other hand, when the caller receives a -EFAULT it
will probably just give up on the whole VM.

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