f2fs: Fix deadlock in shutdown ioctl

PATCH Superseded Sahitya Tummala 3 participants 4 messages May 17, 2018 – May 18, 2018 F2FS file system

lore

Share & tools

lore

Sahitya Tummala

author

2018-05-17 08:03 UTC #0 lore raw

PATCH v2 1/1 f2fs: Fix deadlock in shutdown ioctl

f2fs_ioc_shutdown() ioctl gets stuck in the below path when issued with F2FS_GOING_DOWN_FULLSYNC option.

__switch_to+0x90/0xc4
percpu_down_write+0x8c/0xc0
freeze_super+0xec/0x1e4
freeze_bdev+0xc4/0xcc
f2fs_ioctl+0xc0c/0x1ce0
f2fs_compat_ioctl+0x98/0x1f0

Signed-off-by Sahitya Tummala <[email protected]>

---
v2:
remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by Chao.

 fs/f2fs/file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/file.c +3 -1

index 6b94f19..5a132c9 100644
@@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 
 	switch (in) {
 	case F2FS_GOING_DOWN_FULLSYNC:
+		mnt_drop_write_file(filp);
 		sb = freeze_bdev(sb->s_bdev);
 		if (IS_ERR(sb)) {
 			ret = PTR_ERR(sb);

@@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 
 	f2fs_update_time(sbi, REQ_TIME);
 out:
-	mnt_drop_write_file(filp);
+	if (in != F2FS_GOING_DOWN_FULLSYNC)
+		mnt_drop_write_file(filp);
 	return ret;
 }

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Chao Yu

2018-05-17 11:12 UTC #1 lore raw

fs/f2fs/file.c static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)

Quoted 23 lines · Sahitya Tummala

f2fs_ioc_shutdown() ioctl gets stuck in the below path when issued with F2FS_GOING_DOWN_FULLSYNC option.
__switch_to+0x90/0xc4
percpu_down_write+0x8c/0xc0
freeze_super+0xec/0x1e4
freeze_bdev+0xc4/0xcc
f2fs_ioctl+0xc0c/0x1ce0
f2fs_compat_ioctl+0x98/0x1f0
Signed-off-by Sahitya Tummala <[email protected]>
---
v2:
remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by Chao.
 fs/f2fs/file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/f2fs/file.c +0 -0
index 6b94f19..5a132c9 100644
@@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)

How about:

if (in != F2FS_GOING_DOWN_FULLSYNC)

	mnt_want_write_file();

switch()

{
	handle command;
}

if (in != F2FS_GOING_DOWN_FULLSYNC)

	mnt_drop_write_file();

Thanks,

Quoted 18 lines

fs/f2fs/file.c +3 -1

 	switch (in) {
 	case F2FS_GOING_DOWN_FULLSYNC:
+		mnt_drop_write_file(filp);
 		sb = freeze_bdev(sb->s_bdev);
 		if (IS_ERR(sb)) {
 			ret = PTR_ERR(sb);

@@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 
 	f2fs_update_time(sbi, REQ_TIME);
 out:
-	mnt_drop_write_file(filp);
+	if (in != F2FS_GOING_DOWN_FULLSYNC)
+		mnt_drop_write_file(filp);
 	return ret;
 }

Jaegeuk Kim

maintainer

2018-05-18 01:08 UTC #2 lore raw

Quoted 31 lines · Chao Yu

fs/f2fs/file.c static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
Quoted 23 lines · Sahitya Tummala
f2fs_ioc_shutdown() ioctl gets stuck in the below path when issued with F2FS_GOING_DOWN_FULLSYNC option.
__switch_to+0x90/0xc4
percpu_down_write+0x8c/0xc0
freeze_super+0xec/0x1e4
freeze_bdev+0xc4/0xcc
f2fs_ioctl+0xc0c/0x1ce0
f2fs_compat_ioctl+0x98/0x1f0
Signed-off-by Sahitya Tummala <[email protected]>
---
v2:
remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by Chao.
 fs/f2fs/file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/f2fs/file.c +0 -0
index 6b94f19..5a132c9 100644
@@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
How about:
if (in != F2FS_GOING_DOWN_FULLSYNC)
	mnt_want_write_file();
switch()

Quoted 8 lines

{
	handle command;
}
if (in != F2FS_GOING_DOWN_FULLSYNC)
	mnt_drop_write_file();
Thanks,

if (in == F2FS_GOING_DOWN_FULLSYNC) {
	sb = freeze_bdev();
	if (IS_ERR(sb))
		return PTR_ERR(sb);
	if (unlikely(!sb))
		return -EINVAL;
}

ret = mnt_want_write_file();
...
switch() {
case F2FS_GOING_DOWN_FULLSYNC:

	f2fs_stop_checkpoint();
	dhaw_bdev();
	break;

...
}

drop:
...

Quoted 19 lines

fs/f2fs/file.c +3 -1

 	switch (in) {
 	case F2FS_GOING_DOWN_FULLSYNC:
+		mnt_drop_write_file(filp);
 		sb = freeze_bdev(sb->s_bdev);
 		if (IS_ERR(sb)) {
 			ret = PTR_ERR(sb);

@@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 
 	f2fs_update_time(sbi, REQ_TIME);
 out:
-	mnt_drop_write_file(filp);
+	if (in != F2FS_GOING_DOWN_FULLSYNC)
+		mnt_drop_write_file(filp);
 	return ret;
 }

It will be stuck/blocked here as freeze_bdev() now holds the write lock for all the cases including SB_FREEZE_WRITE. As freeze_bdev() holds the write lock, I think f2fs_stop_checkpoint() should be safe even without mnt_want_write_file().

I have posted v3 as per Chao's comments to exclude mnt_want_write_file() for F2FS_GOING_DOWN_FULLSYNC case.
Please check and let me know if there are any further comments.

Quoted 13 lines

...
switch() {
case F2FS_GOING_DOWN_FULLSYNC:
	f2fs_stop_checkpoint();
	dhaw_bdev();
	break;
...
}
drop:
...

--
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.