fix crash on ocfs2_duplicate_clusters_by_page

PATCH In review Larry Chen 3 participants 4 messages Aug 29, 2018 – Aug 30, 2018 OCFS2

Share & tools

Larry Chen

author

2018-08-29 07:47 UTC #0 lore raw

PATCH 1/1 fix crash on ocfs2_duplicate_clusters_by_page

ocfs2_duplicate_clusters_by_page may crash if one of extent's pages is dirty. When a page has not been written back, it is still in dirty state. If ocfs2_duplicate_clusters_by_page is called against the
dirty page, the crash happens.

To fix this bug, we can just unlock the page and wait the page until it's not dirty.

The following is the buck trace dump:

kernel BUG at /root/code/ocfs2/refcounttree.c:2961! [exception RIP: ocfs2_duplicate_clusters_by_page+822]

__ocfs2_move_extent+0x80/0x450 [ocfs2]
? __ocfs2_claim_clusters+0x130/0x250 [ocfs2]
ocfs2_defrag_extent+0x5b8/0x5e0 [ocfs2]
__ocfs2_move_extents_range+0x2a4/0x470 [ocfs2]
ocfs2_move_extents+0x180/0x3b0 [ocfs2]
? ocfs2_wait_for_recovery+0x13/0x70 [ocfs2]
ocfs2_ioctl_move_extents+0x133/0x2d0 [ocfs2]
ocfs2_ioctl+0x253/0x640 [ocfs2]
do_vfs_ioctl+0x90/0x5f0
SyS_ioctl+0x74/0x80
do_syscall_64+0x74/0x140
entry_SYSCALL_64_after_hwframe+0x3d/0xa2

Change-log:
1. Once we founce the page is dirty, we do not wait until it's clean, but rather we use write_one_page to write it back

2. write_one_page arguments list changed, adjust and retest this patch.

Signed-off-by Larry Chen <[email protected]>

---
 fs/ocfs2/refcounttree.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

fs/ocfs2/refcounttree.c +10 -2

index 7869622af22a..d6022e04c35a 100644
@@ -2946,6 +2946,7 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 		if (map_end & (PAGE_SIZE - 1))
 			to = map_end & (PAGE_SIZE - 1);
 
