From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Carstens Subject: Re: [RFC 2/2] KVM: s390: add floating irq controller Date: Mon, 29 Jul 2013 09:41:15 +0200 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 Cc: Christian Borntraeger , Cornelia Huck , kvm@vger.kernel.org, qemu-devel@nongnu.org, Dominik Dingel , Alexander Graf To: Jens Freimann Return-path: Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:45564 "EHLO e06smtp11.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751084Ab3G2HlV (ORCPT ); Mon, 29 Jul 2013 03:41:21 -0400 Received: from /spool/local by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Jul 2013 08:35:12 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 787A817D8067 for ; Mon, 29 Jul 2013 08:43:00 +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 r6T7f5qS61014226 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 r6T7fGpu020091 for ; Mon, 29 Jul 2013 01:41:16 -0600 Content-Disposition: inline In-Reply-To: <1374857279-28248-3-git-send-email-jfrei@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 ;)