lkml.org 
[lkml]   [2013]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [fuse-devel] [PATCH] fuse: fix occasional dentry leak when readdirplus is used
    On Tue, Jul 16, 2013 at 06:14:58PM +0200, Miklos Szeredi wrote:
    > On Tue, Jul 16, 2013 at 09:15:16AM -0400, Brian Foster wrote:
    >
    > > I'm not sure why it would need to have a valid inode. A dentry with a
    > > NULL inode is valid, no?
    >
    > It is valid, yes. It's called a "negative" dentry, which caches the information
    > that the file does not exist.
    >
    > > I think the question is whether the above state (multiple, hashed dentries)
    > > can be valid for whatever reason. It certainly looks suspicious.
    >
    > That state is not valid. So we certainly need to unhash the negative dentry
    > first, before hashing a new one. We could also reuse the old dentry, but that
    > is just an optimization.
    >
    > Below patch fixes several issues with that function. Could you please give it a
    > go?

    Thanks Miklos! This looks much cleaner and the additional error checking
    surely reduces the chance of any further problems. I've added only one
    little comment in the patch below.

    I can confirm that your patch fixes the original panic/BUG() with both
    Brian's test-case and the GlusterFS regression tests.

    Feel free to add my Tested-by/Reviewed-by signoff for this patch.

    Cheers,
    Niels


    >
    > Thanks,
    > Miklos
    >
    >
    > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
    > index 0eda527..a8208c5 100644
    > --- a/fs/fuse/dir.c
    > +++ b/fs/fuse/dir.c
    > @@ -1223,28 +1223,45 @@ static int fuse_direntplus_link(struct file *file,
    > if (name.name[1] == '.' && name.len == 2)
    > return 0;
    > }
    > +
    > + if (invalid_nodeid(o->nodeid))
    > + return -EIO;
    > + if (!fuse_valid_type(o->attr.mode))
    > + return -EIO;
    > +
    > fc = get_fuse_conn(dir);
    >
    > name.hash = full_name_hash(name.name, name.len);
    > dentry = d_lookup(parent, &name);
    > - if (dentry && dentry->d_inode) {
    > + if (dentry) {
    > inode = dentry->d_inode;
    > - if (get_node_id(inode) == o->nodeid) {
    > + if (!inode) {
    > + d_drop(dentry);
    > + } else if (get_node_id(inode) != o->nodeid ||
    > + ((o->attr.mode ^ inode->i_mode) & S_IFMT)) {
    > + err = d_invalidate(dentry);
    > + if (err)
    > + goto out;
    > + } else if (is_bad_inode(inode)) {
    > + err = -EIO;
    > + goto out;
    > + } else {
    > struct fuse_inode *fi;
    > fi = get_fuse_inode(inode);
    > spin_lock(&fc->lock);
    > fi->nlookup++;
    > spin_unlock(&fc->lock);
    >
    > + fuse_change_attributes(inode, &o->attr,
    > + entry_attr_timeout(o),
    > + attr_version);
    > +
    > /*
    > * The other branch to 'found' comes via fuse_iget()
    > * which bumps nlookup inside
    > */
    > goto found;
    > }
    > - err = d_invalidate(dentry);
    > - if (err)
    > - goto out;
    > dput(dentry);
    > dentry = NULL;

    You might want to remove that 'dentry = NULL' line, as the result of
    d_alloc() is assigned to dentry on line below the if-statement.

    > }
    > @@ -1259,25 +1276,30 @@ static int fuse_direntplus_link(struct file *file,
    > if (!inode)
    > goto out;
    >
    > - alias = d_materialise_unique(dentry, inode);
    > - err = PTR_ERR(alias);
    > - if (IS_ERR(alias))
    > - goto out;
    > + if (S_ISDIR(inode->i_mode)) {
    > + mutex_lock(&fc->inst_mutex);
    > + alias = fuse_d_add_directory(dentry, inode);
    > + mutex_unlock(&fc->inst_mutex);
    > + err = PTR_ERR(alias);
    > + if (IS_ERR(alias)) {
    > + iput(inode);
    > + goto out;
    > + }
    > + } else {
    > + alias = d_splice_alias(inode, dentry);
    > + }
    > +
    > if (alias) {
    > dput(dentry);
    > dentry = alias;
    > }
    >
    > found:
    > - fuse_change_attributes(inode, &o->attr, entry_attr_timeout(o),
    > - attr_version);
    > -
    > fuse_change_entry_timeout(dentry, o);
    >
    > err = 0;
    > out:
    > - if (dentry)
    > - dput(dentry);
    > + dput(dentry);
    > return err;
    > }
    >


    \
     
     \ /
      Last update: 2013-07-17 14:01    [W:4.083 / U:0.304 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site