From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Subject: Re: [PATCH v2 1/5] KVM: eventfd: Fix lock order inversion. Date: Mon, 17 Mar 2014 22:55:49 +0100 Message-ID: <53276F65.4070501@de.ibm.com> References: <1395079899-29239-1-git-send-email-cornelia.huck@de.ibm.com> <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Cornelia Huck , kvm@vger.kernel.org, linux-s390@vger.kernel.org, qemu-devel@nongnu.org Cc: agraf@suse.de, pbonzini@redhat.com, gleb@kernel.org List-ID: On 17/03/14 19:11, Cornelia Huck wrote: > When registering a new irqfd, we call its ->poll method to collect any > event that might have previously been pending so that we can trigger it. > This is done under the kvm->irqfds.lock, which means the eventfd's ctx > lock is taken under it. > > However, if we get a POLLHUP in irqfd_wakeup, we will be called with the > ctx lock held before getting the irqfds.lock to deactivate the irqfd, > causing lockdep to complain. > > Calling the ->poll method does not really need the irqfds.lock, so let's > just move it after we've given up the irqfds.lock in kvm_irqfd_assign(). > > Signed-off-by: Cornelia Huck Do you still have the lockdep message somewhere? Looking at the patch and the description this makes sense. Even without irqfd for s390: Reviewed-by: Christian Borntraeger Paolo, maybe this patch can go in independently from s390? Christian > --- > virt/kvm/eventfd.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index abe4d60..29c2a04 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -391,19 +391,19 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args) > lockdep_is_held(&kvm->irqfds.lock)); > irqfd_update(kvm, irqfd, irq_rt); > > - events = f.file->f_op->poll(f.file, &irqfd->pt); > - > list_add_tail(&irqfd->list, &kvm->irqfds.items); > > + spin_unlock_irq(&kvm->irqfds.lock); > + > /* > * Check if there was an event already pending on the eventfd > * before we registered, and trigger it as if we didn't miss it. > */ > + events = f.file->f_op->poll(f.file, &irqfd->pt); > + > if (events & POLLIN) > schedule_work(&irqfd->inject); > > - spin_unlock_irq(&kvm->irqfds.lock); > - > /* > * do not drop the file until the irqfd is fully initialized, otherwise > * we might race against the POLLHUP > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPfVq-0003KH-Ru for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:56:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPfVh-0002WM-Nq for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:56:02 -0400 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:48948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPfVh-0002WG-Ed for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:55:53 -0400 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 17 Mar 2014 21:55:52 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id B8DA8219004D for ; Mon, 17 Mar 2014 21:55:45 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2HLtcRu59637996 for ; Mon, 17 Mar 2014 21:55:38 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2HLtnpd021969 for ; Mon, 17 Mar 2014 17:55:50 -0400 Message-ID: <53276F65.4070501@de.ibm.com> Date: Mon, 17 Mar 2014 22:55:49 +0100 From: Christian Borntraeger MIME-Version: 1.0 References: <1395079899-29239-1-git-send-email-cornelia.huck@de.ibm.com> <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com> In-Reply-To: <1395079899-29239-2-git-send-email-cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/5] KVM: eventfd: Fix lock order inversion. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , kvm@vger.kernel.org, linux-s390@vger.kernel.org, qemu-devel@nongnu.org Cc: gleb@kernel.org, pbonzini@redhat.com, agraf@suse.de On 17/03/14 19:11, Cornelia Huck wrote: > When registering a new irqfd, we call its ->poll method to collect any > event that might have previously been pending so that we can trigger it. > This is done under the kvm->irqfds.lock, which means the eventfd's ctx > lock is taken under it. > > However, if we get a POLLHUP in irqfd_wakeup, we will be called with the > ctx lock held before getting the irqfds.lock to deactivate the irqfd, > causing lockdep to complain. > > Calling the ->poll method does not really need the irqfds.lock, so let's > just move it after we've given up the irqfds.lock in kvm_irqfd_assign(). > > Signed-off-by: Cornelia Huck Do you still have the lockdep message somewhere? Looking at the patch and the description this makes sense. Even without irqfd for s390: Reviewed-by: Christian Borntraeger Paolo, maybe this patch can go in independently from s390? Christian > --- > virt/kvm/eventfd.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index abe4d60..29c2a04 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -391,19 +391,19 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args) > lockdep_is_held(&kvm->irqfds.lock)); > irqfd_update(kvm, irqfd, irq_rt); > > - events = f.file->f_op->poll(f.file, &irqfd->pt); > - > list_add_tail(&irqfd->list, &kvm->irqfds.items); > > + spin_unlock_irq(&kvm->irqfds.lock); > + > /* > * Check if there was an event already pending on the eventfd > * before we registered, and trigger it as if we didn't miss it. > */ > + events = f.file->f_op->poll(f.file, &irqfd->pt); > + > if (events & POLLIN) > schedule_work(&irqfd->inject); > > - spin_unlock_irq(&kvm->irqfds.lock); > - > /* > * do not drop the file until the irqfd is fully initialized, otherwise > * we might race against the POLLHUP >