Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 12 Mar 2019 13:09:32 -0700
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <yuchao0@huawei.com>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, chao@kernel.org
Subject: Re: [PATCH] f2fs: fix to avoid deadlock in f2fs_read_inline_dir()
Message-ID: <20190312200932.GD45421@jaegeuk-macbookpro.roam.corp.google.com>
References: <20190312074427.75515-1-yuchao0@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190312074427.75515-1-yuchao0@huawei.com>
User-Agent: Mutt/1.8.2 (2017-04-18)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 03/12, Chao Yu wrote:
> As Jiqun Li reported in bugzilla:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=202883
> 
> sometimes, dead lock when make system call SYS_getdents64 with fsync() is
> called by another process.
> 
> monkey running on android9.0
> 
> 1.  task 9785 held sbi->cp_rwsem and waiting lock_page()
> 2.  task 10349 held mm_sem and waiting sbi->cp_rwsem
> 3. task 9709 held lock_page() and waiting mm_sem
> 
> so this is a dead lock scenario.
> 
> task stack is show by crash tools as following
> 
> crash_arm64> bt ffffffc03c354080
> PID: 9785   TASK: ffffffc03c354080  CPU: 1   COMMAND: "RxIoScheduler-3"
> >> #7 [ffffffc01b50fac0] __lock_page at ffffff80081b11e8
> 
> crash-arm64> bt 10349
> PID: 10349  TASK: ffffffc018b83080  CPU: 1   COMMAND: "BUGLY_ASYNC_UPL"
> >> #3 [ffffffc01f8cfa40] rwsem_down_read_failed at ffffff8008a93afc
>      PC: 00000033  LR: 00000000  SP: 00000000  PSTATE: ffffffffffffffff
> 
> crash-arm64> bt 9709
> PID: 9709   TASK: ffffffc03e7f3080  CPU: 1   COMMAND: "IntentService[A"
> >> #3 [ffffffc001e67850] rwsem_down_read_failed at ffffff8008a93afc
> >> #8 [ffffffc001e67b80] el1_ia at ffffff8008084fc4
>      PC: ffffff8008274114  [compat_filldir64+120]
>      LR: ffffff80083584d4  [f2fs_fill_dentries+448]
>      SP: ffffffc001e67b80  PSTATE: 80400145
>     X29: ffffffc001e67b80  X28: 0000000000000000  X27: 000000000000001a
>     X26: 00000000000093d7  X25: ffffffc070d52480  X24: 0000000000000008
>     X23: 0000000000000028  X22: 00000000d43dfd60  X21: ffffffc001e67e90
>     X20: 0000000000000011  X19: ffffff80093a4000  X18: 0000000000000000
>     X17: 0000000000000000  X16: 0000000000000000  X15: 0000000000000000
>     X14: ffffffffffffffff  X13: 0000000000000008  X12: 0101010101010101
>     X11: 7f7f7f7f7f7f7f7f  X10: 6a6a6a6a6a6a6a6a   X9: 7f7f7f7f7f7f7f7f
>      X8: 0000000080808000   X7: ffffff800827409c   X6: 0000000080808000
>      X5: 0000000000000008   X4: 00000000000093d7   X3: 000000000000001a
>      X2: 0000000000000011   X1: ffffffc070d52480   X0: 0000000000800238
> >> #9 [ffffffc001e67be0] f2fs_fill_dentries at ffffff80083584d0
>      PC: 0000003c  LR: 00000000  SP: 00000000  PSTATE: 000000d9
>     X12: f48a02ff X11: d4678960 X10: d43dfc00  X9: d4678ae4
>      X8: 00000058  X7: d4678994  X6: d43de800  X5: 000000d9
>      X4: d43dfc0c  X3: d43dfc10  X2: d46799c8  X1: 00000000
>      X0: 00001068
> 
> Below potential deadlock will happen between three threads:
> Thread A		Thread B		Thread C
> - f2fs_do_sync_file
>  - f2fs_write_checkpoint
>   - down_write(&sbi->node_change) -- 1)
> 			- do_page_fault
> 			 - down_write(&mm->mmap_sem) -- 2)
> 			  - do_wp_page
> 			   - f2fs_vm_page_mkwrite
> 						- getdents64
> 						 - f2fs_read_inline_dir
> 						  - lock_page -- 3)
>   - f2fs_sync_node_pages
>    - lock_page -- 3)
> 			    - __do_map_lock
> 			     - down_read(&sbi->node_change) -- 1)
> 						  - f2fs_fill_dentries
> 						   - dir_emit
> 						    - compat_filldir64
> 						     - do_page_fault
> 						      - down_read(&mm->mmap_sem) -- 2)
> 
> Since f2fs_readdir is protected by inode.i_rwsem, there should not be

Well, what about other dir operations which can convert inline_dentry?
How about allocating a buffer shortly like this?

---
 fs/f2fs/inline.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index a1381d05956b..50a946d55778 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -647,12 +647,10 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx,
 	struct inode *inode = file_inode(file);
 	struct page *ipage = NULL;
 	struct f2fs_dentry_ptr d;
-	void *inline_dentry = NULL;
+	void *inline_dentry = NULL, *buf = NULL;
 	int err;
 
-	make_dentry_ptr_inline(inode, &d, inline_dentry);
-
-	if (ctx->pos == d.max)
+	if (ctx->pos >= NR_INLINE_DENTRY(inode))
 		return 0;
 
 	ipage = f2fs_get_node_page(F2FS_I_SB(inode), inode->i_ino);
@@ -660,14 +658,24 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx,
 		return PTR_ERR(ipage);
 
 	inline_dentry = inline_data_addr(inode, ipage);
+	buf = f2fs_kmalloc(F2FS_I_SB(inode),
+				MAX_INLINE_DATA(inode), GFP_F2FS_ZERO);
+	if (!buf) {
+		f2fs_put_page(ipage, 1);
+		err = -ENOMEM;
+		goto out;
+	}
+	memcpy(buf, inline_dentry, MAX_INLINE_DATA(inode));
+	make_dentry_ptr_inline(inode, &d, buf);
 
-	make_dentry_ptr_inline(inode, &d, inline_dentry);
+	f2fs_put_page(ipage, 1);
 
 	err = f2fs_fill_dentries(ctx, &d, 0, fstr);
 	if (!err)
 		ctx->pos = d.max;
 
-	f2fs_put_page(ipage, 1);
+	kvfree(buf);
+out:
 	return err < 0 ? err : 0;
 }
 
-- 
2.19.0.605.g01d371f741-goog