Re: [patch] nfs: fix locking in nfs/inode.c in nfs_free_open_context

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 26 July 2007, Trond Myklebust wrote:
> Really ugly. Here's an alternative that is a lot more palatable.

Yes, much better than mine.

> ---------------------
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date: Thu, 26 Jul 2007 12:06:17 -0400
> NFS: Fix put_nfs_open_context
> 
> We need to grab the inode->i_lock atomically with the last reference put in
> order to remove the open context that is being freed from the
> nfsi->open_files list.
> 
> Fix by converting the kref to a standard atomic counter and then using
> atomic_dec_and_lock()...
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

>From the bit of research I did on the bug yesterday, it seems
that the race has been in there for ages, but may have become easier
to hit with the change to kref after 2.6.22. I don't really
understand why we didn't hit it in RHEL5/2.6.18 with the same
test case, but in Fedora 7/2.6.21.

Should a patch like the one below (I said like, you know you
can't trust my patches for your code ;-) be put into 2.6.22.x
and updated distro kernels?

	Arnd <><

--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -482,21 +482,19 @@ struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx)
 
 void put_nfs_open_context(struct nfs_open_context *ctx)
 {
-	if (atomic_dec_and_test(&ctx->count)) {
-		if (!list_empty(&ctx->list)) {
-			struct inode *inode = ctx->dentry->d_inode;
-			spin_lock(&inode->i_lock);
-			list_del(&ctx->list);
-			spin_unlock(&inode->i_lock);
-		}
-		if (ctx->state != NULL)
-			nfs4_close_state(ctx->state, ctx->mode);
-		if (ctx->cred != NULL)
-			put_rpccred(ctx->cred);
-		dput(ctx->dentry);
-		mntput(ctx->vfsmnt);
-		kfree(ctx);
-	}
+	struct inode *inode = ctx->dentry->d_inode;
+
+	if (!atomic_dec_and_lock(&ctx->count, &inode->i_lock))
+		return;
+	list_del(&ctx->list);
+	spin_unlock(&inode->i_lock);
+	if (ctx->state != NULL)
+		nfs4_close_state(ctx->state, ctx->mode);
+	if (ctx->cred != NULL)
+		put_rpccred(ctx->cred);
+	dput(ctx->dentry);
+	mntput(ctx->vfsmnt);
+	kfree(ctx);
 }
 
 /*