+retry:
 		page = find_or_create_page(mapping, page_index, GFP_NOFS);
 		if (!page) {
 			ret = -ENOMEM;

@@ -2957,8 +2958,15 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 		 * In case PAGE_SIZE <= CLUSTER_SIZE, This page
 		 * can't be dirtied before we CoW it out.
 		 */
-		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize)
-			BUG_ON(PageDirty(page));
+		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
+			if (PageDirty(page)) {
+				/*
+				 * write_on_page will unlock the page on return
+				 */
+				ret = write_one_page(page);
+				goto retry;
+			}
+		}
 
 		if (!PageUptodate(page)) {
 			ret = block_read_full_page(page, ocfs2_get_block);

Signature · 1 line

2.13.7

Andrew Morton

2018-08-29 22:25 UTC #1 lore raw

Quoted 36 lines · On Wed, 29 Aug 2018 15:47:40 +0800 Larry Chen <[email protected]> wrote:

ocfs2_duplicate_clusters_by_page may crash if one of extent's pages is dirty. When a page has not been written back, it is still in dirty state. If ocfs2_duplicate_clusters_by_page is called against the
dirty page, the crash happens.
To fix this bug, we can just unlock the page and wait the page until it's not dirty.
The following is the buck trace dump:
kernel BUG at /root/code/ocfs2/refcounttree.c:2961! [exception RIP: ocfs2_duplicate_clusters_by_page+822]
__ocfs2_move_extent+0x80/0x450 [ocfs2]
? __ocfs2_claim_clusters+0x130/0x250 [ocfs2]
ocfs2_defrag_extent+0x5b8/0x5e0 [ocfs2]
__ocfs2_move_extents_range+0x2a4/0x470 [ocfs2]
ocfs2_move_extents+0x180/0x3b0 [ocfs2]
? ocfs2_wait_for_recovery+0x13/0x70 [ocfs2]
ocfs2_ioctl_move_extents+0x133/0x2d0 [ocfs2]
ocfs2_ioctl+0x253/0x640 [ocfs2]
do_vfs_ioctl+0x90/0x5f0
SyS_ioctl+0x74/0x80
do_syscall_64+0x74/0x140
entry_SYSCALL_64_after_hwframe+0x3d/0xa2
fs/ocfs2/refcounttree.c +1 -0
@@ -2946,6 +2946,7 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 		if (map_end & (PAGE_SIZE - 1))
 			to = map_end & (PAGE_SIZE - 1);
 
+retry:
 		page = find_or_create_page(mapping, page_index, GFP_NOFS);
 		if (!page) {
 			ret = -ENOMEM;

fs/ocfs2/refcounttree.c int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2960

Quoted 3 lines

fs/ocfs2/refcounttree.c +0 -0

@@ -2957,8 +2958,15 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 		 * In case PAGE_SIZE <= CLUSTER_SIZE, This page
 		 * can't be dirtied before we CoW it out.
 		 */

Looks sane, but the below change shows that the above comment is untrue. Can we please update the comment as well?

Quoted 14 lines

fs/ocfs2/refcounttree.c +9 -2

-		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize)
-			BUG_ON(PageDirty(page));
+		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
+			if (PageDirty(page)) {
+				/*
+				 * write_on_page will unlock the page on return
+				 */
+				ret = write_one_page(page);
+				goto retry;
+			}
+		}
 
 		if (!PageUptodate(page)) {
 			ret = block_read_full_page(page, ocfs2_get_block);

Changwei Ge

2018-08-30 02:29 UTC #2 lore raw

Hi Larry,

Besides Andrew's comments, other parts of this patch look good to me.

Thanks,

Changwei

Quoted 64 lines · Andrew Morton

Quoted 36 lines · On Wed, 29 Aug 2018 15:47:40 +0800 Larry Chen <[email protected]> wrote:
ocfs2_duplicate_clusters_by_page may crash if one of extent's pages is dirty. When a page has not been written back, it is still in dirty state. If ocfs2_duplicate_clusters_by_page is called against the
dirty page, the crash happens.
To fix this bug, we can just unlock the page and wait the page until it's not dirty.
The following is the buck trace dump:
kernel BUG at /root/code/ocfs2/refcounttree.c:2961! [exception RIP: ocfs2_duplicate_clusters_by_page+822]
__ocfs2_move_extent+0x80/0x450 [ocfs2]
? __ocfs2_claim_clusters+0x130/0x250 [ocfs2]
ocfs2_defrag_extent+0x5b8/0x5e0 [ocfs2]
__ocfs2_move_extents_range+0x2a4/0x470 [ocfs2]
ocfs2_move_extents+0x180/0x3b0 [ocfs2]
? ocfs2_wait_for_recovery+0x13/0x70 [ocfs2]
ocfs2_ioctl_move_extents+0x133/0x2d0 [ocfs2]
ocfs2_ioctl+0x253/0x640 [ocfs2]
do_vfs_ioctl+0x90/0x5f0
SyS_ioctl+0x74/0x80
do_syscall_64+0x74/0x140
entry_SYSCALL_64_after_hwframe+0x3d/0xa2
fs/ocfs2/refcounttree.c +1 -0
@@ -2946,6 +2946,7 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
  		if (map_end & (PAGE_SIZE - 1))
  			to = map_end & (PAGE_SIZE - 1);
 
+retry:
  		page = find_or_create_page(mapping, page_index, GFP_NOFS);
  		if (!page) {
  			ret = -ENOMEM;
fs/ocfs2/refcounttree.c int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2960
Quoted 3 lines
fs/ocfs2/refcounttree.c +0 -0
@@ -2957,8 +2958,15 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
  		 * In case PAGE_SIZE <= CLUSTER_SIZE, This page
  		 * can't be dirtied before we CoW it out.
  		 */
Looks sane, but the below change shows that the above comment is untrue. Can we please update the comment as well?
Quoted 4 lines
fs/ocfs2/refcounttree.c +2 -2
-		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize)
-			BUG_ON(PageDirty(page));
+		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
+			if (PageDirty(page)) {
fs/ocfs2/refcounttree.c int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2972
Quoted 10 lines
fs/ocfs2/refcounttree.c +7 -0
+				/*
+				 * write_on_page will unlock the page on return
+				 */
+				ret = write_one_page(page);
+				goto retry;
+			}
+		}
 
  		if (!PageUptodate(page)) {
  			ret = block_read_full_page(page, ocfs2_get_block);
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Larry Chen

author

2018-08-30 02:40 UTC #3 lore raw

Hi Andrew and Changwei,

Thanks for your review, I'll propose a new version with comments corrected.

Thanks,
Larry

Quoted 75 lines · Changwei Ge

Hi Larry,
Besides Andrew's comments, other parts of this patch look good to me.
Thanks,
Changwei
Quoted 64 lines · Andrew Morton
Quoted 36 lines · On Wed, 29 Aug 2018 15:47:40 +0800 Larry Chen <[email protected]> wrote:
ocfs2_duplicate_clusters_by_page may crash if one of extent's pages is dirty. When a page has not been written back, it is still in dirty state. If ocfs2_duplicate_clusters_by_page is called against the
dirty page, the crash happens.
To fix this bug, we can just unlock the page and wait the page until it's not dirty.
The following is the buck trace dump:
kernel BUG at /root/code/ocfs2/refcounttree.c:2961! [exception RIP: ocfs2_duplicate_clusters_by_page+822]
__ocfs2_move_extent+0x80/0x450 [ocfs2]
? __ocfs2_claim_clusters+0x130/0x250 [ocfs2]
ocfs2_defrag_extent+0x5b8/0x5e0 [ocfs2]
__ocfs2_move_extents_range+0x2a4/0x470 [ocfs2]
ocfs2_move_extents+0x180/0x3b0 [ocfs2]
? ocfs2_wait_for_recovery+0x13/0x70 [ocfs2]
ocfs2_ioctl_move_extents+0x133/0x2d0 [ocfs2]
ocfs2_ioctl+0x253/0x640 [ocfs2]
do_vfs_ioctl+0x90/0x5f0
SyS_ioctl+0x74/0x80
do_syscall_64+0x74/0x140
entry_SYSCALL_64_after_hwframe+0x3d/0xa2
fs/ocfs2/refcounttree.c +1 -0
@@ -2946,6 +2946,7 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
   		if (map_end & (PAGE_SIZE - 1))
   			to = map_end & (PAGE_SIZE - 1);
 
+retry:
   		page = find_or_create_page(mapping, page_index, GFP_NOFS);
   		if (!page) {
   			ret = -ENOMEM;
fs/ocfs2/refcounttree.c int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2960
Quoted 3 lines
fs/ocfs2/refcounttree.c +0 -0
@@ -2957,8 +2958,15 @@ int ocfs2_duplicate_clusters_by_page(handle_t *handle,
   		 * In case PAGE_SIZE <= CLUSTER_SIZE, This page
   		 * can't be dirtied before we CoW it out.
   		 */
Looks sane, but the below change shows that the above comment is untrue. Can we please update the comment as well?
Quoted 4 lines
fs/ocfs2/refcounttree.c +2 -2
-		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize)
-			BUG_ON(PageDirty(page));
+		if (PAGE_SIZE <= OCFS2_SB(sb)->s_clustersize) {
+			if (PageDirty(page)) {
fs/ocfs2/refcounttree.c int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2972
Quoted 10 lines
fs/ocfs2/refcounttree.c +7 -0
+				/*
+				 * write_on_page will unlock the page on return
+				 */
+				ret = write_one_page(page);
+				goto retry;
+			}
+		}
 
   		if (!PageUptodate(page)) {
   			ret = block_read_full_page(page, ocfs2_get_block);
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel