Re: [PATCH v3 2/3] KVM: s390: Kick PV cpus at the right time for service irqs

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional acquisition of a sleeping lock (`mutex_lock(&kvm->lock)`) in the floating interrupt injection hot-path causes a system BUG when called from atomic context.
- [Critical] Acquiring `kvm->lock` inside `__floating_irq_kick()` violates the KVM locking hierarchy, creating ABBA deadlocks with `vcpu->mutex` and `kvm->srcu`.
- [High] In `kvm_s390_pv_sclp_kick()`, `IRQ_PEND_EXT_SERVICE` is incorrectly used as a bitmask instead of a bit index, causing the kick condition to evaluate incorrectly.
- [High] `inti_to_irq_pend_mask()` returns an unshifted bit index instead of a bitmask for I/O interrupts and double-converts the ISC value, breaking IO floating IRQ routing.
- [Medium] The local variable `parm` is passed by value uninitialized to `__floating_irq_kick()` for several interrupt types, triggering Undefined Behavior and KMSAN warnings.
--

--- Patch [2]: [PATCH v3 2/3] KVM: s390: Kick PV cpus at the right time for service irqs ---
commit a45baf236c563050e29f20187e5498732692fe4c
Author: Janosch Frank <[email protected]>

KVM: s390: Kick PV cpus at the right time for service irqs

Service call handling is a two stage process for PV vms. First we
receive the secure instruction intercept and then the secure
instruction notification intercept.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 3af060ec5be8e..53ba74a749e8a 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -1998,10 +1995,49 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
>  	vcpu_intervention_kick(dst_vcpu, type);
>  }
>  
> +void kvm_s390_pv_sclp_kick(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * The cpu that called sclp likely will also take the IRQ, no
> +	 * need to kick anyone.
> +	 */
> +	if (likely(deliverable_irqs(vcpu) & IRQ_PEND_EXT_SERVICE))
> +		return;

[Severity: High]
Is IRQ_PEND_EXT_SERVICE missing a BIT() macro here?

The deliverable_irqs() function returns a bitmask, but IRQ_PEND_EXT_SERVICE
is an enum value (15). Doesn't this evaluate the bitwise AND against 15
(0b1111) rather than checking bit 15? This could result in false negatives
and lost wakeups for target vCPUs waiting for service interrupts.

> +
> +	/*
> +	 * For the other cases we might have sleeping cpus with open
> +	 * masks. Time to find and kick them.
> +	 */
> +	kick_cpu_irq(vcpu->kvm, KVM_S390_INT_SERVICE, -1);
> +}
> +
> +/*
> + * Find a destination VCPU for a floating irq and kick it.
> + */
> +static void __floating_irq_kick(struct kvm *kvm, u64 type, u64 parm)
> +{
> +	int prot;
> +
> +	mutex_lock(&kvm->lock);
> +	prot = kvm_s390_pv_is_protected(kvm);
> +	mutex_unlock(&kvm->lock);

[Severity: Critical]
Can acquiring this sleeping lock cause a 'scheduling while atomic' bug?

When called via the interrupt injection path, this can execute in an atomic
context:

kvm_arch_set_irq_inatomic()
   spin_lock(&fi->ais_lock);
   kvm_s390_inject_vm()
      __inject_vm()
         __floating_irq_kick()
            mutex_lock(&kvm->lock);

[Severity: Critical]
Does taking kvm->lock here also create an ABBA deadlock?

During standard I/O interrupt reinjection:

vcpu_run()
   handle_tpi()
      kvm_s390_reinject_io_int()
         __inject_vm()
            __floating_irq_kick()

In this path, vcpu->mutex is already held by KVM_RUN, and the vCPU is inside
an SRCU read-side critical section. Acquiring kvm->lock while holding these
violates the KVM locking hierarchy.

> +	/*
> +	 * No need to kick on non-ev service IRQs for PV VMs, we're
> +	 * not allowed to inject anyway. We need to wait for the sclp
> +	 * instruction notification AFTER re-entry of the vcpu that
> +	 * handled the instruction intercept.
> +	 */
> +	if (prot && type == KVM_S390_INT_SERVICE &&
> +	    !(parm & SCCB_EVENT_PENDING))
> +		return;
> +
> +	kick_cpu_irq(kvm, type, parm);
> +}
> +
>  static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  {
>  	u64 type = READ_ONCE(inti->type);
> -	int isc = -1;
> +	u64 parm;
>  	int rc;
>  
>  	switch (type) {

[ ... ]

>  	case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
>  		/* Grab isc here since __inject_io() might free inti */
> -		isc = isc_to_irq_type(int_word_to_isc(inti->io.io_int_word));
> +		parm = isc_to_irq_type(int_word_to_isc(inti->io.io_int_word));

[Severity: High]
Does this result in a double-conversion of the ISC value?

Down in inti_to_irq_pend_mask(), the switch case for I/O interrupts does:

    case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
        return isc_to_irq_type(isc);

Since parm is already converted here and passed into inti_to_irq_pend_mask()
as the isc parameter, it looks like it gets processed through
isc_to_irq_type() a second time. It also appears to lack a BIT() macro
wrapper, returning a raw integer (0-7) instead of a bitmask.

>  		rc = __inject_io(kvm, inti);
>  		break;
>  	default:
> @@ -2028,7 +2065,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	if (rc)
>  		return rc;
>  
> -	__floating_irq_kick(kvm, type, isc);
> +	__floating_irq_kick(kvm, type, parm);

[Severity: Medium]
Does parm get passed uninitialized here for certain interrupt types?

For cases like KVM_S390_MCHK, KVM_S390_INT_VIRTIO, and
KVM_S390_INT_PFAULT_DONE, parm is never assigned a value in the switch
statement above. Passing it by value to __floating_irq_kick() causes an
uninitialized read that triggers KMSAN warnings.

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2