Re: [KVM PATCH v10] kvm: add support for irqfd

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Gregory Haskins <ghaskins@novell.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	avi@redhat.com, davidel@xmailserver.org, mtosatti@redhat.com
Subject: Re: [KVM PATCH v10] kvm: add support for irqfd
Date: Wed, 27 May 2009 21:41:06 +0300	[thread overview]
Message-ID: <20090527184106.GA18463@redhat.com> (raw)
In-Reply-To: <4A1D48FA.9080403@novell.com>

On Wed, May 27, 2009 at 10:06:50AM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Wed, May 20, 2009 at 10:30:49AM -0400, Gregory Haskins wrote:
> >   
> >> +static int
> >> +kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
> >> +{
> >> +	struct _irqfd *irqfd;
> >> +	struct file *file = NULL;
> >> +	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);
> >> +
> >> +	/*
> >> +	 * Embed the file* lifetime in the irqfd.
> >> +	 */
> >> +	file = fget(fd);
> >> +	if (IS_ERR(file)) {
> >> +		ret = PTR_ERR(file);
> >> +		goto fail;
> >> +	}
> >>     
> >
> > So we get a reference to a file, and unless the user is nice to us, it
> > will only be dropped when kvm char device file is closed?
> > I think this will deadlock if the fd in question is the open kvm char device.
> >
> >
> >   
> Hmm...I hadn't considered this possibility, though I am not sure if it
> would cause a deadlock in the pattern you suggest.  It seems more like
> it would result in, at worst, an extra reference to itself (and thus a
> leak) rather than a deadlock...
> 
> I digress.  In either case, perhaps I should s/fget/eventfd_fget to at
> least limit the type of fd to eventfd.  I was trying to be "slick" by
> not needing the eventfd_fget() exported, but I am going to need to
> export it later anyway for iosignalfd, so its probably a moot point.
> 
> Thanks Michael,
> -Greg
> 

This only works as long as eventfd does not do fget on some fd as well.
Which it does not do now, and may never do - but we create a fragile
system this way.

I think it's really wrong, fundamentally, to keep a reference to a
file until another file is closed, unless you are code under fs/.
We will get nasty circular references sooner or later.

Isn't the real reason we use fd to be able to support the same interface
on top of both kvm and lguest?
And if so, wouldn't some kind of bus be a better solution?

-- 
MST