ext4: fix journal ref count in move_extent_par_page

PATCH In review Peng Tao 2 participants 10 messages Aug 2, 2009 – Aug 10, 2009 ext4 file system

Share & tools

Peng Tao

author

2009-08-02 11:43 UTC #0 lore raw

PATCH 1/2 ext4: fix journal ref count in move_extent_par_page

move_extent_par_page calls a_ops->write_begin() to increase journal handler's reference count. However, if either mext_replace_branches() or ext4_get_block fails, the increased reference count isn't decreased. This will cause later umounting of the fs forever hangs. The patch addresses the issue by calling ext4_journal_stop() if page is not NULL (which means a_ops->write_end() isn't invoked).

Signed-off-by Peng Tao <[email protected]>

---
 fs/ext4/move_extent.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/ext4/move_extent.c +1 -0

index bbf2dd9..5821e0b 100644
@@ -871,6 +871,7 @@ out:
 		if (PageLocked(page))
 			unlock_page(page);
 		page_cache_release(page);
+		ext4_journal_stop(handle);
 	}
 out2:
 	ext4_journal_stop(handle);

Signature · 1 line

1.6.2.GIT

Peng Tao

author

2009-08-02 11:43 UTC #1 lore raw

PATCH 2/2 ext4: fix null pointer bug in move_extent.c

