| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20080605153019.77d61199.akpm@linux-foundation.org>
Date: Thu, 5 Jun 2008 15:30:19 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Ian Kent <raven@...maw.net>
Cc: torvalds@...ux-foundation.org, viro@...IV.linux.org.uk,
miklos@...redi.hu, jesper@...gh.cc, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, jmoyer@...hat.com
Subject: Re: Linux 2.6.26-rc4
On Thu, 05 Jun 2008 15:31:37 +0800
Ian Kent <raven@...maw.net> wrote:
>
> On Tue, 2008-06-03 at 08:01 -0700, Linus Torvalds wrote:
> >
> > On Tue, 3 Jun 2008, Ian Kent wrote:
> > > >
> > > > > I think it must be autofs4 doing something weird. Like this in
> > > > > autofs4_lookup_unhashed():
> > > > >
> > > > > /*
> > > > > * Make the rehashed dentry negative so the VFS
> > > > > * behaves as it should.
> > > > > */
> > > > > if (inode) {
> > > > > dentry->d_inode = NULL;
> >
> > Uhhuh. Yeah, that's not allowed.
> >
> > A dentry inode can start _out_ as NULL, but it can never later become NULL
> > again until it is totally unused.
>
> Here is a patch for autofs4 to, hopefully, resolve this.
>
> Keep in mind this doesn't address any other autofs4 issues but I it
> should allow us to identify if this was in fact the root cause of the
> problem Jesper reported.
>
> autofs4 - leave rehashed dentry positive
>
> From: Ian Kent <raven@...maw.net>
>
> Correct the error of making a positive dentry negative after it has been
> instantiated.
>
> This involves removing the code in autofs4_lookup_unhashed() that
> makes the dentry negative and updating autofs4_dir_symlink() and
> autofs4_dir_mkdir() to recognise they have been given a postive
> dentry (previously the dentry was always negative) and deal with
> it. In addition the dentry info struct initialization, autofs4_init_ino(),
> and the symlink free function, ino_lnkfree(), have been made aware
> of this possible re-use. This is needed because the current case
> re-uses a dentry in order to preserve it's flags as described in
> commit f50b6f8691cae2e0064c499dd3ef3f31142987f0.
>
> ...
>
> ...
>
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index edf5b6b..6ce603b 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -555,24 +555,8 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
> goto next;
>
> if (d_unhashed(dentry)) {
> - struct inode *inode = dentry->d_inode;
> -
> - ino = autofs4_dentry_ino(dentry);
> list_del_init(&ino->rehash);
> dget(dentry);
> - /*
> - * Make the rehashed dentry negative so the VFS
> - * behaves as it should.
> - */
> - if (inode) {
> - dentry->d_inode = NULL;
> - list_del_init(&dentry->d_alias);
> - spin_unlock(&dentry->d_lock);
> - spin_unlock(&sbi->rehash_lock);
> - spin_unlock(&dcache_lock);
> - iput(inode);
> - return dentry;
> - }
> spin_unlock(&dentry->d_lock);
> spin_unlock(&sbi->rehash_lock);
> spin_unlock(&dcache_lock);
> @@ -728,35 +712,50 @@ static int autofs4_dir_symlink(struct inode *dir,
> return -EACCES;
>
> ino = autofs4_init_ino(ino, sbi, S_IFLNK | 0555);
> - if (ino == NULL)
> + if (!ino)
> return -ENOSPC;
Should have been ENOMEM, I guess.
> - ino->size = strlen(symname);
> - ino->u.symlink = cp = kmalloc(ino->size + 1, GFP_KERNEL);
> -
> - if (cp == NULL) {
> - kfree(ino);
> + cp = kmalloc(ino->size + 1, GFP_KERNEL);
> + if (!cp) {
> + if (!dentry->d_fsdata)
> + kfree(ino);
OK, so here we work out that autofs4_init_ino() had to allocate a new
autofs_info and if so, free it here. It took me a moment..
> return -ENOSPC;
ENOMEM?
> }
>
> strcpy(cp, symname);
>
> - inode = autofs4_get_inode(dir->i_sb, ino);
> - d_add(dentry, inode);
> + inode = dentry->d_inode;
> + if (inode)
> + d_rehash(dentry);
> + else {
> + inode = autofs4_get_inode(dir->i_sb, ino);
> + if (!inode) {
> + kfree(cp);
> + if (!dentry->d_fsdata)
> + kfree(ino);
> + return -ENOSPC;
> + }
> +
> + d_add(dentry, inode);
>
> - if (dir == dir->i_sb->s_root->d_inode)
> - dentry->d_op = &autofs4_root_dentry_operations;
> - else
> - dentry->d_op = &autofs4_dentry_operations;
> + if (dir == dir->i_sb->s_root->d_inode)
> + dentry->d_op = &autofs4_root_dentry_operations;
> + else
> + dentry->d_op = &autofs4_dentry_operations;
> +
> + dentry->d_fsdata = ino;
> + ino->dentry = dentry;
> + ino->inode = inode;
> + }
> + dget(dentry);
>
> - dentry->d_fsdata = ino;
> - ino->dentry = dget(dentry);
> atomic_inc(&ino->count);
> p_ino = autofs4_dentry_ino(dentry->d_parent);
> if (p_ino && dentry->d_parent != dentry)
> atomic_inc(&p_ino->count);
> - ino->inode = inode;
>
> + ino->u.symlink = cp;
> + ino->size = strlen(symname);
> dir->i_mtime = CURRENT_TIME;
This all seems a bit ungainly. I assume that on entry to
autofs4_dir_symlink(), ino->size is equal to strlen(symname)? If it's
not, that strcpy() will overrun.
But if ino->size _is_ equal to strlen(symname) then why did we just
recalculate the same thing?
I'm suspecting we can zap a lump of code and just do
cp = kstrdup(symname, GFP_KERNEL);
Anyway, please check that.
> return 0;
> @@ -866,24 +865,38 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> dentry, dentry->d_name.len, dentry->d_name.name);
>
> ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
> - if (ino == NULL)
> + if (!ino)
> return -ENOSPC;
ENOMEM?
> - inode = autofs4_get_inode(dir->i_sb, ino);
> - d_add(dentry, inode);
> + inode = dentry->d_inode;
> + if (inode)
> + d_rehash(dentry);
> + else {
> + inode = autofs4_get_inode(dir->i_sb, ino);
> + if (!inode) {
> + if (!dentry->d_fsdata)
> + kfree(ino);
> + return -ENOSPC;
> + }
>
> - if (dir == dir->i_sb->s_root->d_inode)
> - dentry->d_op = &autofs4_root_dentry_operations;
> - else
> - dentry->d_op = &autofs4_dentry_operations;
> + d_add(dentry, inode);
> +
> + if (dir == dir->i_sb->s_root->d_inode)
> + dentry->d_op = &autofs4_root_dentry_operations;
> + else
> + dentry->d_op = &autofs4_dentry_operations;
> +
> + dentry->d_fsdata = ino;
> + ino->dentry = dentry;
> + ino->inode = inode;
> + }
> + dget(dentry);
This all looks very similar to the code in autofs4_dir_symlink(). Some
refactoring might be needed at some stage?
> - dentry->d_fsdata = ino;
> - ino->dentry = dget(dentry);
> atomic_inc(&ino->count);
> p_ino = autofs4_dentry_ino(dentry->d_parent);
> if (p_ino && dentry->d_parent != dentry)
> atomic_inc(&p_ino->count);
> - ino->inode = inode;
> +
> inc_nlink(dir);
> dir->i_mtime = CURRENT_TIME;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists