kvm_read_guest_page() missing kvm->srcu read lock?

[Andre Przywara <andre.przywara@arm.com>]

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*.

Does that sound correct? Has anyone run a lockdep enabled kernel with
PROVE_RCU before and stumbled upon this message?
I am a bit doubtful that this is valid because this is generic KVM code
and should trigger with every kvm_read_guest() user.

With my limited understanding I would say the we just need to have a
srcu_read_lock() call at the beginning of kvm_read_guest_page() and the
corresponding srcu_read_unlock() call at the end of that function. Is
that enough?

Cheers,
Andre.