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

login
register
mail settings
Submitter Akira Fujita
Date Aug. 4, 2009, 8:24 a.m.
Message ID <4A77F049.4050301@rs.jp.nec.com>
Download mbox | patch
Permalink /patch/30707/
State New
Headers show

Comments

Akira Fujita - Aug. 4, 2009, 8:24 a.m.
Hi Peng,

Peng Tao wrote:
> 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 <a-fujita@rs.jp.nec.com>
---
 move_extent.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peng Tao - Aug. 8, 2009, 5:18 p.m.
Hi, Akira

Sorry to response late.
Akira Fujita wrote:
> Hi Peng,
> 
> Peng Tao wrote:
>> 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'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.
> 
> 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 <a-fujita@rs.jp.nec.com>
> ---
>  move_extent.c |   32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> --- linux-2.6.31-rc4-a/fs/ext4/move_extent.c	2009-07-23 11:32:59.000000000 +0900
> +++ linux-2.6.31-rc4-b/fs/ext4/move_extent.c	2009-08-04 15:51:04.000000000 +0900
> @@ -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 - Aug. 10, 2009, 5:26 p.m.
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?

Akira Fujita wrote:
> Hi Peng,
> 
> Peng Tao wrote:
>> 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 <a-fujita@rs.jp.nec.com>
> ---
>  move_extent.c |   32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> --- linux-2.6.31-rc4-a/fs/ext4/move_extent.c	2009-07-23 11:32:59.000000000 +0900
> +++ linux-2.6.31-rc4-b/fs/ext4/move_extent.c	2009-08-04 15:51:04.000000000 +0900
> @@ -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)
> 
>

Patch

--- linux-2.6.31-rc4-a/fs/ext4/move_extent.c	2009-07-23 11:32:59.000000000 +0900
+++ linux-2.6.31-rc4-b/fs/ext4/move_extent.c	2009-08-04 15:51:04.000000000 +0900
@@ -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)