Messages in this thread | | | Date | Tue, 05 May 2009 21:10:47 +0300 | From | Avi Kivity <> | Subject | Re: [KVM PATCH v4 2/2] kvm: add support for irqfd via eventfd-notification interface |
| |
Gregory Haskins wrote: > Avi Kivity wrote: > >> 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; >>> >>> >> You need to increase the refcount on struct kvm here. Otherwise evil >> userspace will create an irqfd, close the vm and vcpu fds, and inject >> an interrupt. >> > > I just reviewed the code in prep for v5, and now I remember why I didnt > take a reference: I designed it the opposite direction: the vm-fd owns > a reference to the irqfd, and will decouple the kvm context from the > eventfd on shutdown (see kvm_irqfd_release()). I still need to spin a > v5 regardless in order to add the padding as previously discussed. But > let me know if you still see any holes in light of this alternate object > lifetime approach I am using. >
Right, irqfd_release works. But I think refcounting is simpler, since we already kvm_get_kvm() and kvm_put_kvm(), and you wouldn't need the irqfd list. On the other hand, I'm not sure you get a callback from eventfd on close(), so refcounting may not be implementable.
Drat, irqfd_release doesn't work. You reference kvm->lock in irqfd_inject without taking any locks.
btw, there's still your original idea of creating the eventfd in userspace and passing it down. That would be workable if we can see a way to both signal the eventfd and get called back in irq context. Maybe that's preferable to what we're doing here, but we need to see how it would work.
-- Do not meddle in the internals of kernels, for they are subtle and quick to panic.
| |