Messages in this thread Patch in this message | | | From | Arnd Bergmann <> | Subject | Re: [patch] nfs: fix locking in nfs/inode.c in nfs_free_open_context | Date | Thu, 26 Jul 2007 17:13:07 +0200 |
| |
On Thursday 26 July 2007, Trond Myklebust wrote: > > > > > Wrong. It is quite safe to test the structure member ctx->list for > > > > emptiness outside the spinlock because we have an explicit guarantee > > > > that nobody else has a reference to this structure, plus the > > > > atomic_dec_and_test() in kref_put() has acted as a memory barrier for > > > > us > > > > > > Well, the real question then is how the ctx can still be present in the > > > nfsi->open_files list. Since we are in nfs_free_open_context(), there > > > must not be any pointer to the ctx anywhere, but still we have this other > > > thread calling get_nfs_open_context() on it > > > No. That is still incorrect. The list of open contexts is used for > things like NFSv4 state recovery (when we're doing background writes, > and the server happens to reboot). The lifetime of the open context may > exceed that of the struct file that created it.
Unfortunately, you didn't answer my question. The observed problem is that the final kref_put gets called at a time where there are still references to the context in nfsi->open_files, and other threads therefore do get at them.
The list_empty()/list_del() in nfs_free_open_context is bogus, because as you say, there is a guarantee that there is no other reference on the structure, and I take it that ought to include the ->open_files list.
I would guess that a quick fix for this looks something like the patch below. The race is in put_nfs_open_context() between the point where we check the reference count and the point where the context gets removed from the open_files list. If another thread searches the list between these points and grabs a new reference, we die.
The patch holds the i_lock around the kref_put to prevent others from searching the list. Ugly, I know, but it seems that's the price you pay for using a kref in such unconventional ways, i.e. not counting every reference.
Arnd <><
--- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -487,10 +487,10 @@ static void nfs_free_open_context(struct kref *kref) if (!list_empty(&ctx->list)) { struct inode *inode = ctx->path.dentry->d_inode; - spin_lock(&inode->i_lock); list_del(&ctx->list); - spin_unlock(&inode->i_lock); } + spin_unlock(&inode->i_lock); + if (ctx->state != NULL) nfs4_close_state(&ctx->path, ctx->state, ctx->mode); if (ctx->cred != NULL) @@ -498,11 +498,15 @@ static void nfs_free_open_context(struct kref *kref) dput(ctx->path.dentry); mntput(ctx->path.mnt); kfree(ctx); + + spin_lock(&inode->i_lock); } void put_nfs_open_context(struct nfs_open_context *ctx) { + spin_lock(&inode->i_lock); kref_put(&ctx->kref, nfs_free_open_context); + spin_unlock(&inode->i_lock); } /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |