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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
