[v2,0/6] fs: Fix directory corruption when moving directories

Message

Jan Kara June 1, 2023, 10:58 a.m. UTC

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.

Changes since v1:
* Made sure lock_two_inodes() uses subclass1 for the obtained lock in case
  there is only one inode locked
* Fixes unlocked_two_nondirectories() to properly unlock inodes even if
  directories are accidentally passed in.

								Honza

[1] https://lore.kernel.org/all/20230117123735.un7wbamlbdihninm@quack3
[2] https://lore.kernel.org/all/20230517045836.GA11594@frogsfrogsfrogs

Previous versions:
Link: http://lore.kernel.org/r/[e-mail] # v1

Comments

Christian Brauner June 2, 2023, 1:05 p.m. UTC | #1

On Thu, 01 Jun 2023 12:58:20 +0200, Jan Kara wrote:
> 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.
> 
> [...]

I've picked this up to get it into -next. I've folded the following fix
for the missing { into [4/6].

---

Applied to the vfs.rename.locking branch of the vfs/vfs.git tree.
Patches in the vfs.rename.locking branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.rename.locking

[1/6] ext4: Remove ext4 locking of moved directory
      https://git.kernel.org/vfs/vfs/c/3658840cd363
[2/6] Revert "udf: Protect rename against modification of moved directory"
      https://git.kernel.org/vfs/vfs/c/7517ce5dc4d6
[3/6] Revert "f2fs: fix potential corruption when moving a directory"
      https://git.kernel.org/vfs/vfs/c/cde3c9d7e2a3
[4/6] fs: Establish locking order for unrelated directories
      https://git.kernel.org/vfs/vfs/c/f23ce7571853
[5/6] fs: Lock moved directories
      https://git.kernel.org/vfs/vfs/c/28eceeda130f
[6/6] fs: Restrict lock_two_nondirectories() to non-directory inodes
      https://git.kernel.org/vfs/vfs/c/afb4adc7c3ef

[e-mail] July 6, 2023, 12:18 a.m. UTC | #2

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Christian Brauner <[e-mail]>:

On Thu,  1 Jun 2023 12:58:20 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,v2,1/6] ext4: Remove ext4 locking of moved directory
    https://git.kernel.org/jaegeuk/f2fs/c/3658840cd363
  - [f2fs-dev,v2,2/6] Revert "udf: Protect rename against modification of moved directory"
    https://git.kernel.org/jaegeuk/f2fs/c/7517ce5dc4d6
  - [f2fs-dev,v2,3/6] Revert "f2fs: fix potential corruption when moving a directory"
    https://git.kernel.org/jaegeuk/f2fs/c/cde3c9d7e2a3
  - [f2fs-dev,v2,4/6] fs: Establish locking order for unrelated directories
    (no matching commit)
  - [f2fs-dev,v2,5/6] fs: Lock moved directories
    https://git.kernel.org/jaegeuk/f2fs/c/28eceeda130f
  - [f2fs-dev,v2,6/6] fs: Restrict lock_two_nondirectories() to non-directory inodes
    (no matching commit)

You are awesome, thank you!