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

Arnd Bergmann
2007-07-26T15:23:23Z

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);
}

/*