[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.1.10.0905060818000.32007@makko.or.mcafeemobile.com>
Date: Wed, 6 May 2009 08:24:53 -0700 (PDT)
From: Davide Libenzi <davidel@...ilserver.org>
To: Gregory Haskins <ghaskins@...ell.com>
cc: viro@...IV.linux.org.uk, kvm@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
avi@...hat.com
Subject: Re: [KVM PATCH v4 2/2] kvm: add support for irqfd via eventfd-notification
interface
On Wed, 6 May 2009, Gregory Haskins wrote:
> Al, Davide,
>
> Gregory Haskins wrote:
> > +
> > +int
> > +kvm_irqfd(struct kvm *kvm, int gsi, int flags)
> > +{
> > + struct _irqfd *irqfd;
> > + struct file *file = NULL;
> > + int fd = -1;
> > + int ret;
> > +
> > + irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
> > + if (!irqfd)
> > + return -ENOMEM;
> > +
> > + irqfd->kvm = kvm;
> > + irqfd->gsi = gsi;
> > + INIT_LIST_HEAD(&irqfd->list);
> > + INIT_WORK(&irqfd->work, irqfd_inject);
> > +
> > + /*
> > + * We re-use eventfd for irqfd, and therefore will embed the eventfd
> > + * lifetime in the irqfd.
> > + */
> > + file = eventfd_file_create(0, 0);
> > + if (IS_ERR(file)) {
> > + ret = PTR_ERR(file);
> > + goto fail;
> > + }
> > +
> > + irqfd->file = file;
> > +
> > + /*
> > + * Install our own custom wake-up handling so we are notified via
> > + * a callback whenever someone signals the underlying eventfd
> > + */
> > + init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
> > + init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
> > +
> > + ret = file->f_op->poll(file, &irqfd->pt);
> > + if (ret < 0)
> > + goto fail;
> > +
> > + mutex_lock(&kvm->lock);
> > + list_add_tail(&irqfd->list, &kvm->irqfds);
> > + mutex_unlock(&kvm->lock);
> > +
> > + ret = get_unused_fd();
> > + if (ret < 0)
> > + goto fail;
> > +
> > + fd = ret;
> > +
> > + fd_install(fd, file);
> >
>
> Can you comment on whether this function needs to take an additional
> reference on file here? (one for the one held by userspace/fd, and the
> other for irqfd->file) My instinct is telling me this may be currently
> broken, but I am not sure.
I don't know exactly how it is used, but looks broken. If the fd is
returned to userspace, and userspace closes the fd, you will leak the
irqfd*. If you do an extra fget(), you will never know if the userspace
closed the last-1 instance of the eventfd file*.
What is likely needed, is add a callback to eventfd_file_create(), so that
the eventfd can call your callback on ->release, and give you a chance to
perform proper cleanup of the irqfd*.
- Davide
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists