Re: [KVM PATCH v4 2/2] kvm: add support for irqfd via eventfd-notificationinterface

[Gregory Haskins]

Davide Libenzi wrote:
> 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.
Yeah, I think it is.  I addressed this in v5 so please review that
version if you have a moment.

>  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*.
>   

I think we are ok in this regard (at least in v5) without the callback. 
kvm holds irqfd, which holds eventfd.  In a normal situation, we will
have eventfd with 2 references.  If userspace closes the eventfd, it
will drop 1 of the 2 eventfd file references, but the object should
remain intact as long as kvm still holds it as well.  When the kvm-fd is
released, we will then decouple from the eventfd->wqh and drop the last
fput(), officially freeing it.

Likewise, if kvm is closed before the eventfd, we will simply decouple
from the wqh and fput(eventfd), leaving the last reference held by
userspace until it closes as well.

Let me know if you see any holes in that.

Thanks Davide,
-Greg

Attachment: signature.asc
Description: OpenPGP digital signature