Re: [PATCH] vfs: fix vfs_rename_dir for FS_RENAME_DOES_D_MOVE filesystems

[Sage Weil <sage@newdream.net>]

On Fri, 11 Jul 2008, Miklos Szeredi wrote:
> On Fri, 11 Jul 2008, Sage Weil wrote:
> > However, vfs_rename_dir() doesn't properly account for filesystems with 
> > FS_RENAME_DOES_D_MOVE.  If new_dentry has a target inode attached, it 
> > unhashes the new_dentry prior to the rename() iop and rehashes it after, 
> > but doesn't account for the possibility that rename() may have swapped 
> > {old,new}_dentry.  For FS_RENAME_DOES_D_MOVE filesystems, it rehashes 
> > new_dentry (now the old renamed-from name, which d_move() expected to go 
> > away), such that a subsequent lookup will find it.
> > 
> > To correct this, move vfs_rename_dir()'s call to d_move() _before_ the 
> > target inode mutex is dealt with.  Since d_move() will have been called 
> > for all filesystems at this point, there is no need to rehash new_dentry 
> > unless the rename failed.  (If the rename succeeded, old_dentry should 
> > already be rehashed in the new location.)
> 
> I think rehashing the new dentry is bogus, even on error.  And it
> looks racy with lookup as well.
> 
> I wonder what the original reason for that was?  Git history doesn't
> tell...

I assume just to leave the dentry in the same stat we originally found it 
in?  

> So a better fix would be just to remove the rehashing completely.
> Does the below patch work for you?

This would work as well, yeah.  I've no real preference, here...

thanks-
sage


> 
> Thanks,
> Miklos
> 
> ---
>  fs/namei.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> Index: linux-2.6/fs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/namei.c	2008-07-11 22:09:32.000000000 +0200
> +++ linux-2.6/fs/namei.c	2008-07-11 22:40:16.000000000 +0200
> @@ -2643,8 +2643,6 @@ static int vfs_rename_dir(struct inode *
>  		if (!error)
>  			target->i_flags |= S_DEAD;
>  		mutex_unlock(&target->i_mutex);
> -		if (d_unhashed(new_dentry))
> -			d_rehash(new_dentry);
>  		dput(new_dentry);
>  	}
>  	if (!error)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>