linux-kernel - Re: [KVM PATCH v4 2/2] kvm: add support for irqfd via eventfd-notification interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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/