Hello,
this patch set fixes a problem with cross directory renames originally reported
in [1]. To quickly sum it up some filesystems (so far we know at least about
ext4, udf, f2fs, ocfs2, likely also reiserfs, gfs2 and others) need to lock the
directory when it is being renamed into another directory. This is because we
need to update the parent pointer in the directory in that case and if that
races with other operation on the directory (in particular a conversion from
one directory format into another), bad things can happen.
So far we've done the locking in the filesystem code but recently Darrick
pointed out [2] that we've missed the RENAME_EXCHANGE case in our ext4 fix.
That one is particularly nasty because RENAME_EXCHANGE can arbitrarily mix
regular files and directories and proper lock ordering is not achievable in the
filesystems alone.
This patch set adds locking into vfs_rename() so that not only parent
directories but also moved inodes (regardless whether they are directories or
not) are locked when calling into the filesystem.
Honza
[1] https://lore.kernel.org/all/20230117123735.un7wbamlbdihninm@quack3
[2] https://lore.kernel.org/all/20230517045836.GA11594@frogsfrogsfrogs
This reverts commit f950fd0529130a617b3da526da9fb6a896ce87c2. The
locking is going to be provided by vfs_rename() in the following
patches.
CC: [email protected]
Signed-off-by: Jan Kara <[email protected]>
---
fs/udf/namei.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index fd20423d3ed2..fd29a66e7241 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -793,11 +793,6 @@ static int udf_rename(struct mnt_idmap *idmap, struct inode *old_dir,
if (!empty_dir(new_inode))
goto out_oiter;
}
- /*
- * We need to protect against old_inode getting converted from
- * ICB to normal directory.
- */
- inode_lock_nested(old_inode, I_MUTEX_NONDIR2);
retval = udf_fiiter_find_entry(old_inode, &dotdot_name,
&diriter);
if (retval == -ENOENT) {
@@ -806,10 +801,8 @@ static int udf_rename(struct mnt_idmap *idmap, struct inode *old_dir,
old_inode->i_ino);
retval = -EFSCORRUPTED;
}
- if (retval) {
- inode_unlock(old_inode);
+ if (retval)
goto out_oiter;
- }
has_diriter = true;
tloc = lelb_to_cpu(diriter.fi.icb.extLocation);
if (udf_get_lb_pblock(old_inode->i_sb, &tloc, 0) !=
@@ -889,7 +882,6 @@ static int udf_rename(struct mnt_idmap *idmap, struct inode *old_dir,
udf_dir_entry_len(&diriter.fi));
udf_fiiter_write_fi(&diriter, NULL);
udf_fiiter_release(&diriter);
- inode_unlock(old_inode);
inode_dec_link_count(old_dir);
if (new_inode)
@@ -901,10 +893,8 @@ static int udf_rename(struct mnt_idmap *idmap, struct inode *old_dir,
}
return 0;
out_oiter:
- if (has_diriter) {
+ if (has_diriter)
udf_fiiter_release(&diriter);
- inode_unlock(old_inode);
- }
udf_fiiter_release(&oiter);
return retval;
--
2.35.3
Remove locking of moved directory in ext4_rename2(). We will take care
of it in VFS instead. This effectively reverts commit 0813299c586b
("ext4: Fix possible corruption when moving a directory") and followup
fixes.
CC: Ted Tso <[email protected]>
CC: [email protected]
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/namei.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 45b579805c95..0caf6c730ce3 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3834,19 +3834,10 @@ static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir,
return retval;
}
- /*
- * We need to protect against old.inode directory getting converted
- * from inline directory format into a normal one.
- */
- if (S_ISDIR(old.inode->i_mode))
- inode_lock_nested(old.inode, I_MUTEX_NONDIR2);
-
old.bh = ext4_find_entry(old.dir, &old.dentry->d_name, &old.de,
&old.inlined);
- if (IS_ERR(old.bh)) {
- retval = PTR_ERR(old.bh);
- goto unlock_moved_dir;
- }
+ if (IS_ERR(old.bh))
+ return PTR_ERR(old.bh);
/*
* Check for inode number is _not_ due to possible IO errors.
@@ -4043,10 +4034,6 @@ static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir,
brelse(old.bh);
brelse(new.bh);
-unlock_moved_dir:
- if (S_ISDIR(old.inode->i_mode))
- inode_unlock(old.inode);
-
return retval;
}
--
2.35.3
On Thu, May 25, 2023 at 12:16:06PM +0200, Jan Kara wrote:
> Hello,
>
> this patch set fixes a problem with cross directory renames originally reported
> in [1]. To quickly sum it up some filesystems (so far we know at least about
> ext4, udf, f2fs, ocfs2, likely also reiserfs, gfs2 and others) need to lock the
> directory when it is being renamed into another directory. This is because we
> need to update the parent pointer in the directory in that case and if that
> races with other operation on the directory (in particular a conversion from
> one directory format into another), bad things can happen.
>
> So far we've done the locking in the filesystem code but recently Darrick
> pointed out [2] that we've missed the RENAME_EXCHANGE case in our ext4 fix.
> That one is particularly nasty because RENAME_EXCHANGE can arbitrarily mix
> regular files and directories and proper lock ordering is not achievable in the
> filesystems alone.
>
> This patch set adds locking into vfs_rename() so that not only parent
> directories but also moved inodes (regardless whether they are directories or
> not) are locked when calling into the filesystem.
This locking is trauma inducing.
So I was staring at this for a long time and the thing that bothered me
big time was that I couldn't easily figure out how we ended up with the
locking scheme that we have. So I went digging. Corrections and
additions very welcome...
Before 914a6e0ea12 ("Import 2.3.51pre1") locking for rename was based on
s_vfs_rename_sem and i_sem. For both within- and across-directory
renames s_vfs_rename_sem was acquired and the i_sem on the parent
directories was acquired but the target wasn't locked.
Then 914a6e0ea12 ("Import 2.3.51pre1") introduced an additional i_zombie
semaphore to protect against create, link, mknod, mkdir, unlink, and
rmdir on the target. So i_zombie had to be acquired during
vfs_rename_dir() on both parent and the victim but only if the source
was a directory. Back then RENAME_EXCHANGE didn't exist so if source was
a directory then target if it existed must've been a directory as well.
The original reasoning behind only locking the target if the source was
a directory was based on some filesystems not being able to deal with
opened but unlinked directories.
The i_zombie finally died in 1b3d7c93c6d ("[PATCH] (2.5.4) death of
->i_zombie") and a new locking scheme was introduced. The
s_vfs_rename_sem was now only used for across-directory renames. Instead
of having i_zombie and i_sem only i_sem was left. Now, both
vfs_rename_dir(/* if renaming directory */) and vfs_rename_other()
grabbed i_sem on both parents and on the target. So now the target was
always explicitly protected against a concurrent unlink or rmdir
as that would be done as part of the rename operation and that race
would just been awkward to allow afaict. Probably always was.
The locking of source however is a different story. This was introduced
in 6cedba8962f4 ("vfs: take i_mutex on renamed file") to prevent new
delegations from being acquired during rename, link, or unlink. So now
both source and target were locked. The target seemingly because it
would be unlinked in the filesystem's rename method and the source to
prevent new delegations from being acquired. So, since leases can only
be taken on regular files vfs_setlease()/generic_setlease() directories
were never considered for locking. So the lock never had to be acquired
for source directories.
So in any case, under the assumption that the broad strokes are correct
there seems to be no inherent reason why locking source and target if
they're directories couldn't be done if the ordering is well-defined.
Which is what originally made me hesitate. IOW, given my current
knowledge this seems ok.
Frankly, if we end up unconditionally locking source and target we're in
a better place from a pure maintainability perspective as far as I'm
concerned. Even if we end up taking the lock pointlessly for e.g., NFS
with RENAME_EXCHANGE. The last thing we need is more conditions on when
things are locked and why.
/me goes off into the weekend
On Fri, May 26, 2023 at 05:58:14PM +0200, Christian Brauner wrote:
> On Thu, May 25, 2023 at 12:16:06PM +0200, Jan Kara wrote:
> > Hello,
> >
> > this patch set fixes a problem with cross directory renames originally reported
> > in [1]. To quickly sum it up some filesystems (so far we know at least about
> > ext4, udf, f2fs, ocfs2, likely also reiserfs, gfs2 and others) need to lock the
> > directory when it is being renamed into another directory. This is because we
> > need to update the parent pointer in the directory in that case and if that
> > races with other operation on the directory (in particular a conversion from
> > one directory format into another), bad things can happen.
> >
> > So far we've done the locking in the filesystem code but recently Darrick
> > pointed out [2] that we've missed the RENAME_EXCHANGE case in our ext4 fix.
> > That one is particularly nasty because RENAME_EXCHANGE can arbitrarily mix
> > regular files and directories and proper lock ordering is not achievable in the
> > filesystems alone.
> >
> > This patch set adds locking into vfs_rename() so that not only parent
> > directories but also moved inodes (regardless whether they are directories or
> > not) are locked when calling into the filesystem.
>
> This locking is trauma inducing.
>
> So I was staring at this for a long time and the thing that bothered me
> big time was that I couldn't easily figure out how we ended up with the
> locking scheme that we have. So I went digging. Corrections and
> additions very welcome...
This kept being stuck in the back of my mind so a few
additions/corrections.
>
> Before 914a6e0ea12 ("Import 2.3.51pre1") locking for rename was based on
> s_vfs_rename_sem and i_sem. For both within- and across-directory
> renames s_vfs_rename_sem was acquired and the i_sem on the parent
> directories was acquired but the target wasn't locked.
>
> Then 914a6e0ea12 ("Import 2.3.51pre1") introduced an additional i_zombie
> semaphore to protect against create, link, mknod, mkdir, unlink, and
> rmdir on the target. So i_zombie had to be acquired during
If I reconstructed this correctly, then the motivation behind the
introduction of i_zombie was that locking order for i_sem was solely
based on pointer order before 1b3d7c93c6d ("[PATCH] (2.5.4"). So when it
turned out that an existing directory that was replaced during a rename
had to be locked it would've meant deadlocks:
mv /a/b /a/c/d rmdir /a/c/d
pointer order: a < c pointer order: d > c
inode_lock(a->i_sem)
inode_lock(c->i_sem) inode_lock(d->i_sem)
// acquired separately
// after lock on parents
inode_lock(d->i_sem) inode_lock(c->i_sem)
and the immediate fix was to introduce a separate i_zombie mutex that
was used to protect against concurrent removals of the target directory.
> vfs_rename_dir() on both parent and the victim but only if the source
> was a directory. Back then RENAME_EXCHANGE didn't exist so if source was
> a directory then target if it existed must've been a directory as well.
>
> The original reasoning behind only locking the target if the source was
> a directory was based on some filesystems not being able to deal with
> opened but unlinked directories.
>
> The i_zombie finally died in 1b3d7c93c6d ("[PATCH] (2.5.4) death of
> ->i_zombie") and a new locking scheme was introduced. The
And that locking scheme realized that the topological ordering of the
subtrees we're operating on makes it possible to lock in ancestor
order. IOW, lock parent directories before child directories and if no
ancestor relationship exists lock by pointer ordering. This allowed to
kill i_zombie...
> s_vfs_rename_sem was now only used for across-directory renames. Instead
> of having i_zombie and i_sem only i_sem was left. Now, both
> vfs_rename_dir(/* if renaming directory */) and vfs_rename_other()
> grabbed i_sem on both parents and on the target. So now the target was
> always explicitly protected against a concurrent unlink or rmdir
> as that would be done as part of the rename operation and that race
> would just been awkward to allow afaict. Probably always was.
I think the main worry making the target directory locking necessary
during rename were also scenarios like the following. Because an
existing target directory that was about to be removed during rename
wasn't locked - in contrast to an explicit rmdir - it would've meant
that the following race should've been possible:
mv /a/b /a/c/d touch /a/c/d/e
inode_lock(a->i_sem)
inode_lock(c->i_sem)
// now we're removing d // lock on d isn't held by rename
inode_lock(d->i_sem)
// add new file into d
iow, this would be racing adding a new link to the directory that's
in the process of being removed.
>
> The locking of source however is a different story. This was introduced
> in 6cedba8962f4 ("vfs: take i_mutex on renamed file") to prevent new
> delegations from being acquired during rename, link, or unlink. So now
> both source and target were locked. The target seemingly because it
> would be unlinked in the filesystem's rename method and the source to
> prevent new delegations from being acquired. So, since leases can only
> be taken on regular files vfs_setlease()/generic_setlease() directories
> were never considered for locking. So the lock never had to be acquired
> for source directories.
>
> So in any case, under the assumption that the broad strokes are correct
> there seems to be no inherent reason why locking source and target if
> they're directories couldn't be done if the ordering is well-defined.
> Which is what originally made me hesitate. IOW, given my current
> knowledge this seems ok.
>
> Frankly, if we end up unconditionally locking source and target we're in
> a better place from a pure maintainability perspective as far as I'm
> concerned. Even if we end up taking the lock pointlessly for e.g., NFS
> with RENAME_EXCHANGE. The last thing we need is more conditions on when
> things are locked and why.
>
> /me goes off into the weekend