Re: [PATCH V4] fuse: Fix VM_BUG_ON_PAGE issue while accessing zero ref count page

[ppvk@codeaurora.org]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On 2020-09-14 19:02, ppvk@codeaurora.org wrote:
> On 2020-09-14 13:41, Miklos Szeredi wrote:
>> On Thu, Sep 10, 2020 at 5:42 PM <ppvk@codeaurora.org> wrote:
>>> 
>>> On 2020-09-08 16:55, Miklos Szeredi wrote:
>>> > On Tue, Sep 8, 2020 at 10:17 AM Pradeep P V K
>>> > <pragalla@qti.qualcomm.com> wrote:
>>> >>
>>> >> From: Pradeep P V K <ppvk@codeaurora.org>
>>> >>
>>> >> There is a potential race between fuse_abort_conn() and
>>> >> fuse_copy_page() as shown below, due to which VM_BUG_ON_PAGE
>>> >> crash is observed for accessing a free page.
>>> >>
>>> >> context#1:                      context#2:
>>> >> fuse_dev_do_read()              fuse_abort_conn()
>>> >> ->fuse_copy_args()               ->end_requests()
>>> >
>>> > This shouldn't happen due to FR_LOCKED logic.   Are you seeing this on
>>> > an upstream kernel?  Which version?
>>> >
>>> > Thanks,
>>> > Miklos
>>> 
>>> This is happen just after unlock_request() in fuse_ref_page(). In
>>> unlock_request(), it will clear the FR_LOCKED bit.
>>> As there is no protection between context#1 & context#2 during
>>> unlock_request(), there are chances that it could happen.
>> 
>> Ah, indeed, I missed that one.
>> 
>> Similar issue in fuse_try_move_page(), which dereferences oldpage
>> after unlock_request().
>> 
>> Fix for both is to grab a reference to the page from ap->pages[] array
>> *before* calling unlock_request().
>> 
>> Attached untested patch.   Could you please verify that it fixes the 
>> bug?
>> 
> Thanks for the patch. It is an one time issue and bit hard to
> reproduce but still we
> will verify the above proposed patch and update the test results here.
> 
Not seen any issue during 24 hours(+) of stability run with your 
proposed patch.
This covers reads/writes on fuse paths + reboots + other concurrency's.

> Minor comments on the commit text of the proposed patch : This issue
> was originally reported by me and kernel test robot
> identified compilation errors on the patch that i submitted.
> This confusion might be due to un proper commit text note on "changes 
> since v1"
> 
>> Thanks,
>> Miklos
> 
> Thanks and Regards,
> Pradeep

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fuse-fix-page-dereference-after-free.patch --]
[-- Type: text/x-diff; name=fuse-fix-page-dereference-after-free.patch, Size: 2325 bytes --]

From: Miklos Szeredi <mszeredi@redhat.com>
Subject: fuse: fix page dereference after free

After unlock_request() pages from the ap->pages[] array may be put and can
be freed.

Prevent use after free by grabbing a reference to the page before calling
unlock_request().

This was originally reported by kernel test robot and the original patch
was created by Pradeep P V K.

Reported-by: Pradeep P V K <ppvk@codeaurora.org>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dev.c |   28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -785,15 +785,16 @@ static int fuse_try_move_page(struct fus
 	struct page *newpage;
 	struct pipe_buffer *buf = cs->pipebufs;
 
+	get_page(oldpage);
 	err = unlock_request(cs->req);
 	if (err)
-		return err;
+		goto out_put_old;
 
 	fuse_copy_finish(cs);
 
 	err = pipe_buf_confirm(cs->pipe, buf);
 	if (err)
-		return err;
+		goto out_put_old;
 
 	BUG_ON(!cs->nr_segs);
 	cs->currbuf = buf;
@@ -833,7 +834,7 @@ static int fuse_try_move_page(struct fus
 	err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
 	if (err) {
 		unlock_page(newpage);
-		return err;
+		goto out_put_old;
 	}
 
 	get_page(newpage);
@@ -852,14 +853,19 @@ static int fuse_try_move_page(struct fus
 	if (err) {
 		unlock_page(newpage);
 		put_page(newpage);
-		return err;
+		goto out_put_old;
 	}
 
 	unlock_page(oldpage);
+	/* Drop ref for ap->pages[] array */
 	put_page(oldpage);
 	cs->len = 0;
 
-	return 0;
+	err = 0;
+out_put_old:
+	/* Drop ref obtained in this function */
+	put_page(oldpage);
+	return err;
 
 out_fallback_unlock:
 	unlock_page(newpage);
@@ -868,10 +874,10 @@ static int fuse_try_move_page(struct fus
 	cs->offset = buf->offset;
 
 	err = lock_request(cs->req);
-	if (err)
-		return err;
+	if (!err)
+		err = 1;
 
-	return 1;
+	goto out_put_old;
 }
 
 static int fuse_ref_page(struct fuse_copy_state *cs, struct page *page,
@@ -883,14 +889,16 @@ static int fuse_ref_page(struct fuse_cop
 	if (cs->nr_segs >= cs->pipe->max_usage)
 		return -EIO;
 
+	get_page(page);
 	err = unlock_request(cs->req);
-	if (err)
+	if (err) {
+		put_page(page);
 		return err;
+	}
 
 	fuse_copy_finish(cs);
 
 	buf = cs->pipebufs;
-	get_page(page);
 	buf->page = page;
 	buf->offset = offset;
 	buf->len = count;