Re: Possible race condition during lock_count increment in srcu_read_lock

[Paolo Bonzini <pbonzini@redhat.com>]

Adding Paul McKenney...

On 29/05/2017 16:30, Linu Cherian wrote:
> In KVM irq injection path, irqfd_wakeup in interrupt context
> calls srcu_read_lock(&kvm->irq_srcu). A worker thread, irqfd_inject
> in process context also calls srcu_read_lock inside kvm_set_irq. 
> 
> This can lead to race condition while incrementing the
> lock_count(using __this_cpu_inc), since atomic instructions being not
> used. 
> 
> Replacing __this_cpu_inc with this_cpu_inc(atomic version) workarounds the
> problem which backs up this analysis.

Paul, how would you feel about either using this_cpu_inc in
__srcu_read_lock and __srcu_read_unlock?  Many architectures already
provide efficient this_cpu_inc, and any architecture that has LL/SC
could provide it easily.

Thanks,

Paolo

> Possible solution
> -----------------
> One way is to avoid the srcu_read_lock/unlock usage in irq context.
> In arm/arm64 architecture, 	
> if (kvm_arch_set_irq_inatomic(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
>       false) == -EWOULDBLOCK)
> in irqfd_wakefup is always true and hence schedule_work can be called
> directly
> 
> @@ -195,6 +195,11 @@ int __attribute__((weak))
> kvm_arch_set_irq_inatomic(
>         int idx;
>  
>         if (flags & POLLIN) {
> +               if (kvm_arch_set_irq_will_block_always()) {
> +                       schedule_work(&irqfd->inject);
> +                       goto skiplock;
> +               }
> +
>                 idx = srcu_read_lock(&kvm->irq_srcu);
>                 do {
>                         seq =
> 			read_seqcount_begin(&irqfd->irq_entry_sc);
> @@ -208,6 +213,7 @@ int __attribute__((weak))
> kvm_arch_set_irq_inatomic(
>                 srcu_read_unlock(&kvm->irq_srcu, idx);
>         }
>  
> +skiplock:
>         if (flags & POLLHUP) {
>                 /* The eventfd is closing, detach from KVM */
>                 unsigned long flags;
> 
> This works without giving any warnings as well.
> 
> Is a patch welcome in that direction ?
> 
> Appreicate your feedback on this.
> 
> 
>