Re: kvm_read_guest_page() missing kvm->srcu read lock?

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

Hi,

On 11/05/18 17:44, Paolo Bonzini wrote:
> On 11/05/2018 15:25, Andre Przywara wrote:
>> - I couldn't find any protection for the usage in
>> arch/powerpc/kvm/powerpc.c, but the call chain is quite convoluted
>> there, so I might have missed something. It would be good if someone
>> more familiar with this code would take a look.
> 
> I also didn't find anything, I got up to kvmppc_handle_exit_pr in
> book3s_pr.c and kvmppc_handle_exit in booke.c, then the callers are
> assembly and I decided it's buggy. :)
> 
>>> Adding the srcu_read_lock/unlock directly in kvm_arch_vcpu_ioctl_run and
>>> any other ioctls that need it is best, but in any case adding more pairs
>>> is safe because they can be nested.
>>
>> So I added a small wrapper around kvm_read_guest(), which takes and
>> drops the lock. Will send out the patch shortly. If powerpc needs it, I
>> am happy to provide this wrapper in kvm_main.c instead of some arm
>> header file instead.
> 
> I think that risks having some performance impact, though perhaps
> mitigated by ARM having many virtual devices in the core.  Moving it
> above would be better, to the equivalent of POWER's kvmppc_handle_exit*
> functions.

Well, I am not sure the performance is super important here, since doing
kvm_read_guest() in the first place is probably not very cheap anyway.
Also some instances of kvm_read_guest() are called from the save/restore
code, which originates from userland, so we wouldn't be covered by doing
in handle_exit.
But we can somewhat optimize by taking some locks outside of loops, for
instance. Not sure that is premature optimization, though. I would
prefer doing that later. I think in the past I had some idea how to
avoid accessing guest memory in some cases, not sure those still apply.

Cheers,
Andre.