* [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page @ 2009-08-02 11:43 Peng Tao 2009-08-02 11:43 ` [PATCH 2/2] ext4: fix null pointer bug in move_extent.c Peng Tao 2009-08-03 8:30 ` [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Akira Fujita 0 siblings, 2 replies; 10+ messages in thread From: Peng Tao @ 2009-08-02 11:43 UTC (permalink / raw) To: linux-ext4; +Cc: Theodore Ts'o, Akira Fujita 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-02 11:43 [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Peng Tao @ 2009-08-02 11:43 ` Peng Tao 2009-08-03 8:30 ` Akira Fujita 2009-08-03 8:30 ` [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Akira Fujita 1 sibling, 1 reply; 10+ messages in thread From: Peng Tao @ 2009-08-02 11:43 UTC (permalink / raw) To: linux-ext4; +Cc: Theodore Ts'o, Akira Fujita 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-02 11:43 ` [PATCH 2/2] ext4: fix null pointer bug in move_extent.c Peng Tao @ 2009-08-03 8:30 ` Akira Fujita 2009-08-03 13:20 ` Peng Tao 0 siblings, 1 reply; 10+ messages in thread From: Akira Fujita @ 2009-08-03 8:30 UTC (permalink / raw) To: Peng Tao; +Cc: linux-ext4, Theodore Ts'o 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-03 8:30 ` Akira Fujita @ 2009-08-03 13:20 ` Peng Tao 2009-08-04 8:24 ` Akira Fujita 0 siblings, 1 reply; 10+ messages in thread From: Peng Tao @ 2009-08-03 13:20 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-03 13:20 ` Peng Tao @ 2009-08-04 8:24 ` Akira Fujita 2009-08-08 17:18 ` Peng Tao 2009-08-10 17:26 ` Peng Tao 0 siblings, 2 replies; 10+ messages in thread From: Akira Fujita @ 2009-08-04 8:24 UTC (permalink / raw) To: Peng Tao; +Cc: linux-ext4, Theodore Ts'o 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) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-04 8:24 ` Akira Fujita @ 2009-08-08 17:18 ` Peng Tao 2009-08-10 17:26 ` Peng Tao 1 sibling, 0 replies; 10+ messages in thread From: Peng Tao @ 2009-08-08 17:18 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4, Theodore Ts'o 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ext4: fix null pointer bug in move_extent.c 2009-08-04 8:24 ` Akira Fujita 2009-08-08 17:18 ` Peng Tao @ 2009-08-10 17:26 ` Peng Tao 1 sibling, 0 replies; 10+ messages in thread From: Peng Tao @ 2009-08-10 17:26 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4, Theodore Ts'o 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page 2009-08-02 11:43 [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Peng Tao 2009-08-02 11:43 ` [PATCH 2/2] ext4: fix null pointer bug in move_extent.c Peng Tao @ 2009-08-03 8:30 ` Akira Fujita 2009-08-03 13:17 ` Peng Tao 1 sibling, 1 reply; 10+ messages in thread From: Akira Fujita @ 2009-08-03 8:30 UTC (permalink / raw) To: Peng Tao; +Cc: linux-ext4, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page 2009-08-03 8:30 ` [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Akira Fujita @ 2009-08-03 13:17 ` Peng Tao 2009-08-04 8:23 ` Akira Fujita 0 siblings, 1 reply; 10+ messages in thread From: Peng Tao @ 2009-08-03 13:17 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page 2009-08-03 13:17 ` Peng Tao @ 2009-08-04 8:23 ` Akira Fujita 0 siblings, 0 replies; 10+ messages in thread From: Akira Fujita @ 2009-08-04 8:23 UTC (permalink / raw) To: Peng Tao; +Cc: linux-ext4, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-08-10 17:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-02 11:43 [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Peng Tao 2009-08-02 11:43 ` [PATCH 2/2] ext4: fix null pointer bug in move_extent.c Peng Tao 2009-08-03 8:30 ` Akira Fujita 2009-08-03 13:20 ` Peng Tao 2009-08-04 8:24 ` Akira Fujita 2009-08-08 17:18 ` Peng Tao 2009-08-10 17:26 ` Peng Tao 2009-08-03 8:30 ` [PATCH 1/2] ext4: fix journal ref count in move_extent_par_page Akira Fujita 2009-08-03 13:17 ` Peng Tao 2009-08-04 8:23 ` Akira Fujita
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).