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

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

[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).

Signed-off-by: Peng Tao <bergwolf@gmail.com>
---
 fs/ext4/move_extent.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index bbf2dd9..5821e0b 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -871,6 +871,7 @@ out:
 		if (PageLocked(page))
 			unlock_page(page);
 		page_cache_release(page);
+		ext4_journal_stop(handle);
 	}
 out2:
 	ext4_journal_stop(handle);
-- 
1.6.2.GIT

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

[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 <bergwolf@gmail.com>
---
 fs/ext4/move_extent.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 5821e0b..4923f70 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -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,
-- 
1.6.2.GIT

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

[Akira Fujita]

Hi Peng,

Peng Tao wrote:
> 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

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

[Akira Fujita]

Peng Tao wrote:
> 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 <bergwolf@gmail.com>
> ---
>  fs/ext4/move_extent.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 5821e0b..4923f70 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -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,

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 <a-fujita@rs.jp.nec.com>

---
 move_extent.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
--- ../../LATEST/fs/ext4/move_extent.c	2009-08-03 14:53:43.000000000 +0900
+++ fs/ext4/move_extent.c	2009-08-03 15:03:33.000000000 +0900
@@ -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);

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

[Peng Tao]

Hi, Akira,

2009/8/3 Akira Fujita <a-fujita@rs.jp.nec.com>:
> Hi Peng,
>
> Peng Tao wrote:
>> 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.
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.
>
> 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
>



-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Peng Tao]

Hi, Akira,

2009/8/3 Akira Fujita <a-fujita@rs.jp.nec.com>:
>
> Peng Tao wrote:
>> 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 <bergwolf@gmail.com>
>> ---
>>  fs/ext4/move_extent.c |   12 ++++++++++++
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
>> index 5821e0b..4923f70 100644
>> --- a/fs/ext4/move_extent.c
>> +++ b/fs/ext4/move_extent.c
>> @@ -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,
>
> 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().
>
> Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com>
>
> ---
>  move_extent.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> --- ../../LATEST/fs/ext4/move_extent.c  2009-08-03 14:53:43.000000000 +0900
> +++ fs/ext4/move_extent.c       2009-08-03 15:03:33.000000000 +0900
> @@ -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);
>



-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Akira Fujita]

Peng Tao wrote:
> Hi, Akira,
> 
> 2009/8/3 Akira Fujita <a-fujita@rs.jp.nec.com>:
>> Hi Peng,
>>
>> Peng Tao wrote:
>>> 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.
> 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

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

[Akira Fujita]

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)

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

[Peng Tao]

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)
> 
> 


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

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

[Peng Tao]

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)
> 
> 


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