On 10/05/2018 19:41, Andre Przywara wrote: > Hi, > > Jan posted an lockdep splat complaining about a suspicious > rcu_dereference_check: > https://lists.cs.columbia.edu/pipermail/kvmarm/2018-May/031116.html > > The gist of that is: > ... > [ 1025.695517] dump_stack+0x9c/0xd4 > [ 1025.695524] lockdep_rcu_suspicious+0xcc/0x118 > [ 1025.695537] gfn_to_memslot+0x174/0x190 > [ 1025.695546] kvm_read_guest+0x50/0xb0 > [ 1025.695553] vgic_its_check_id.isra.0+0x114/0x148 > ... > I chased that down and wonder if kvm_read_guest{,_page} is supposed to > be called inside a kvm->srcu critical section? > > We have a check that suggests that eventually someone needs to enter the > SRCU criticial section: > static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm, > int as_id) > { > return srcu_dereference_check(kvm->memslots[as_id], &kvm->srcu, > lockdep_is_held(&kvm->slots_lock) || > !refcount_read(&kvm->users_count)); > } > > If I get this correctly this mean for accessing kvm->memslots we either > need to be inside an srcu critical section or hold the kvm->slots_lock > (for updates only). > > If I am not mistaken, it is not necessary for *callers* of > kvm_read_guest_page() to do this, as this could be entirely contained > inside this function - since we only use the reference to the memslot > entry while doing the copy_from_user(), and the data is safe afterwards > from an RCU point of view because it has been *copied*. Yes, it's the caller's responsibility. srcu_read_lock/unlock is pretty expensive so KVM assumes that the topmost callers do it. Paolo