In mext_replace_branches(), the oext and dext virable might be NULL if the orig extent and donor extent are empty. Later calling *oext and *dext will trigger a kernel null pointer bug like this:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0636563>] mext_replace_branches+0x10c/0x2f3 [ext4]
PGD 37dba067 PUD cd8ac067 PMD 0
Oops: 0000 [#1] SMP

The patch checks the two virables and returns EOPNOTSUPP if either of them is NULL.

Signed-off-by Peng Tao <[email protected]>

---
 fs/ext4/move_extent.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

fs/ext4/move_extent.c +12 -0

index 5821e0b..4923f70 100644
@@ -641,10 +641,22 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
 		goto out;
 	depth = ext_depth(orig_inode);
 	oext = orig_path[depth].p_ext;
+	if (oext == NULL) {
+		ext4_debug("ext4 move extent: "
+					"orig extents should not be empty\n");
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 	tmp_oext = *oext;
 
 	depth = ext_depth(donor_inode);
 	dext = donor_path[depth].p_ext;
+	if (dext == NULL) {
+		ext4_debug("ext4 move extent: "
+					"donor extents should not be empty\n");
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 	tmp_dext = *dext;
 
 	mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,

Signature · 1 line

1.6.2.GIT

Akira Fujita

2009-08-03 08:31 UTC #2 lore raw

Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c

Quoted 34 lines · Peng Tao

In mext_replace_branches(), the oext and dext virable might be NULL if the orig extent and donor extent are empty. Later calling *oext and *dext will trigger a kernel null pointer bug like this:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0636563>] mext_replace_branches+0x10c/0x2f3 [ext4]
PGD 37dba067 PUD cd8ac067 PMD 0
Oops: 0000 [#1] SMP
The patch checks the two virables and returns EOPNOTSUPP if either of them is NULL.
Signed-off-by Peng Tao <[email protected]>
---
 fs/ext4/move_extent.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
fs/ext4/move_extent.c +6 -0
index 5821e0b..4923f70 100644
@@ -641,10 +641,22 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
 		goto out;
 	depth = ext_depth(orig_inode);
 	oext = orig_path[depth].p_ext;
+	if (oext == NULL) {
+		ext4_debug("ext4 move extent: "
+					"orig extents should not be empty\n");
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 	tmp_oext = *oext;
 
 	depth = ext_depth(donor_inode);

fs/ext4/move_extent.c mext_replace_branches(handle_t *handle, struct inode *orig_inode, · line 662

Quoted 10 lines

fs/ext4/move_extent.c +6 -0

 	dext = donor_path[depth].p_ext;
+	if (dext == NULL) {
+		ext4_debug("ext4 move extent: "
+					"donor extents should not be empty\n");
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 	tmp_dext = *dext;
 
 	mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,

Yes, this problem occurs if extent which ext4_ext_path indicates is NULL, but this check should be done in ext4_move_extents()
which is called before mext_replace_branches().
And in this case, ENOENT is better for error value, I think. How about this patch?

Signed-off-by Akira Fujita <[email protected]>

---
 move_extent.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

ext4/move_extent.c renamed from ../LATEST/fs/ext4/move_extent.c +15 -4

@@ -1145,19 +1145,23 @@ ext4_move_extents(struct file *o_filp, s
 	if (file_end < block_end)
 		len -= block_end - file_end;
 
+	depth = ext_depth(orig_inode);
 	get_ext_path(orig_path, orig_inode, block_start, ret);
 	if (orig_path == NULL)
 		goto out2;
+	else if (orig_path[depth].p_ext == NULL) {
+		ret = -ENOENT;
+		goto out;
+	}
 
 	/* Get path structure to check the hole */
 	get_ext_path(holecheck_path, orig_inode, block_start, ret);
 	if (holecheck_path == NULL)
 		goto out;
 
-	depth = ext_depth(orig_inode);
 	ext_cur = holecheck_path[depth].p_ext;
 	if (ext_cur == NULL) {
-		ret = -EINVAL;
+		ret = -ENOENT;
 		goto out;
 	}

@@ -1280,11 +1284,14 @@ ext4_move_extents(struct file *o_filp, s
 		/* Decrease buffer counter */
 		if (holecheck_path)
 			ext4_ext_drop_refs(holecheck_path);
-		get_ext_path(holecheck_path, orig_inode,
-				      seq_start, ret);
+		get_ext_path(holecheck_path, orig_inode, seq_start, ret);
 		if (holecheck_path == NULL)
 			break;
 		depth = holecheck_path->p_depth;
+		if (holecheck_path[depth].p_ext == NULL) {
+			ret = -ENOENT;
+			break;
+		}
 
 		/* Decrease buffer counter */
 		if (orig_path)

@@ -1292,6 +1299,10 @@ ext4_move_extents(struct file *o_filp, s
 		get_ext_path(orig_path, orig_inode, seq_start, ret);
 		if (orig_path == NULL)
 			break;
+		else if (orig_path[depth].p_ext == NULL) {
+			ret = -ENOENT;
+			break;
+		}
 
 		ext_cur = holecheck_path[depth].p_ext;
 		add_blocks = ext4_ext_get_actual_len(ext_cur);

Peng Tao

author

2009-08-03 13:27 UTC #3 lore raw

Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c

Hi, Akira,

2009/8/3 Akira Fujita <[email protected]>:

Quoted 45 lines

Quoted 43 lines · Peng Tao

In mext_replace_branches(), the oext and dext virable might be NULL if the orig extent and donor extent are empty. Later calling *oext and *dext will trigger a kernel null pointer bug like this:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0636563>] mext_replace_branches+0x10c/0x2f3 [ext4]
PGD 37dba067 PUD cd8ac067 PMD 0
Oops: 0000 [#1] SMP

The patch checks the two virables and returns EOPNOTSUPP if either of them is NULL.

Signed-off-by Peng Tao <[email protected]>

---
fs/ext4/move_extent.c | 12 ++++++++++++

 1 files changed, 12 insertions(+), 0 deletions(-)

fs/ext4/move_extent.c +12 -0

index 5821e0b..4923f70 100644
@@ -641,10 +641,22 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
               goto out;
       depth = ext_depth(orig_inode);
       oext = orig_path[depth].p_ext;
+     if (oext == NULL) {
+             ext4_debug("ext4 move extent: "
+                                     "orig extents should not be empty\n");
+             err = -EOPNOTSUPP;
+             goto out;
+     }
       tmp_oext = *oext;
 
       depth = ext_depth(donor_inode);
       dext = donor_path[depth].p_ext;
+     if (dext == NULL) {
+             ext4_debug("ext4 move extent: "
+                                     "donor extents should not be empty\n");
+             err = -EOPNOTSUPP;
+             goto out;
+     }
       tmp_dext = *dext;

Quoted 7 lines

fs/ext4/move_extent.c mext_replace_branches(handle_t *handle, struct inode *orig_inode, · line 662
Quoted 1 line
fs/ext4/move_extent.c +0 -0
       mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
Yes, this problem occurs if extent which ext4_ext_path indicates is NULL, but this check should be done in ext4_move_extents()
which is called before mext_replace_branches().
And in this case, ENOENT is better for error value, I think. How about this patch?

Will there be situations where empty extents exist in the middle of an extent tree? Because your patch only checks NULL extents once at the begining. If some NULL extents are later found in the loop, the bug still can be triggered and we should check NULL extents in mext_replace_branches().

Quoted 63 lines

Signed-off-by Akira Fujita <[email protected]>

---
move_extent.c | 19 +++++++++++++++----

 1 file changed, 15 insertions(+), 4 deletions(-)

ext4/move_extent.c 2009-08-03 15:03:33.000000000 +0900 renamed from ../LATEST/fs/ext4/move_extent.c 2009-08-03 14:53:43.000000000 +0900 +15 -4

@@ -1145,19 +1145,23 @@ ext4_move_extents(struct file *o_filp, s
        if (file_end < block_end)
                len -= block_end - file_end;
 
+       depth = ext_depth(orig_inode);
        get_ext_path(orig_path, orig_inode, block_start, ret);
        if (orig_path == NULL)
                goto out2;
+       else if (orig_path[depth].p_ext == NULL) {
+               ret = -ENOENT;
+               goto out;
+       }
 
        /* Get path structure to check the hole */
        get_ext_path(holecheck_path, orig_inode, block_start, ret);
        if (holecheck_path == NULL)
                goto out;
 
-       depth = ext_depth(orig_inode);
        ext_cur = holecheck_path[depth].p_ext;
        if (ext_cur == NULL) {
-               ret = -EINVAL;
+               ret = -ENOENT;
                goto out;
        }

@@ -1280,11 +1284,14 @@ ext4_move_extents(struct file *o_filp, s
                /* Decrease buffer counter */
                if (holecheck_path)
                        ext4_ext_drop_refs(holecheck_path);
-               get_ext_path(holecheck_path, orig_inode,
-                                     seq_start, ret);
+               get_ext_path(holecheck_path, orig_inode, seq_start, ret);
                if (holecheck_path == NULL)
                        break;
                depth = holecheck_path->p_depth;
+               if (holecheck_path[depth].p_ext == NULL) {
+                       ret = -ENOENT;
+                       break;
+               }
 
                /* Decrease buffer counter */
                if (orig_path)

@@ -1292,6 +1299,10 @@ ext4_move_extents(struct file *o_filp, s
                get_ext_path(orig_path, orig_inode, seq_start, ret);
                if (orig_path == NULL)
                        break;
+               else if (orig_path[depth].p_ext == NULL) {
+                       ret = -ENOENT;
+                       break;
+               }
 
                ext_cur = holecheck_path[depth].p_ext;
                add_blocks = ext4_ext_get_actual_len(ext_cur);

Signature · 8 lines

Cheers,
Peng Tao
State Key Laboratory of Networking and Switching Technology
Beijing Univ. of Posts and Telecoms.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Akira Fujita

2009-08-04 08:24 UTC #4 lore raw

Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c

Hi Peng,

Quoted 5 lines · Peng Tao

Will there be situations where empty extents exist in the middle of an extent tree? Because your patch only checks NULL extents once at the begining. If some NULL extents are later found in the loop, the bug still can be triggered and we should check NULL extents in mext_replace_branches().

In the case of eh->entries is 0 in ext4_ext_find_extent(),
the extent which ext4_ext_path indicates is null.
This is happened if file blocks are not allocated yet.
But an extent tree does not have empty extents in the middle of it, I think.

I have no idea we should handle empty extents case in EXT4_IOC_MOVE_EXTENTS, but if yes, we should do null check at other places
not only mext_replace_branches().

So I add this null check to get_ext_path macro
which is used to get ext4_ext_path.

Regards,
Akira Fujita

Signed-off-by Akira Fujita <[email protected]>

---
 move_extent.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

fs/ext4/move_extent.c +15 ~~-17~~

@@ -25,6 +25,8 @@
 		if (IS_ERR(path)) {					\
 			ret = PTR_ERR(path);				\
 			path = NULL;					\
+		} else if (path[ext_depth(inode)].p_ext == NULL) {	\
+			ret = -ENOENT;					\
 		}							\
 	} while (0)

@@ -284,7 +286,7 @@ mext_insert_across_blocks(handle_t *hand
 
 	if (new_flag) {
 		get_ext_path(orig_path, orig_inode, eblock, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -295,7 +297,7 @@ mext_insert_across_blocks(handle_t *hand
 	if (end_flag) {
 		get_ext_path(orig_path, orig_inode,
 				      le32_to_cpu(end_ext->ee_block) - 1, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -632,12 +634,12 @@ mext_replace_branches(handle_t *handle,
 
 	/* Get the original extent for the block "orig_off" */
 	get_ext_path(orig_path, orig_inode, orig_off, err);
-	if (orig_path == NULL)
+	if (err)
 		goto out;
 
 	/* Get the donor extent for the head */
 	get_ext_path(donor_path, donor_inode, donor_off, err);
-	if (donor_path == NULL)
+	if (err)
 		goto out;
 	depth = ext_depth(orig_inode);
 	oext = orig_path[depth].p_ext;

@@ -679,7 +681,7 @@ mext_replace_branches(handle_t *handle,
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, orig_off, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(orig_inode);
 		oext = orig_path[depth].p_ext;

@@ -694,7 +696,7 @@ mext_replace_branches(handle_t *handle,
 			ext4_ext_drop_refs(donor_path);
 		get_ext_path(donor_path, donor_inode,
 				      donor_off, err);
-		if (donor_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(donor_inode);
 		dext = donor_path[depth].p_ext;

@@ -1138,7 +1140,7 @@ ext4_move_extents(struct file *o_filp, s
 					donor_start, &len, *moved_len);
 	mext_double_up_read(orig_inode, donor_inode);
 	if (ret)
-		goto out2;
+		goto out;
 
 	file_end = (i_size_read(orig_inode) - 1) >> orig_inode->i_blkbits;
 	block_end = block_start + len - 1;

@@ -1146,20 +1148,16 @@ ext4_move_extents(struct file *o_filp, s
 		len -= block_end - file_end;
 
 	get_ext_path(orig_path, orig_inode, block_start, ret);
-	if (orig_path == NULL)
-		goto out2;
+	if (ret)
+		goto out;
 
 	/* Get path structure to check the hole */
 	get_ext_path(holecheck_path, orig_inode, block_start, ret);
-	if (holecheck_path == NULL)
+	if (ret)
 		goto out;
 
 	depth = ext_depth(orig_inode);
 	ext_cur = holecheck_path[depth].p_ext;
-	if (ext_cur == NULL) {
-		ret = -EINVAL;
-		goto out;
-	}
 
 	/*
 	 * Get proper extent whose ee_block is beyond block_start

@@ -1282,7 +1280,7 @@ ext4_move_extents(struct file *o_filp, s
 			ext4_ext_drop_refs(holecheck_path);
 		get_ext_path(holecheck_path, orig_inode,
 				      seq_start, ret);
-		if (holecheck_path == NULL)
+		if (ret)
 			break;
 		depth = holecheck_path->p_depth;

@@ -1290,7 +1288,7 @@ ext4_move_extents(struct file *o_filp, s
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, seq_start, ret);
-		if (orig_path == NULL)
+		if (ret)
 			break;
 
 		ext_cur = holecheck_path[depth].p_ext;

@@ -1307,7 +1305,7 @@ out:
 		ext4_ext_drop_refs(holecheck_path);
 		kfree(holecheck_path);
 	}
-out2:
+
 	mext_inode_double_unlock(orig_inode, donor_inode);
 
 	if (ret)

Peng Tao

author

2009-08-08 17:18 UTC #5 lore raw

Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c

Hi, Akira

Sorry to response late.

Quoted 9 lines · Akira Fujita

Hi Peng,
Quoted 5 lines · Peng Tao
Will there be situations where empty extents exist in the middle of an extent tree? Because your patch only checks NULL extents once at the begining. If some NULL extents are later found in the loop, the bug still can be triggered and we should check NULL extents in mext_replace_branches().

Quoted 4 lines

In the case of eh->entries is 0 in ext4_ext_find_extent(),
the extent which ext4_ext_path indicates is null.
This is happened if file blocks are not allocated yet.
But an extent tree does not have empty extents in the middle of it, I think.

I'm afraid it does when the targeted extent points to an empty leaf. from fs/ext4/extents.c ext4_ext_find_extent()
681

 682         /* find extent */
 683         ext4_ext_binsearch(inode, path + ppos, block);
 684         /* if not an empty leaf */
 685         if (path[ppos].p_ext)

686 path[ppos].p_block = ext_pblock(path[ppos].p_ext);
687
Please correct me if I am wrong.

BTW, I don't think ENOENT (translated to "No such file or directory") is a good return value , because the file is obviously out there. It just has empty extents.

Quoted 139 lines

I have no idea we should handle empty extents case in EXT4_IOC_MOVE_EXTENTS, but if yes, we should do null check at other places
not only mext_replace_branches().

So I add this null check to get_ext_path macro
which is used to get ext4_ext_path.

Regards,
Akira Fujita

Signed-off-by Akira Fujita <[email protected]>

---
 move_extent.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

fs/ext4/move_extent.c +15 ~~-17~~

@@ -25,6 +25,8 @@
 		if (IS_ERR(path)) {					\
 			ret = PTR_ERR(path);				\
 			path = NULL;					\
+		} else if (path[ext_depth(inode)].p_ext == NULL) {	\
+			ret = -ENOENT;					\
 		}							\
 	} while (0)

@@ -284,7 +286,7 @@ mext_insert_across_blocks(handle_t *hand
 
 	if (new_flag) {
 		get_ext_path(orig_path, orig_inode, eblock, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -295,7 +297,7 @@ mext_insert_across_blocks(handle_t *hand
 	if (end_flag) {
 		get_ext_path(orig_path, orig_inode,
 				      le32_to_cpu(end_ext->ee_block) - 1, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -632,12 +634,12 @@ mext_replace_branches(handle_t *handle,
 
 	/* Get the original extent for the block "orig_off" */
 	get_ext_path(orig_path, orig_inode, orig_off, err);
-	if (orig_path == NULL)
+	if (err)
 		goto out;
 
 	/* Get the donor extent for the head */
 	get_ext_path(donor_path, donor_inode, donor_off, err);
-	if (donor_path == NULL)
+	if (err)
 		goto out;
 	depth = ext_depth(orig_inode);
 	oext = orig_path[depth].p_ext;

@@ -679,7 +681,7 @@ mext_replace_branches(handle_t *handle,
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, orig_off, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(orig_inode);
 		oext = orig_path[depth].p_ext;

@@ -694,7 +696,7 @@ mext_replace_branches(handle_t *handle,
 			ext4_ext_drop_refs(donor_path);
 		get_ext_path(donor_path, donor_inode,
 				      donor_off, err);
-		if (donor_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(donor_inode);
 		dext = donor_path[depth].p_ext;

@@ -1138,7 +1140,7 @@ ext4_move_extents(struct file *o_filp, s
 					donor_start, &len, *moved_len);
 	mext_double_up_read(orig_inode, donor_inode);
 	if (ret)
-		goto out2;
+		goto out;
 
 	file_end = (i_size_read(orig_inode) - 1) >> orig_inode->i_blkbits;
 	block_end = block_start + len - 1;

@@ -1146,20 +1148,16 @@ ext4_move_extents(struct file *o_filp, s
 		len -= block_end - file_end;
 
 	get_ext_path(orig_path, orig_inode, block_start, ret);
-	if (orig_path == NULL)
-		goto out2;
+	if (ret)
+		goto out;
 
 	/* Get path structure to check the hole */
 	get_ext_path(holecheck_path, orig_inode, block_start, ret);
-	if (holecheck_path == NULL)
+	if (ret)
 		goto out;
 
 	depth = ext_depth(orig_inode);
 	ext_cur = holecheck_path[depth].p_ext;
-	if (ext_cur == NULL) {
-		ret = -EINVAL;
-		goto out;
-	}
 
 	/*
 	 * Get proper extent whose ee_block is beyond block_start

@@ -1282,7 +1280,7 @@ ext4_move_extents(struct file *o_filp, s
 			ext4_ext_drop_refs(holecheck_path);
 		get_ext_path(holecheck_path, orig_inode,
 				      seq_start, ret);
-		if (holecheck_path == NULL)
+		if (ret)
 			break;
 		depth = holecheck_path->p_depth;

@@ -1290,7 +1288,7 @@ ext4_move_extents(struct file *o_filp, s
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, seq_start, ret);
-		if (orig_path == NULL)
+		if (ret)
 			break;
 
 		ext_cur = holecheck_path[depth].p_ext;

@@ -1307,7 +1305,7 @@ out:
 		ext4_ext_drop_refs(holecheck_path);
 		kfree(holecheck_path);
 	}
-out2:
+
 	mext_inode_double_unlock(orig_inode, donor_inode);
 
 	if (ret)

Signature · 4 lines

Best Regards,
Peng Tao
State Key Laboratory of Networking and Switching Technology
Beijing Univ. of Posts and Telecoms.

Peng Tao

author

2009-08-10 17:26 UTC #6 lore raw

Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c

In reply to Akira Fujita

Hi, Akira

When testing your patch, I noticed that if mext_replace_branches() ever fails, it will leave the orig file in a broken state. Later accessing the file will lead ext4 complaining like:

EXT4-fs error (device sda6): check_block_validity: inode #20 logical
block 0 mapped to 4295143424 (size 1)

My test case is running EXT4_IOC_MOVE_EXTENTS against sparse donor files created by:
$dd if=/dev/zero of=zero.img bs=10M count=0 seek=50 $dd if=../609xp.img of=first.img bs=10M count=1 $dd if=/dev/zero of=first.img bs=10M count=0 seek=50 $dd if=../609xp.img of=last.img bs=10M count=1 seek=49 $dd if=../609xp.img of=middle.img bs=10M count=1 seek=24 $dd if=/dev/zero of=middle.img bs=10M count=0 seek=50

../609xp.img is a 10G sized vm image(sparse file too).

Any fixup suggestions?

Quoted 152 lines · Akira Fujita

Hi Peng,

Quoted 5 lines · Peng Tao

Will there be situations where empty extents exist in the middle of an extent tree? Because your patch only checks NULL extents once at the begining. If some NULL extents are later found in the loop, the bug still can be triggered and we should check NULL extents in mext_replace_branches().

I have no idea we should handle empty extents case in EXT4_IOC_MOVE_EXTENTS, but if yes, we should do null check at other places
not only mext_replace_branches().

So I add this null check to get_ext_path macro
which is used to get ext4_ext_path.

Regards,
Akira Fujita

Signed-off-by Akira Fujita <[email protected]>

---
 move_extent.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

fs/ext4/move_extent.c +15 ~~-17~~

@@ -25,6 +25,8 @@
 		if (IS_ERR(path)) {					\
 			ret = PTR_ERR(path);				\
 			path = NULL;					\
+		} else if (path[ext_depth(inode)].p_ext == NULL) {	\
+			ret = -ENOENT;					\
 		}							\
 	} while (0)

@@ -284,7 +286,7 @@ mext_insert_across_blocks(handle_t *hand
 
 	if (new_flag) {
 		get_ext_path(orig_path, orig_inode, eblock, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -295,7 +297,7 @@ mext_insert_across_blocks(handle_t *hand
 	if (end_flag) {
 		get_ext_path(orig_path, orig_inode,
 				      le32_to_cpu(end_ext->ee_block) - 1, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 
 		if (ext4_ext_insert_extent(handle, orig_inode,

@@ -632,12 +634,12 @@ mext_replace_branches(handle_t *handle,
 
 	/* Get the original extent for the block "orig_off" */
 	get_ext_path(orig_path, orig_inode, orig_off, err);
-	if (orig_path == NULL)
+	if (err)
 		goto out;
 
 	/* Get the donor extent for the head */
 	get_ext_path(donor_path, donor_inode, donor_off, err);
-	if (donor_path == NULL)
+	if (err)
 		goto out;
 	depth = ext_depth(orig_inode);
 	oext = orig_path[depth].p_ext;

@@ -679,7 +681,7 @@ mext_replace_branches(handle_t *handle,
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, orig_off, err);
-		if (orig_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(orig_inode);
 		oext = orig_path[depth].p_ext;

@@ -694,7 +696,7 @@ mext_replace_branches(handle_t *handle,
 			ext4_ext_drop_refs(donor_path);
 		get_ext_path(donor_path, donor_inode,
 				      donor_off, err);
-		if (donor_path == NULL)
+		if (err)
 			goto out;
 		depth = ext_depth(donor_inode);
 		dext = donor_path[depth].p_ext;

@@ -1138,7 +1140,7 @@ ext4_move_extents(struct file *o_filp, s
 					donor_start, &len, *moved_len);
 	mext_double_up_read(orig_inode, donor_inode);
 	if (ret)
-		goto out2;
+		goto out;
 
 	file_end = (i_size_read(orig_inode) - 1) >> orig_inode->i_blkbits;
 	block_end = block_start + len - 1;

@@ -1146,20 +1148,16 @@ ext4_move_extents(struct file *o_filp, s
 		len -= block_end - file_end;
 
 	get_ext_path(orig_path, orig_inode, block_start, ret);
-	if (orig_path == NULL)
-		goto out2;
+	if (ret)
+		goto out;
 
 	/* Get path structure to check the hole */
 	get_ext_path(holecheck_path, orig_inode, block_start, ret);
-	if (holecheck_path == NULL)
+	if (ret)
 		goto out;
 
 	depth = ext_depth(orig_inode);
 	ext_cur = holecheck_path[depth].p_ext;
-	if (ext_cur == NULL) {
-		ret = -EINVAL;
-		goto out;
-	}
 
 	/*
 	 * Get proper extent whose ee_block is beyond block_start

@@ -1282,7 +1280,7 @@ ext4_move_extents(struct file *o_filp, s
 			ext4_ext_drop_refs(holecheck_path);
 		get_ext_path(holecheck_path, orig_inode,
 				      seq_start, ret);
-		if (holecheck_path == NULL)
+		if (ret)
 			break;
 		depth = holecheck_path->p_depth;

@@ -1290,7 +1288,7 @@ ext4_move_extents(struct file *o_filp, s
 		if (orig_path)
 			ext4_ext_drop_refs(orig_path);
 		get_ext_path(orig_path, orig_inode, seq_start, ret);
-		if (orig_path == NULL)
+		if (ret)
 			break;
 
 		ext_cur = holecheck_path[depth].p_ext;

@@ -1307,7 +1305,7 @@ out:
 		ext4_ext_drop_refs(holecheck_path);
 		kfree(holecheck_path);
 	}
-out2:
+
 	mext_inode_double_unlock(orig_inode, donor_inode);
 
 	if (ret)

Signature · 4 lines

Best Regards,
Peng Tao
State Key Laboratory of Networking and Switching Technology
Beijing Univ. of Posts and Telecoms.

Akira Fujita

2009-08-03 08:31 UTC #7 lore raw

In reply to Peng Tao

Hi Peng,

Quoted 6 lines · Peng Tao

move_extent_par_page calls a_ops->write_begin() to increase journal handler's reference count. However, if either mext_replace_branches() or ext4_get_block fails, the increased reference count isn't decreased. This will cause later umounting of the fs forever hangs. The patch addresses the issue by calling ext4_journal_stop() if page is not NULL (which means a_ops->write_end() isn't invoked).

In case mext_replaced_branches() or ext4_get_block failed, ext4_journal_stop() is called at out2 label(*) and then journal reference counter is decreased. Therefore I think this fix is not necessary.

static int
move_extent_par_page(struct file *o_filp, struct inode *donor_inode,
pgoff_t orig_page_offset, int data_offset_in_page, int block_len_in_page, int uninit)
<snip>

out:

        if (unlikely(page)) {
                if (PageLocked(page))
                        unlock_page(page);
                page_cache_release(page);
        }

out2:
* ext4_journal_stop(handle);

        return ret < 0 ? ret : 0;
}

Regards,
Akira Fujita

Peng Tao

author

2009-08-03 13:17 UTC #8 lore raw

Hi, Akira,

2009/8/3 Akira Fujita <[email protected]>:

Quoted 10 lines

Hi Peng,
Quoted 6 lines · Peng Tao
move_extent_par_page calls a_ops->write_begin() to increase journal handler's reference count. However, if either mext_replace_branches() or ext4_get_block fails, the increased reference count isn't decreased. This will cause later umounting of the fs forever hangs. The patch addresses the issue by calling ext4_journal_stop() if page is not NULL (which means a_ops->write_end() isn't invoked).

Quoted 4 lines

In case mext_replaced_branches() or ext4_get_block failed, ext4_journal_stop() is called at out2 label(*) and then journal reference counter is decreased. Therefore I think this fix is not necessary.

well, the orginal code calls both ext4_journal_start and a_ops->write_begin(). So the journal reference was increased twice but only gets decreased once in case of failure. This can be simply verified by returning -1 at the begining of mext_replaced_branches(). With such modification, the fs cannot be umounted because of the wrong reference count.

Quoted 22 lines

static int
move_extent_par_page(struct file *o_filp, struct inode *donor_inode, pgoff_t orig_page_offset, int data_offset_in_page, int block_len_in_page, int uninit) <snip>
out:
if (unlikely(page)) {
if (PageLocked(page))
unlock_page(page);
page_cache_release(page);
}
out2:
* ext4_journal_stop(handle);
return ret < 0 ? ret : 0;
}
Regards,
Akira Fujita

Signature · 8 lines

Cheers,
Peng Tao
State Key Laboratory of Networking and Switching Technology
Beijing Univ. of Posts and Telecoms.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Akira Fujita

2009-08-04 08:24 UTC #9 lore raw

Quoted 16 lines · Peng Tao

Hi, Akira,
2009/8/3 Akira Fujita <[email protected]>:
Quoted 9 lines
Hi Peng,
Quoted 6 lines · Peng Tao
move_extent_par_page calls a_ops->write_begin() to increase journal handler's reference count. However, if either mext_replace_branches() or ext4_get_block fails, the increased reference count isn't decreased. This will cause later umounting of the fs forever hangs. The patch addresses the issue by calling ext4_journal_stop() if page is not NULL (which means a_ops->write_end() isn't invoked).
Quoted 4 lines
In case mext_replaced_branches() or ext4_get_block failed, ext4_journal_stop() is called at out2 label(*) and then journal reference counter is decreased. Therefore I think this fix is not necessary.

Quoted 6 lines

well, the orginal code calls both ext4_journal_start and a_ops->write_begin(). So the journal reference was increased twice but only gets decreased once in case of failure. This can be simply verified by returning -1 at the begining of mext_replaced_branches(). With such modification, the fs cannot be umounted because of the wrong reference count.

Ah, I missed.
Sorry for the noise.
The patch looks good to me.

Regards,
Akira Fujita