Re: [Qemu-devel] [RFC 2/2] KVM: s390: add floating irq controller

[Heiko Carstens <heiko.carstens@de.ibm.com>]

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 <jfrei@linux.vnet.ibm.com>

[...]

> +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 ;)