Message ID | 4FC5F8CC.20400@panasas.com |
---|---|
State | Not Applicable, archived |
Headers | show |
On 05/30/2012 01:39 PM, Boaz Harrosh wrote: > > When directory hierarchy is corrupted and contains cycles, d_splice_alias() can > fail. Handle the failure cleanly. > > Identical/coppied from: > ext2: Handle error from d_splice_alias() > Signed-off-by: Jan Kara <jack@suse.cz> > > [exofs is just yet another copy/paste of ext2 code] > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> Please carry in your tree, since it's dependent on [patch 1/4] Thanks Boaz > --- > fs/exofs/namei.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/fs/exofs/namei.c b/fs/exofs/namei.c > index fc7161d..82de452 100644 > --- a/fs/exofs/namei.c > +++ b/fs/exofs/namei.c > @@ -50,13 +50,19 @@ static struct dentry *exofs_lookup(struct inode *dir, struct dentry *dentry, > { > struct inode *inode; > ino_t ino; > + struct dentry *ret; > > if (dentry->d_name.len > EXOFS_NAME_LEN) > return ERR_PTR(-ENAMETOOLONG); > > ino = exofs_inode_by_name(dir, dentry); > inode = ino ? exofs_iget(dir->i_sb, ino) : NULL; > - return d_splice_alias(inode, dentry); > + ret = d_splice_alias(inode, dentry); > + if (IS_ERR(ret)) { > + EXOFS_ERR("directory #%lu corrupted", dir->i_ino); > + iput(inode); > + } > + return ret; > } > > static int exofs_create(struct inode *dir, struct dentry *dentry, umode_t mode, -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 30, 2012 at 01:39:08PM +0300, Boaz Harrosh wrote: > + ret = d_splice_alias(inode, dentry); > + if (IS_ERR(ret)) { > + EXOFS_ERR("directory #%lu corrupted", dir->i_ino); > + iput(inode); That's a bloody wrong interface. If you add d_splice_alias() failure exit like that, do iput() *there*. Requiring every caller to deal with failure exit cleanups like that is the recipe for recurring bugs. Don't Do That. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/09/2012 12:59 AM, Al Viro wrote: > On Wed, May 30, 2012 at 01:39:08PM +0300, Boaz Harrosh wrote: >> + ret = d_splice_alias(inode, dentry); >> + if (IS_ERR(ret)) { >> + EXOFS_ERR("directory #%lu corrupted", dir->i_ino); >> + iput(inode); > > That's a bloody wrong interface. If you add d_splice_alias() failure > exit like that, do iput() *there*. Requiring every caller to deal with > failure exit cleanups like that is the recipe for recurring bugs. > Don't Do That. I agree. Thanks. My point being that please any changes made to ext2, in this area please also apply to exofs, since it is just another copy/paste of ext2. I'll ACK any which way you guys decide to properly go with, as part of the VFS changes. Thanks Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 11, 2012 at 06:41:30PM +0300, Boaz Harrosh wrote: > > My point being that please any changes made to ext2, in this area please also > apply to exofs, since it is just another copy/paste of ext2. I'll ACK any > which way you guys decide to properly go with, as part of the VFS changes. Well, I already have this quick and dirty fix to address the problem in ext4. See commit 7e936b7372. If we need to make changes to all of the file systems to accomodate some new VFS abstraction, it might be worth considering whether it's easier/simpler to just put in a quick check like I did for ext4 (just so I could plug the security hole[1] quickly). [1] It's a denial of service attack for kiosks that do automounts of USB sticks; granted, it's not that big a of a security deal, but some people care about such things. Of course, if the new/changed VFS abstraction solves other problems, that's cool, but if not, sometimes a simple brute force check is better than something complicated if elegant. :-) - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 11-06-12 15:01:08, Ted Tso wrote: > On Mon, Jun 11, 2012 at 06:41:30PM +0300, Boaz Harrosh wrote: > > > > My point being that please any changes made to ext2, in this area please also > > apply to exofs, since it is just another copy/paste of ext2. I'll ACK any > > which way you guys decide to properly go with, as part of the VFS changes. > > Well, I already have this quick and dirty fix to address the problem > in ext4. See commit 7e936b7372. If we need to make changes to all of > the file systems to accomodate some new VFS abstraction, it might be > worth considering whether it's easier/simpler to just put in a quick > check like I did for ext4 (just so I could plug the security hole[1] > quickly). > > [1] It's a denial of service attack for kiosks that do automounts of > USB sticks; granted, it's not that big a of a security deal, but some > people care about such things. > > Of course, if the new/changed VFS abstraction solves other problems, > that's cool, but if not, sometimes a simple brute force check is > better than something complicated if elegant. :-) I think that fix in ext4 is fine. Just you don't catch the situation when the directory entry points e.g. to a parent and that's deadlockable trivially as well. Even if it points to some unrelated directory, you can easily deadlock rename which tries to lock both directories. So I attempted for a fix in VFS because that's the only place having enough information to be able to tell whether you are creating directory hardlink or not. Honza
diff --git a/fs/exofs/namei.c b/fs/exofs/namei.c index fc7161d..82de452 100644 --- a/fs/exofs/namei.c +++ b/fs/exofs/namei.c @@ -50,13 +50,19 @@ static struct dentry *exofs_lookup(struct inode *dir, struct dentry *dentry, { struct inode *inode; ino_t ino; + struct dentry *ret; if (dentry->d_name.len > EXOFS_NAME_LEN) return ERR_PTR(-ENAMETOOLONG); ino = exofs_inode_by_name(dir, dentry); inode = ino ? exofs_iget(dir->i_sb, ino) : NULL; - return d_splice_alias(inode, dentry); + ret = d_splice_alias(inode, dentry); + if (IS_ERR(ret)) { + EXOFS_ERR("directory #%lu corrupted", dir->i_ino); + iput(inode); + } + return ret; } static int exofs_create(struct inode *dir, struct dentry *dentry, umode_t mode,