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
