Re: [PATCH v2] KVM: s390: Improve floating IRQ injection behavior

Am 17.08.26 um 14:28 schrieb [email protected]:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Use-After-Free of `inti` when injecting floating interrupts.
> - [High] Missed vCPU kick causes lost or delayed floating interrupts when all vCPUs have masks closed.
> --
> 
> --- Patch [1]: [PATCH v2] KVM: s390: Improve floating IRQ injection behavior ---
> commit e7cb031c2f46e8290e932cd848faad5eec943d11
> Author: Janosch Frank <[email protected]>
> Subject: KVM: s390: Improve floating IRQ injection behavior
> 
> Floating IRQs can be handled by any VCPU that opened its masks. The
> current design does not check if the mask is open when a floating IRQ
> is injected via the FLIC. It will wakeup the last VCPU that went
> sleeping hoping it's the correct one.
> 
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 8f24bcd1a6d36..e67cdedb5aece 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
> 
> [ ... ]
> 
>> @@ -1915,22 +1946,48 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
> 
> [ ... ]
> 
>>   /*
>>    * Find a destination VCPU for a floating irq and kick it.
>>    */
>> -static void __floating_irq_kick(struct kvm *kvm, u64 type)
>> +static void __floating_irq_kick(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>   {
>>   	struct kvm_vcpu *dst_vcpu;
>>   	int sigcpu, online_vcpus, nr_tries = 0;
>> +	u64 type = READ_ONCE(inti->type);
>> +	u64 irq_pend_mask;
>>   
>>   	online_vcpus = atomic_read(&kvm->online_vcpus);
>>   	if (!online_vcpus)
>>   		return;
>>   
>> +	irq_pend_mask = inti_to_irq_pend_mask(inti);
>>   	for (sigcpu = kvm->arch.float_int.last_sleep_cpu; ; sigcpu++) {
>>   		sigcpu %= online_vcpus;
>>   		dst_vcpu = kvm_get_vcpu(kvm, sigcpu);
>> -		if (!is_vcpu_stopped(dst_vcpu))
>> +		if (!is_vcpu_stopped(dst_vcpu) &&
>> +		    deliverable_irqs(dst_vcpu) & irq_pend_mask)
>>   			break;
>>   		/* avoid endless loops if all vcpus are stopped */
>>   		if (nr_tries++ >= online_vcpus)
>>   			return;
> 
> [Severity: High]
> Does this code drop the interrupt kick entirely if all vCPUs currently
> have their interrupt masks closed?

I think this is a corner case but still a valid finding. We can probably consider this
slowpath and wakeup/set cpuflags for ALL cpus? maybe after doing 2 rounds instead of one?