From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 3/4] [PATCH] kvm: Fix tprot locking Date: Thu, 17 Nov 2011 12:27:41 +0200 Message-ID: <4EC4E19D.5070801@redhat.com> References: <20111117100040.458397238@de.ibm.com> <20111117100600.399128904@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tossati , Christian Borntraeger , Heiko Carstens , Martin Schwidefsky , Cornelia Huck , KVM To: Carsten Otte Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32768 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756520Ab1KQK1r (ORCPT ); Thu, 17 Nov 2011 05:27:47 -0500 In-Reply-To: <20111117100600.399128904@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 11/17/2011 12:00 PM, Carsten Otte wrote: > From: Christian Borntraeger > > There is a potential host deadlock in the tprot intercept handling. > We must not hold the mmap semaphore while resolving the guest > address. If userspace is remapping, then the memory detection in > the guest is broken anyway so we can safely separate the > address translation from walking the vmas. > > Signed-off-by: Christian Borntraeger > Signed-off-by: Carsten Otte > --- > > arch/s390/kvm/priv.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff -urpN linux-2.6/arch/s390/kvm/priv.c linux-2.6-patched/arch/s390/kvm/priv.c > --- linux-2.6/arch/s390/kvm/priv.c 2011-10-24 09:10:05.000000000 +0200 > +++ linux-2.6-patched/arch/s390/kvm/priv.c 2011-11-17 10:03:53.000000000 +0100 > @@ -336,6 +336,7 @@ static int handle_tprot(struct kvm_vcpu > u64 address1 = disp1 + base1 ? vcpu->arch.guest_gprs[base1] : 0; > u64 address2 = disp2 + base2 ? vcpu->arch.guest_gprs[base2] : 0; > struct vm_area_struct *vma; > + unsigned long user_address; > > vcpu->stat.instruction_tprot++; > > @@ -349,9 +350,14 @@ static int handle_tprot(struct kvm_vcpu > return -EOPNOTSUPP; > > > + /* we must resolve the address without holding the mmap semaphore. > + * This is ok since the userspace hypervisor is not supposed to change > + * the mapping while the guest queries the memory. Otherwise the guest > + * might crash or get wrong info anyway. */ > + user_address = (unsigned long) __guestaddr_to_user(vcpu, address1); > + > down_read(¤t->mm->mmap_sem); > - vma = find_vma(current->mm, > - (unsigned long) __guestaddr_to_user(vcpu, address1)); > + vma = find_vma(current->mm, user_address); > if (!vma) { > up_read(¤t->mm->mmap_sem); > return kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING); > Unrelated to the patch, but I'm curious: it looks like __gmap_fault() dereferences the guest page table? How can it assume that it is mapped? I'm probably misreading the code. A little closer to the patch, x86 handles the same issue by calling get_user_pages_fast(). This should be more scalable than bouncing mmap_sem, something to consider. -- error compiling committee.c: too many arguments to function