Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Sat, Mar 08, 2008 at 06:45:34PM -0800, Roland Dreier wrote:
spin_lock(kvm_lock);
+ if (--kvm-refcount) {
+ spin_lock(kvm_lock);
obvious typo here...
Indeed. Any comments from the kvm developers in this approach? The
current multi-level file refcounting seems rather bogus so I'd like
to make some progress on this.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
Christoph Hellwig wrote:
On Sat, Mar 08, 2008 at 06:45:34PM -0800, Roland Dreier wrote:
spin_lock(kvm_lock);
+ if (--kvm-refcount) {
+ spin_lock(kvm_lock);
obvious typo here...
Indeed. Any comments from the kvm developers in this approach? The
current multi-level file refcounting seems rather bogus so I'd like
to make some progress on this.
I'm okay with switching away from the file-based refcounts to a refcount
embedded in the kvm structures, though I'm not particularly thrilled by
it. Any reason not to use krefs for this?
--
error compiling committee.c: too many arguments to function
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
spin_lock(kvm_lock);
+if (--kvm-refcount) {
+spin_lock(kvm_lock);
obvious typo here...
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, Feb 27, 2008 at 03:42:42PM -0800, Roland Dreier wrote:
http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034
Actually, looking closer at the kvm changes here, I think that
create_vcpu_fd() needs the same treatment as kvm_dev_ioctl_create_vm()
gets in the patch because of the race I mentioned in the changelog
for my patch: otherwise kvm_vcpu_release() could drop the last
reference to vcpu-kvm-filp before the get_file() gets an extra
reference.
Actually using the file pointer for reference counting in kvm is quite
stupid and risky, it should use a normal reference count instead.
See untested patch below.
I'm beginning to think that moving the fd_install() out of
anon_inode_getfd() really is worth it to make a safer interface.
No, the best interface is one where the driver doesn't even see
inode or file. Of course that's not actually possible in a few
nasty cases like the infiniband code, and for those it might be
better to do the fd_isntall themselves.
Index: linux-2.6/include/linux/kvm_host.h
===
--- linux-2.6.orig/include/linux/kvm_host.h 2008-02-23 20:28:14.0
+0100
+++ linux-2.6/include/linux/kvm_host.h 2008-02-23 20:36:20.0 +0100
@@ -113,7 +113,7 @@ struct kvm {
KVM_PRIVATE_MEM_SLOTS];
struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
struct list_head vm_list;
- struct file *filp;
+ int refcount;
struct kvm_io_bus mmio_bus;
struct kvm_io_bus pio_bus;
struct kvm_vm_stat stat;
Index: linux-2.6/virt/kvm/kvm_main.c
===
--- linux-2.6.orig/virt/kvm/kvm_main.c 2008-02-23 20:29:12.0 +0100
+++ linux-2.6/virt/kvm/kvm_main.c 2008-02-24 02:34:44.0 +0100
@@ -169,6 +169,8 @@ static struct kvm *kvm_create_vm(void)
kvm_io_bus_init(kvm-pio_bus);
mutex_init(kvm-lock);
kvm_io_bus_init(kvm-mmio_bus);
+ kvm-refcount = 1;
+
spin_lock(kvm_lock);
list_add(kvm-vm_list, vm_list);
spin_unlock(kvm_lock);
@@ -201,11 +203,16 @@ void kvm_free_physmem(struct kvm *kvm)
kvm_free_physmem_slot(kvm-memslots[i], NULL);
}
-static void kvm_destroy_vm(struct kvm *kvm)
+static void kvm_put_vm(struct kvm *kvm)
{
struct mm_struct *mm = kvm-mm;
spin_lock(kvm_lock);
+ if (--kvm-refcount) {
+ spin_lock(kvm_lock);
+ return;
+ }
+
list_del(kvm-vm_list);
spin_unlock(kvm_lock);
kvm_io_bus_destroy(kvm-pio_bus);
@@ -216,9 +223,7 @@ static void kvm_destroy_vm(struct kvm *k
static int kvm_vm_release(struct inode *inode, struct file *filp)
{
- struct kvm *kvm = filp-private_data;
-
- kvm_destroy_vm(kvm);
+ kvm_put_vm(filp-private_data);
return 0;
}
@@ -700,7 +705,7 @@ static int kvm_vcpu_release(struct inode
{
struct kvm_vcpu *vcpu = filp-private_data;
- fput(vcpu-kvm-filp);
+ kvm_put_vm(vcpu-kvm);
return 0;
}
@@ -724,7 +729,16 @@ static int create_vcpu_fd(struct kvm_vcp
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r)
return r;
- atomic_inc(vcpu-kvm-filp-f_count);
+
+ /*
+* It is guaranteed that the refcount is non-zero here because
+* this function is only called as ioctl method on an open
+* file that has a reference to it.
+*/
+ spin_lock(kvm_lock);
+ vcpu-kvm-refcount++;
+ spin_unlock(kvm_lock);
+
return fd;
}
@@ -1023,12 +1037,10 @@ static int kvm_dev_ioctl_create_vm(void)
return PTR_ERR(kvm);
r = anon_inode_getfd(fd, inode, file, kvm-vm, kvm_vm_fops, kvm);
if (r) {
- kvm_destroy_vm(kvm);
+ kvm_put_vm(kvm);
return r;
}
- kvm-filp = file;
-
return fd;
}
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, Feb 27, 2008 at 11:16:02AM -0800, Roland Dreier wrote: The anonymous inodes interface anon_inode_getfd() calls fd_install() for the newly created fd, which does not work for some use cases where the caller must do futher initialization before exposing the file to userspace. This is also probably not the safest interface, since the caller must be sure that it is OK if userspace closes the fd before anon_inode_getfd() even returns. IMO that's a bad idea - majority of callers only care about fd and burdening them with fd_install() is simply wrong. Separate helper function... - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
Davide Libenzi wrote:
I think that may be a bit cleaner than Al's approach, but it still
leaves the same trap that create_vcpu_fd() falls into. The current
code is:
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
struct inode *inode;
struct file *file;
r = anon_inode_getfd(fd, inode, file,
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r)
return r;
atomic_inc(vcpu-kvm-filp-f_count);
return fd;
}
and with your proposal, the natural way to write that becomes:
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
r = anon_inode_getfd(fd, NULL,
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r)
return r;
atomic_inc(vcpu-kvm-filp-f_count);
return fd;
}
I don't know KVM code, but can't the private_data setup be completed
before calling anon_inode_getfd()?
Creating the fd is the last thing done when creating a vcpu.
Or ...
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
get_file(vcpu-kvm-filp);
r = anon_inode_getfd(fd, NULL,
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r) {
fput(vcpu-kvm-filp);
return r;
}
return fd;
}
This seems reasonable.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, 27 Feb 2008, Roland Dreier wrote:
http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034
Actually, looking closer at the kvm changes here, I think that
create_vcpu_fd() needs the same treatment as kvm_dev_ioctl_create_vm()
gets in the patch because of the race I mentioned in the changelog
for my patch: otherwise kvm_vcpu_release() could drop the last
reference to vcpu-kvm-filp before the get_file() gets an extra
reference.
I'm beginning to think that moving the fd_install() out of
anon_inode_getfd() really is worth it to make a safer interface.
If we let the caller call fd_install(), then it may be messed up WRT
cleanup (fd, file, inode).
How about removing the inode pointer handout altogether, and *doing*
fd_install() inside anon_inode_getfd() like:
if (pfile != NULL) {
get_file(file);
*pfile = file;
}
fd_install(fd, file);
In this way, if the caller want the file* back, he gets the reference
bumped before fd_install().
- Davide
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
If we let the caller call fd_install(), then it may be messed up WRT
cleanup (fd, file, inode).
Yes, that is a tiny bit tricky (need to call put_unused_fd() if you
don't install the fd).
How about removing the inode pointer handout altogether, and *doing*
fd_install() inside anon_inode_getfd() like:
if (pfile != NULL) {
get_file(file);
*pfile = file;
}
fd_install(fd, file);
In this way, if the caller want the file* back, he gets the reference
bumped before fd_install().
I think that may be a bit cleaner than Al's approach, but it still
leaves the same trap that create_vcpu_fd() falls into. The current
code is:
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
struct inode *inode;
struct file *file;
r = anon_inode_getfd(fd, inode, file,
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r)
return r;
atomic_inc(vcpu-kvm-filp-f_count);
return fd;
}
and with your proposal, the natural way to write that becomes:
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
r = anon_inode_getfd(fd, NULL,
kvm-vcpu, kvm_vcpu_fops, vcpu);
if (r)
return r;
atomic_inc(vcpu-kvm-filp-f_count);
return fd;
}
which still has the same bug.
Maybe a good way to handle this is just to make the get_file() not
optional. I dunno... I feel like we've spent more discussion on this
point than it deserves, so someone should just make a decision and
I'll adapt the ib_uverbs code to work with whatever it is.
- R.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
Roland Dreier wrote: The anonymous inodes interface anon_inode_getfd() calls fd_install() for the newly created fd, which does not work for some use cases where the caller must do futher initialization before exposing the file to userspace. This is also probably not the safest interface, since the caller must be sure that it is OK if userspace closes the fd before anon_inode_getfd() even returns. Therefore, change the anonymous inodes interface so that the caller is responsible for calling fd_install(), and change the name of the function from anon_inode_getfd() to anon_inode_allocfd() so that any code using the old interface breaks at compilation rather than failing in a strange way. Fix up all the in-kernel users to use the new interface. The kvm changes are Acked-by: Avi Kivity [EMAIL PROTECTED] -- Any sufficiently difficult bug is indistinguishable from a feature. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, 27 Feb 2008, Roland Dreier wrote: The anonymous inodes interface anon_inode_getfd() calls fd_install() for the newly created fd, which does not work for some use cases where the caller must do futher initialization before exposing the file to userspace. This is also probably not the safest interface, since the caller must be sure that it is OK if userspace closes the fd before anon_inode_getfd() even returns. I believe Al changed the interface to not give out inode* and file*, *and* call fd_install() inside it. I'd slightly prefer Al version, although I don't see any major problems in this one too. Any pointer to that patch? A web search for viro and anon_inode_getfd doesn't turn up anything likely looking. It's inside his vfs git tree: http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034 I'm fine with both approaches. - Davide - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034 Actually, looking closer at the kvm changes here, I think that create_vcpu_fd() needs the same treatment as kvm_dev_ioctl_create_vm() gets in the patch because of the race I mentioned in the changelog for my patch: otherwise kvm_vcpu_release() could drop the last reference to vcpu-kvm-filp before the get_file() gets an extra reference. I'm beginning to think that moving the fd_install() out of anon_inode_getfd() really is worth it to make a safer interface. - R. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
Roland Dreier wrote: http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034 Actually, looking closer at the kvm changes here, I think that create_vcpu_fd() needs the same treatment as kvm_dev_ioctl_create_vm() gets in the patch because of the race I mentioned in the changelog for my patch: otherwise kvm_vcpu_release() could drop the last reference to vcpu-kvm-filp before the get_file() gets an extra reference. I'm beginning to think that moving the fd_install() out of anon_inode_getfd() really is worth it to make a safer interface. It makes is less usable, though (since the last step needs to be taken by the caller. We might add a int (*prepare_file)(...) parameter which anon_inode_getfd() uses to munge the file before installing it. -- error compiling committee.c: too many arguments to function - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
