linux-kernel - Re: [PATCH] KVM: use unified srcu interface function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y7TUPw5i5BejllCB@google.com>
Date:   Wed, 4 Jan 2023 01:19:59 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Hao Peng <flyingpenghao@...il.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: use unified srcu interface function

On Fri, Dec 23, 2022, Paolo Bonzini wrote:
> On 12/20/22 08:47, Hao Peng wrote:
> > > > +       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
> > > > +                                       lockdep_is_held(&kvm->irq_lock));
> > > Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never
> > > called with kvm->irq_srcu held.  I do like the of replacing '1' with
> > > lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just
> > > doing that?  I.e.
> > > 
> > Sorry for the long delay in replying. Although kvm->irq_srcu is not required
> > to protect irq_routing here, this interface function srcu_dereference_check
> > indicates that irq_routing is protected by kvm->irq_srcu in the kvm subsystem.
> > Thanks.
> > 
> 
> I agree, the last two arguments basically are alternative conditions to
> satisfy the check:
> 
> #define srcu_dereference_check(p, ssp, c) \
>         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
>                                 (c) || srcu_read_lock_held(ssp), __rcu)
> 
> The idea is to share the code between readers and writers,

But readers and writers naturally don't share code, and the subsequent
synchronize_srcu_expedited() is what really documents the interaction between
readers and writers.

It's definitely not a sticking point though, and this one does seems to be the
outlier in KVM.

> so what do you think of adding a
> 
> #define kvm_get_irq_routing(kvm) srcu_dereference_check(...)
> 
> macro at the top of virt/kvm/irqchip.c?

I'm fine with any approach, though a macro seems like overkill.