[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page

Yunlong Song
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page
contents all the time") pointed out that "sometimes it was reported that
its contents was missing", so it checks the page's mapping and contents.
When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the
caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs:
clean up node page updating flow") moves "nid != nid_of_node(page)" test
to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a
wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it
was reported that its contents was missing" happens.

This patch restores to check node page contents all the time, and
returns the errno to make the caller known something is wrong and avoid
to use the page. This patch also moves f2fs_bug_on to its proper location.

Signed-off-by: Yunlong Song <[hidden email]>
---
 fs/f2fs/node.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1f21aae..4b04af4 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1140,16 +1140,16 @@ repeat:
  if (err < 0) {
  f2fs_put_page(page, 1);
  return ERR_PTR(err);
- } else if (err == LOCKED_PAGE) {
- goto page_hit;
+ } else if (err != LOCKED_PAGE) {
+ lock_page(page);
  }
 
  if (parent)
  ra_node_pages(parent, start + 1, MAX_RA_NODE);
 
- lock_page(page);
-
- if (unlikely(!PageUptodate(page))) {
+ if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) {
+ f2fs_bug_on(sbi, 1);
+ ClearPageUptodate(page);
  f2fs_put_page(page, 1);
  return ERR_PTR(-EIO);
  }
@@ -1157,8 +1157,6 @@ repeat:
  f2fs_put_page(page, 1);
  goto repeat;
  }
-page_hit:
- f2fs_bug_on(sbi, nid != nid_of_node(page));
  return page;
 }
 
--
1.8.5.2

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page

Jaegeuk Kim-2
Hi Yunlong,

Do we have a bug report in terms of this?

Thanks,

On Wed, May 25, 2016 at 09:01:01PM +0800, Yunlong Song wrote:

> Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page
> contents all the time") pointed out that "sometimes it was reported that
> its contents was missing", so it checks the page's mapping and contents.
> When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the
> caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs:
> clean up node page updating flow") moves "nid != nid_of_node(page)" test
> to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a
> wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it
> was reported that its contents was missing" happens.
>
> This patch restores to check node page contents all the time, and
> returns the errno to make the caller known something is wrong and avoid
> to use the page. This patch also moves f2fs_bug_on to its proper location.
>
> Signed-off-by: Yunlong Song <[hidden email]>
> ---
>  fs/f2fs/node.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 1f21aae..4b04af4 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1140,16 +1140,16 @@ repeat:
>   if (err < 0) {
>   f2fs_put_page(page, 1);
>   return ERR_PTR(err);
> - } else if (err == LOCKED_PAGE) {
> - goto page_hit;
> + } else if (err != LOCKED_PAGE) {
> + lock_page(page);
>   }
>  
>   if (parent)
>   ra_node_pages(parent, start + 1, MAX_RA_NODE);
>  
> - lock_page(page);
> -
> - if (unlikely(!PageUptodate(page))) {
> + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) {
> + f2fs_bug_on(sbi, 1);
> + ClearPageUptodate(page);
>   f2fs_put_page(page, 1);
>   return ERR_PTR(-EIO);
>   }
> @@ -1157,8 +1157,6 @@ repeat:
>   f2fs_put_page(page, 1);
>   goto repeat;
>   }
> -page_hit:
> - f2fs_bug_on(sbi, nid != nid_of_node(page));
>   return page;
>  }
>  
> --
> 1.8.5.2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2] f2fs: Return the errno to the caller to avoid using a wrong page

Yunlong Song
In reply to this post by Yunlong Song
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page
contents all the time") pointed out that "sometimes it was reported that
its contents was missing", so it checks the page's mapping and contents.
When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the
caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs:
clean up node page updating flow") moves "nid != nid_of_node(page)" test
to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a
wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it
was reported that its contents was missing" happens.

This patch restores to check node page contents all the time, and
returns the errno to make the caller known something is wrong and avoid
to use the page. This patch also moves f2fs_bug_on to its proper location.

Signed-off-by: Yunlong Song <[hidden email]>
---
 fs/f2fs/node.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1f21aae..eee56aa 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1149,16 +1149,21 @@ repeat:
 
  lock_page(page);
 
- if (unlikely(!PageUptodate(page))) {
- f2fs_put_page(page, 1);
- return ERR_PTR(-EIO);
- }
+ if (unlikely(!PageUptodate(page)))
+ goto out_err;
+
  if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
  f2fs_put_page(page, 1);
  goto repeat;
  }
 page_hit:
- f2fs_bug_on(sbi, nid != nid_of_node(page));
+ if(nid != nid_of_node(page)) {
+ f2fs_bug_on(sbi, 1);
+ ClearPageUptodate(page);
+out_err:
+ f2fs_put_page(page, 1);
+ return ERR_PTR(-EIO);
+ }
  return page;
 }
 
--
1.8.5.2

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] f2fs: Return the errno to the caller to avoid using a wrong page

Yunlong Song

>  page_hit:
> - f2fs_bug_on(sbi, nid != nid_of_node(page));
> + if(nid != nid_of_node(page)) {

should add "unlikely" here, will fix this in v3 patch.

> + f2fs_bug_on(sbi, 1);
> + ClearPageUptodate(page);
> +out_err:
> + f2fs_put_page(page, 1);
> + return ERR_PTR(-EIO);
> + }
>   return page;
>  }
>  
>


--
Thanks,
Yunlong Song

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3] f2fs: Return the errno to the caller to avoid using a wrong page

Yunlong Song
In reply to this post by Yunlong Song
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page
contents all the time") pointed out that "sometimes it was reported that
its contents was missing", so it checks the page's mapping and contents.
When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the
caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs:
clean up node page updating flow") moves "nid != nid_of_node(page)" test
to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a
wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it
was reported that its contents was missing" happens.

This patch restores to check node page contents all the time, and
returns the errno to make the caller known something is wrong and avoid
to use the page. This patch also moves f2fs_bug_on to its proper location.

Signed-off-by: Yunlong Song <[hidden email]>
---
 fs/f2fs/node.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1f21aae..fdd65ad 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1149,16 +1149,21 @@ repeat:
 
  lock_page(page);
 
- if (unlikely(!PageUptodate(page))) {
- f2fs_put_page(page, 1);
- return ERR_PTR(-EIO);
- }
+ if (unlikely(!PageUptodate(page)))
+ goto out_err;
+
  if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
  f2fs_put_page(page, 1);
  goto repeat;
  }
 page_hit:
- f2fs_bug_on(sbi, nid != nid_of_node(page));
+ if(unlikely(nid != nid_of_node(page))) {
+ f2fs_bug_on(sbi, 1);
+ ClearPageUptodate(page);
+out_err:
+ f2fs_put_page(page, 1);
+ return ERR_PTR(-EIO);
+ }
  return page;
 }
 
--
1.8.5.2

Loading...