f2fs: call unlock_new_inode() before d_instantiate()

PATCH In review Eric Biggers 2 participants 2 messages Apr 18, 2018 – Apr 23, 2018

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Eric Biggers

author

2018-04-18 22:49 UTC #0 lore raw

PATCH 1/1 f2fs: call unlock_new_inode() before d_instantiate()

From: Eric Biggers <[email protected]>

xfstest generic/429 sometimes hangs on f2fs, caused by a thread being unable to take a directory's i_rwsem for write in vfs_rmdir(). In the test, one thread repeatedly creates and removes a directory, and other threads repeatedly look up a file in the directory. The bug is that f2fs_mkdir() calls d_instantiate() before unlock_new_inode(), resulting in the directory inode being exposed to lookups before it has been fully initialized. And with CONFIG_DEBUG_LOCK_ALLOC, unlock_new_inode() reinitializes ->i_rwsem, corrupting its state when it is already held.

Fix it by calling unlock_new_inode() before d_instantiate(). This matches what other filesystems do.

Fixes 57397d86c62d ("f2fs: add inode operations for special inodes")

Signed-off-by Eric Biggers <[email protected]>

---
 fs/f2fs/namei.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/f2fs/namei.c +4 -4

index d5098efe577c..3a7ed962d2f7 100644
@@ -294,8 +294,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 
 	alloc_nid_done(sbi, ino);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 
 	if (IS_DIRSYNC(dir))
 		f2fs_sync_fs(sbi->sb, 1);

@@ -597,8 +597,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 	err = page_symlink(inode, disk_link.name, disk_link.len);
 
 err_out:
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 
 	/*
 	 * Let's flush symlink data in order to avoid broken symlink as much as

@@ -661,8 +661,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 
 	alloc_nid_done(sbi, inode->i_ino);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 
 	if (IS_DIRSYNC(dir))
 		f2fs_sync_fs(sbi->sb, 1);

@@ -713,8 +713,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
 
 	alloc_nid_done(sbi, inode->i_ino);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 
 	if (IS_DIRSYNC(dir))
 		f2fs_sync_fs(sbi->sb, 1);

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Chao Yu

Reviewed-by on v1 1/1

2018-04-23 03:25 UTC #1 lore raw

Quoted 11 lines · Eric Biggers

From: Eric Biggers <[email protected]>
xfstest generic/429 sometimes hangs on f2fs, caused by a thread being unable to take a directory's i_rwsem for write in vfs_rmdir(). In the test, one thread repeatedly creates and removes a directory, and other threads repeatedly look up a file in the directory. The bug is that f2fs_mkdir() calls d_instantiate() before unlock_new_inode(), resulting in the directory inode being exposed to lookups before it has been fully initialized. And with CONFIG_DEBUG_LOCK_ALLOC, unlock_new_inode() reinitializes ->i_rwsem, corrupting its state when it is already held.

Quoted 5 lines

Fix it by calling unlock_new_inode() before d_instantiate(). This matches what other filesystems do.
Fixes 57397d86c62d ("f2fs: add inode operations for special inodes")
Signed-off-by Eric Biggers <[email protected]>

Reviewed-by Chao Yu <[email protected]>

Thanks,