lkml.org 
[lkml]   [2007]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [patch] nfs: fix locking in nfs/inode.c in nfs_free_open_context
Date
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/

\
 
 \ /
  Last update: 2007-07-26 17:27    [W:0.269 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site