Al, Christoph,
Zach just ran into this bug as well. Does this fix look reasonable?
thanks-
sage
----
>From Sage Weil <sage@xxxxxxxxxxxx>
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.
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.
My suspicion is that they are not bitten by this particular bug because
the incorrectly rehashed new_dentry gets rejected by d_revalidate().
This was caught by the recently posted POSIX fstest suite, rename/10.t
test 62 (and others) on ceph. 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
--
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
