Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933601Ab3GPRqe (ORCPT ); Tue, 16 Jul 2013 13:46:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15288 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933158Ab3GPRqd (ORCPT ); Tue, 16 Jul 2013 13:46:33 -0400 Message-ID: <51E58631.7060003@redhat.com> Date: Tue, 16 Jul 2013 13:43:13 -0400 From: Brian Foster User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Miklos Szeredi CC: Niels de Vos , fuse-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [fuse-devel] [PATCH] fuse: fix occasional dentry leak when readdirplus is used References: <1373893160-30601-1-git-send-email-ndevos@redhat.com> <51E456B6.8050601@redhat.com> <20130716103939.GL18803@ndevos-laptop.usersys.redhat.com> <51E54764.4070106@redhat.com> <20130716161458.GA19664@tucsk.piliscsaba.szeredi.hu> In-Reply-To: <20130716161458.GA19664@tucsk.piliscsaba.szeredi.hu> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3553 Lines: 136 On 07/16/2013 12:14 PM, 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. > Thanks Miklos. > Below patch fixes several issues with that function. Could you please give it a > go? > This patch looks good and fixes the issue for me. It might be good to give Neils a chance to beat on it as well, but otherwise: Tested-by: Brian Foster Brian > 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; > } > @@ -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; > } > > -- 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/