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

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 3:11 PM CEST, Claudio Imbrenda wrote:
> 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.
>

Thanks, that would have been my feeling, just wanted to validate.

Reviewed-by: Christoph Schlameuss <schlameuss@xxxxxxxxxxxxx>


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