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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
