From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: kvm_read_guest_page() missing kvm->srcu read lock? Date: Fri, 11 May 2018 17:59:49 +0100 Message-ID: <69edef2b-01f6-4aa0-84b9-da25b54edae1@arm.com> References: <32913bee-6ccc-cab7-65fb-fb6896c3d19c@arm.com> <6a60301f-1fb7-a60a-d47a-85f7ea79b1a7@redhat.com> <7a24f679-142b-9283-b7dc-f3d8e70c420a@redhat.com> <59750e07-2c69-a63e-bad4-25f7c21febbf@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: James Hogan , Marc Zyngier , Jan Glauber , Paul Mackerras , "kvmarm@lists.cs.columbia.edu" , David Gibson To: Paolo Bonzini , "kvm@vger.kernel.org" Return-path: In-Reply-To: Content-Language: en-GB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org 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.