* [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() @ 2009-09-02 3:17 Akira Fujita 2009-09-02 16:02 ` Peng Tao 2009-09-06 2:10 ` Theodore Tso 0 siblings, 2 replies; 5+ messages in thread From: Akira Fujita @ 2009-09-02 3:17 UTC (permalink / raw) To: Theodore Tso; +Cc: linux-ext4 ext4: Fix wrong comparisons in mext_check_arguments() From: Akira Fujita <a-fujita@rs.jp.nec.com> mext_check_arguments() in move_extents.c has wrong comparisons. orig_start which is passed from user-space is block unit, but i_size of inode is byte unit, therefore the checks do not work fine. This mis-check leads to the overflow of 'len' and then hits BUG_ON() in ext4_move_extens(). The patch fixes this issue. Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com> --- fs/ext4/move_extent.c | 39 ++++++++++++++++++++++++--------------- 1 files changed, 24 insertions(+), 15 deletions(-) diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 5821e0b..60ed567 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -972,43 +972,52 @@ mext_check_arguments(struct inode *orig_inode, } if (orig_inode->i_size > donor_inode->i_size) { - if (orig_start >= donor_inode->i_size) { + if (orig_start << orig_inode->i_blkbits >= + donor_inode->i_size) { ext4_debug("ext4 move extent: orig start offset " "[%llu] should be less than donor file size " "[%lld] [ino:orig %lu, donor_inode %lu]\n", - orig_start, donor_inode->i_size, - orig_inode->i_ino, donor_inode->i_ino); + orig_start << orig_inode->i_blkbits, + donor_inode->i_size, orig_inode->i_ino, + donor_inode->i_ino); return -EINVAL; } - - if (orig_start + *len > donor_inode->i_size) { + if ((orig_start + *len) << orig_inode->i_blkbits > + donor_inode->i_size) { ext4_debug("ext4 move extent: End offset [%llu] should " "be less than donor file size [%lld]." "So adjust length from %llu to %lld " "[ino:orig %lu, donor %lu]\n", - orig_start + *len, donor_inode->i_size, - *len, donor_inode->i_size - orig_start, + (orig_start + *len) << orig_inode->i_blkbits, + donor_inode->i_size, + *len, (donor_inode->i_size >> + orig_inode->i_blkbits) - orig_start, orig_inode->i_ino, donor_inode->i_ino); - *len = donor_inode->i_size - orig_start; + *len = (donor_inode->i_size >> orig_inode->i_blkbits) - + orig_start; } } else { - if (orig_start >= orig_inode->i_size) { + if (orig_start << orig_inode->i_blkbits >= + orig_inode->i_size) { ext4_debug("ext4 move extent: start offset [%llu] " "should be less than original file size " "[%lld] [inode:orig %lu, donor %lu]\n", - orig_start, orig_inode->i_size, - orig_inode->i_ino, donor_inode->i_ino); + orig_start << orig_inode->i_blkbits, + orig_inode->i_size, orig_inode->i_ino, + donor_inode->i_ino); return -EINVAL; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() 2009-09-02 3:17 [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() Akira Fujita @ 2009-09-02 16:02 ` Peng Tao 2009-09-06 2:10 ` Theodore Tso 1 sibling, 0 replies; 5+ messages in thread From: Peng Tao @ 2009-09-02 16:02 UTC (permalink / raw) To: Akira Fujita; +Cc: Theodore Tso, linux-ext4 Hi, Akira, Akira Fujita wrote: > ext4: Fix wrong comparisons in mext_check_arguments() > > From: Akira Fujita <a-fujita@rs.jp.nec.com> > > mext_check_arguments() in move_extents.c has wrong comparisons. > orig_start which is passed from user-space is block unit, > but i_size of inode is byte unit, therefore the checks do not work fine. > This mis-check leads to the overflow of 'len' and then hits BUG_ON() > in ext4_move_extens(). The patch fixes this issue. While the bug is true, I wander if it checks all conditions, because i_size isn't blocksize aligned. > > Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com> > --- > fs/ext4/move_extent.c | 39 ++++++++++++++++++++++++--------------- > 1 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c > index 5821e0b..60ed567 100644 > --- a/fs/ext4/move_extent.c > +++ b/fs/ext4/move_extent.c > @@ -972,43 +972,52 @@ mext_check_arguments(struct inode *orig_inode, > } > > if (orig_inode->i_size > donor_inode->i_size) { > - if (orig_start >= donor_inode->i_size) { > + if (orig_start << orig_inode->i_blkbits >= > + donor_inode->i_size) { > ext4_debug("ext4 move extent: orig start offset " > "[%llu] should be less than donor file size " > "[%lld] [ino:orig %lu, donor_inode %lu]\n", > - orig_start, donor_inode->i_size, > - orig_inode->i_ino, donor_inode->i_ino); > + orig_start << orig_inode->i_blkbits, > + donor_inode->i_size, orig_inode->i_ino, > + donor_inode->i_ino); > return -EINVAL; > } > - > - if (orig_start + *len > donor_inode->i_size) { > + if ((orig_start + *len) << orig_inode->i_blkbits > > + donor_inode->i_size) { > ext4_debug("ext4 move extent: End offset [%llu] should " > "be less than donor file size [%lld]." > "So adjust length from %llu to %lld " > "[ino:orig %lu, donor %lu]\n", > - orig_start + *len, donor_inode->i_size, > - *len, donor_inode->i_size - orig_start, > + (orig_start + *len) << orig_inode->i_blkbits, > + donor_inode->i_size, > + *len, (donor_inode->i_size >> > + orig_inode->i_blkbits) - orig_start, > orig_inode->i_ino, donor_inode->i_ino); > - *len = donor_inode->i_size - orig_start; > + *len = (donor_inode->i_size >> orig_inode->i_blkbits) - > + orig_start; > } > } else { > - if (orig_start >= orig_inode->i_size) { > + if (orig_start << orig_inode->i_blkbits >= > + orig_inode->i_size) { > ext4_debug("ext4 move extent: start offset [%llu] " > "should be less than original file size " > "[%lld] [inode:orig %lu, donor %lu]\n", > - orig_start, orig_inode->i_size, > - orig_inode->i_ino, donor_inode->i_ino); > + orig_start << orig_inode->i_blkbits, > + orig_inode->i_size, orig_inode->i_ino, > + donor_inode->i_ino); > return -EINVAL; > } > - > - if (orig_start + *len > orig_inode->i_size) { > + if ((orig_start + *len) << orig_inode->i_blkbits > > + orig_inode->i_size) { > ext4_debug("ext4 move extent: Adjust length " > "from %llu to %lld. Because it should be " > "less than original file size " > "[ino:orig %lu, donor %lu]\n", > - *len, orig_inode->i_size - orig_start, > + *len, (orig_inode->i_size >> > + orig_inode->i_blkbits) - orig_start, > orig_inode->i_ino, donor_inode->i_ino); > - *len = orig_inode->i_size - orig_start; > + *len = (orig_inode->i_size >> orig_inode->i_blkbits) - > + orig_start; > } > } > -- > 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 > -- Best Regards, Peng Tao State Key Laboratory of Networking and Switching Technology Beijing Univ. of Posts and Telecoms. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() 2009-09-02 3:17 [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() Akira Fujita 2009-09-02 16:02 ` Peng Tao @ 2009-09-06 2:10 ` Theodore Tso 2009-09-16 4:56 ` Akira Fujita 1 sibling, 1 reply; 5+ messages in thread From: Theodore Tso @ 2009-09-06 2:10 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4 On Wed, Sep 02, 2009 at 12:17:50PM +0900, Akira Fujita wrote: > ext4: Fix wrong comparisons in mext_check_arguments() > > From: Akira Fujita <a-fujita@rs.jp.nec.com> > > mext_check_arguments() in move_extents.c has wrong comparisons. > orig_start which is passed from user-space is block unit, > but i_size of inode is byte unit, therefore the checks do not work fine. > This mis-check leads to the overflow of 'len' and then hits BUG_ON() > in ext4_move_extens(). The patch fixes this issue. Thanks, I've added this to the ext4 patch queue to be pushed when the 2.6.32 merge window opens. - Ted ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() 2009-09-06 2:10 ` Theodore Tso @ 2009-09-16 4:56 ` Akira Fujita 2009-09-16 18:37 ` Theodore Tso 0 siblings, 1 reply; 5+ messages in thread From: Akira Fujita @ 2009-09-16 4:56 UTC (permalink / raw) To: Theodore Tso; +Cc: linux-ext4 Hi Ted, We found that "fix-wrong-comparisons-in-mext_check_arguments" in the ext4 patch queue does not fix the objective issue completely. (The last partial block is not checked in mext_check_arguments().) Could you replace "fix-wrong-comparisons-in-mext_check_arguments" in the ext4 patch queue with this patch ? Regards, Akira Fujita Theodore Tso wrote: > On Wed, Sep 02, 2009 at 12:17:50PM +0900, Akira Fujita wrote: >> ext4: Fix wrong comparisons in mext_check_arguments() >> >> From: Akira Fujita <a-fujita@rs.jp.nec.com> >> >> mext_check_arguments() in move_extents.c has wrong comparisons. >> orig_start which is passed from user-space is block unit, >> but i_size of inode is byte unit, therefore the checks do not work fine. >> This mis-check leads to the overflow of 'len' and then hits BUG_ON() >> in ext4_move_extens(). The patch fixes this issue. > > Thanks, I've added this to the ext4 patch queue to be pushed when the > 2.6.32 merge window opens. > > - Ted Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com> Reviewed-by: Greg Freemyer <greg.freemyer@gmail.com> --- move_extent.c | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 429fb6f..0670464 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -898,6 +898,10 @@ mext_check_arguments(struct inode *orig_inode, struct inode *donor_inode, __u64 orig_start, __u64 donor_start, __u64 *len, __u64 moved_len) { + ext4_lblk_t orig_blocks, donor_blocks; + unsigned int blkbits = orig_inode->i_blkbits; + unsigned int blocksize = 1 << blkbits; + /* Regular file check */ if (!S_ISREG(orig_inode->i_mode) || !S_ISREG(donor_inode->i_mode)) { ext4_debug("ext4 move extent: The argument files should be " @@ -972,43 +976,47 @@ mext_check_arguments(struct inode *orig_inode, } if (orig_inode->i_size > donor_inode->i_size) { - if (orig_start >= donor_inode->i_size) { + donor_blocks = (donor_inode->i_size + blocksize - 1) >> blkbits; + /* TODO: eliminate this artificial restriction */ + if (orig_start >= donor_blocks) { ext4_debug("ext4 move extent: orig start offset " - "[%llu] should be less than donor file size " - "[%lld] [ino:orig %lu, donor_inode %lu]\n", - orig_start, donor_inode->i_size, + "[%llu] should be less than donor file blocks " + "[%u] [ino:orig %lu, donor %lu]\n", + orig_start, donor_blocks, orig_inode->i_ino, donor_inode->i_ino); return -EINVAL; } - if (orig_start + *len > donor_inode->i_size) { + /* TODO: eliminate this artificial restriction */ + if (orig_start + *len > donor_blocks) { ext4_debug("ext4 move extent: End offset [%llu] should " - "be less than donor file size [%lld]." - "So adjust length from %llu to %lld " + "be less than donor file blocks [%u]." + "So adjust length from %llu to %llu " "[ino:orig %lu, donor %lu]\n", - orig_start + *len, donor_inode->i_size, - *len, donor_inode->i_size - orig_start, + orig_start + *len, donor_blocks, + *len, donor_blocks - orig_start, orig_inode->i_ino, donor_inode->i_ino); - *len = donor_inode->i_size - orig_start; + *len = donor_blocks - orig_start; } } else { - if (orig_start >= orig_inode->i_size) { + orig_blocks = (orig_inode->i_size + blocksize - 1) >> blkbits; + if (orig_start >= orig_blocks) { ext4_debug("ext4 move extent: start offset [%llu] " - "should be less than original file size " - "[%lld] [inode:orig %lu, donor %lu]\n", - orig_start, orig_inode->i_size, + "should be less than original file blocks " + "[%u] [ino:orig %lu, donor %lu]\n", + orig_start, orig_blocks, orig_inode->i_ino, donor_inode->i_ino); return -EINVAL; } - if (orig_start + *len > orig_inode->i_size) { + if (orig_start + *len > orig_blocks) { ext4_debug("ext4 move extent: Adjust length " - "from %llu to %lld. Because it should be " - "less than original file size " + "from %llu to %llu. Because it should be " + "less than original file blocks " "[ino:orig %lu, donor %lu]\n", - *len, orig_inode->i_size - orig_start, + *len, orig_blocks - orig_start, orig_inode->i_ino, donor_inode->i_ino); - *len = orig_inode->i_size - orig_start; + *len = orig_blocks - orig_start; } } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() 2009-09-16 4:56 ` Akira Fujita @ 2009-09-16 18:37 ` Theodore Tso 0 siblings, 0 replies; 5+ messages in thread From: Theodore Tso @ 2009-09-16 18:37 UTC (permalink / raw) To: Akira Fujita; +Cc: linux-ext4 On Wed, Sep 16, 2009 at 01:56:04PM +0900, Akira Fujita wrote: > Hi Ted, > > We found that "fix-wrong-comparisons-in-mext_check_arguments" > in the ext4 patch queue does not fix the objective issue completely. > (The last partial block is not checked in mext_check_arguments().) > > Could you replace "fix-wrong-comparisons-in-mext_check_arguments" > in the ext4 patch queue with this patch ? Done, thanks. - Ted ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-09-16 18:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-02 3:17 [PATCH 1/4]ext4: Fix wrong comparisons in mext_check_arguments() Akira Fujita 2009-09-02 16:02 ` Peng Tao 2009-09-06 2:10 ` Theodore Tso 2009-09-16 4:56 ` Akira Fujita 2009-09-16 18:37 ` Theodore Tso
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).