From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42400) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3i5D-000384-0i for qemu-devel@nongnu.org; Mon, 29 Jul 2013 03:41:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3i54-00081R-3j for qemu-devel@nongnu.org; Mon, 29 Jul 2013 03:41:30 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:57116) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3i53-00081D-Rr for qemu-devel@nongnu.org; Mon, 29 Jul 2013 03:41:22 -0400 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Jul 2013 08:38:31 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 2B8E61B08061 for ; Mon, 29 Jul 2013 08:41:17 +0100 (BST) Received: from d06av05.portsmouth.uk.ibm.com (d06av05.portsmouth.uk.ibm.com [9.149.37.229]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6T7f5HI44105752 for ; Mon, 29 Jul 2013 07:41:05 GMT Received: from d06av05.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av05.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r6T7fGpq020091 for ; Mon, 29 Jul 2013 01:41:16 -0600 Date: Mon, 29 Jul 2013 09:41:15 +0200 From: Heiko Carstens Message-ID: <20130729074115.GF4178@osiris> References: <1374857279-28248-1-git-send-email-jfrei@linux.vnet.ibm.com> <1374857279-28248-3-git-send-email-jfrei@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1374857279-28248-3-git-send-email-jfrei@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [RFC 2/2] KVM: s390: add floating irq controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jens Freimann Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Dominik Dingel , Alexander Graf , Christian Borntraeger , Cornelia Huck On Fri, Jul 26, 2013 at 06:47:59PM +0200, Jens Freimann wrote: > This patch adds a floating irq controller as a kvm_device. > It will be necesary for migration of floating interrupts as well > as for hardening the reset code by allowing user space to explicitly > remove all pending floating interrupts. > > Signed-off-by: Jens Freimann [...] > +static void clear_floating_interrupts(struct kvm *kvm) > +{ > + struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int; > + struct kvm_s390_interrupt_info *n, *inti = NULL; > + > + if (atomic_read(&fi->active)) { > + spin_lock_bh(&fi->lock); > + list_for_each_entry_safe(inti, n, &fi->list, list) { > + list_del(&inti->list); > + kfree(inti); > + } > + atomic_set(&fi->active, 0); > + spin_unlock_bh(&fi->lock); > + } > +} FWIW, unrelated to your patch, since it used to be always like this: the sematics of fi->active atomic_t seem to be a bit odd. It only gets set while the fi->lock spinlock is held and might be read also while not holding the spinlock. Either it's racy, the spinlock is not needed at all, or it should be held everytime. Besides that the meaning of the "active" value seems to be the same as !list_empty()... so you could get rid of it. Just a comment ;) > +static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) > +{ > + int r; > + > + switch (attr->group) { > + case KVM_DEV_FLIC_ENQUEUE: { > + struct kvm_s390_irq *s390irq; > + struct kvm_s390_interrupt_info *inti; > + inti = kzalloc(sizeof(*inti), GFP_KERNEL); > + if (!inti) > + return -ENOMEM; > + s390irq = kzalloc(sizeof(*s390irq), GFP_KERNEL); > + if (!s390irq) > + return -ENOMEM; > + if (copy_from_user(s390irq, (u64 __user *)attr->addr, > + sizeof(s390irq))) > + return -EFAULT; User space triggerable memory leak. > + inti->irq = *s390irq; > + __inject_vm(dev->kvm, inti); I think you must check the irq type, otherwise the kernel may crash if it tries to deliver the interrupt and it doesn't match any of the known irqs. (or remove the BUG() in the deliver function, or both ;)