Re: [PATCH] KVM: x86/xen: Use read_trylock() for GPC locks in hardirq paths

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware interrupt leak on PREEMPT_RT in __kvm_xen_has_interrupt()
--

--- Patch [1]: [PATCH] KVM: x86/xen: Use read_trylock() for GPC locks in hardirq paths ---
commit 094309ad41a72add2b4de793f00e2fe6f42178a3
Author: sai madhu <[email protected]>

KVM: x86/xen: Use read_trylock() for GPC locks in hardirq paths

This commit switches GPC locks in hardirq paths to use read_trylock()
instead of read_lock_irqsave(). This prevents sleeping on PREEMPT_RT
and under lockdep, returning -EWOULDBLOCK when the lock is contended.

Fixes syzbot: https://syzkaller.appspot.com/bug?extid=919877893c9d28162dc2

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 694b31c1fcc9a..02f30d79513db 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -713,7 +714,15 @@ int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
>  	BUILD_BUG_ON(sizeof(rc) !=
>  		     sizeof_field(struct compat_vcpu_info, evtchn_upcall_pending));
>  
> -	read_lock_irqsave(&gpc->lock, flags);
> +	if (atomic) {
> +		local_irq_save(flags);
> +		if (!read_trylock(&gpc->lock)) {
> +			local_irq_restore(flags);
> +			return 1;
> +		}
> +	} else {
> +		read_lock_irqsave(&gpc->lock, flags);
> +	}
>  	while (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
>  		read_unlock_irqrestore(&gpc->lock, flags);

[Severity: High]
Does this leave hardware interrupts disabled on PREEMPT_RT?

Since local_irq_save(flags) is called manually here when atomic is true, but
the lock is later released using read_unlock_irqrestore(&gpc->lock, flags),
will the hardware interrupt state be correctly restored?

Looking at include/linux/rwlock_rt.h, read_unlock_irqrestore() for rwlock_t
on PREEMPT_RT is mapped to rt_read_unlock(), which drops the lock but
completely ignores the flags parameter:

include/linux/rwlock_rt.h:
static __always_inline void read_unlock_irqrestore(rwlock_t *rwlock,
						   unsigned long flags)
{
	rt_read_unlock(rwlock);
}

If atomic is true, doesn't this mean the previously saved hardware interrupt
state is never restored here or at the end of the function, causing IRQs to
remain disabled indefinitely?

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