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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



d_move() is strangely implemented in that it swaps the position of 
new_dentry and old_dentry in the namespace.  This is admittedly weird (see 
comments for d_move_locked()), but normally harmless: even though 
new_dentry swaps places with old_dentry, it is unhashed, and won't be seen 
by a subsequent lookup.

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... and the overwritten 
target inode.

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.)

The only in-tree filesystems with FS_RENAME_DOES_D_MOVE are ocfs2 and nfs.  
I haven't tested either of them... only verified correct behavior on ext3 
and ceph.  My suspicion is that they may not hit this particular bug 
because the incorrectly rehashed new_dentry gets rejected by 
d_revalidate() (not so, in my case).

This was caught by the recently posted POSIX fstest suite, rename/10.t 
test 62 (and others).  With this patch, all tests succeed.


Signed-off-by: Sage Weil <sage@xxxxxxxxxxxx>
---
 fs/namei.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- linux-2.6.25-orig/fs/namei.c	2008-04-16 19:49:44.000000000 -0700
+++ linux/fs/namei.c	2008-04-18 13:59:30.000000000 -0700
@@ -2488,17 +2488,18 @@
 		error = -EBUSY;
 	else 
 		error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
+	if (!error)
+		if (!(old_dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE))
+			d_move(old_dentry, new_dentry);
 	if (target) {
 		if (!error)
 			target->i_flags |= S_DEAD;
 		mutex_unlock(&target->i_mutex);
-		if (d_unhashed(new_dentry))
+		/* only rehash new_dentry if the rename failed */
+		if (error && d_unhashed(new_dentry))
 			d_rehash(new_dentry);
 		dput(new_dentry);
 	}
-	if (!error)
-		if (!(old_dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE))
-			d_move(old_dentry,new_dentry);
 	return error;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux