Re: Linux 2.6.26-rc4

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 05 Jun 2008 15:31:37 +0800
Ian Kent <raven@themaw.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@themaw.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;
>  
>