| Message ID | 1308818837-5243-1-git-send-email-sanbai@taobao.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jun 23, 2011 at 4:47 PM, Robin Dong <hao.bigrat@gmail.com> wrote: > If eh_entries is equal to (or greater than) eh_max, the operation of > inserting new extent_idx will make number of entries overflow. > So check eh_entries before inserting the new extent_idx. > > Signed-off-by: Robin Dong <sanbai@taobao.com> > --- > fs/ext4/extents.c | 18 ++++++++++-------- > 1 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index eb63c7b..792e77e 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -776,6 +776,16 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, > logical, le32_to_cpu(curp->p_idx->ei_block)); > return -EIO; > } > + > + if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) > + >= le16_to_cpu(curp->p_hdr->eh_max))) { > + EXT4_ERROR_INODE(inode, > + "eh_entries %d >= eh_max %d!", > + le16_to_cpu(curp->p_hdr->eh_entries), > + le16_to_cpu(curp->p_hdr->eh_max)); > + return -EIO; > + } > + > len = EXT_MAX_INDEX(curp->p_hdr) - curp->p_idx; > if (logical > le32_to_cpu(curp->p_idx->ei_block)) { > /* insert after */ > @@ -805,14 +815,6 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, > ext4_idx_store_pblock(ix, ptr); > le16_add_cpu(&curp->p_hdr->eh_entries, 1); > > - if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) > - > le16_to_cpu(curp->p_hdr->eh_max))) { > - EXT4_ERROR_INODE(inode, > - "eh_entries %d > eh_max %d!", > - le16_to_cpu(curp->p_hdr->eh_entries), > - le16_to_cpu(curp->p_hdr->eh_max)); > - return -EIO; > - } > if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) { condition ix > EXT_LAST_INDEX(curp->p_hdr) can not be true. Right? May be we can remove this if-statement in this patch. Yongqiang. > EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!"); > return -EIO; > -- > 1.7.1 > > -- > 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 >
On 6/23/11 3:47 AM, Robin Dong wrote: > If eh_entries is equal to (or greater than) eh_max, the operation of > inserting new extent_idx will make number of entries overflow. > So check eh_entries before inserting the new extent_idx. Do you have any testcase you can share which shows this bug? Thanks, -Eric > Signed-off-by: Robin Dong <sanbai@taobao.com> > --- > fs/ext4/extents.c | 18 ++++++++++-------- > 1 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index eb63c7b..792e77e 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -776,6 +776,16 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, > logical, le32_to_cpu(curp->p_idx->ei_block)); > return -EIO; > } > + > + if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) > + >= le16_to_cpu(curp->p_hdr->eh_max))) { > + EXT4_ERROR_INODE(inode, > + "eh_entries %d >= eh_max %d!", > + le16_to_cpu(curp->p_hdr->eh_entries), > + le16_to_cpu(curp->p_hdr->eh_max)); > + return -EIO; > + } > + > len = EXT_MAX_INDEX(curp->p_hdr) - curp->p_idx; > if (logical > le32_to_cpu(curp->p_idx->ei_block)) { > /* insert after */ > @@ -805,14 +815,6 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, > ext4_idx_store_pblock(ix, ptr); > le16_add_cpu(&curp->p_hdr->eh_entries, 1); > > - if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) > - > le16_to_cpu(curp->p_hdr->eh_max))) { > - EXT4_ERROR_INODE(inode, > - "eh_entries %d > eh_max %d!", > - le16_to_cpu(curp->p_hdr->eh_entries), > - le16_to_cpu(curp->p_hdr->eh_max)); > - return -EIO; > - } > if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) { > EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!"); > return -EIO; -- 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
On 2011年06月23日 17:00, Yongqiang Yang Wrote: > On Thu, Jun 23, 2011 at 4:47 PM, Robin Dong <hao.bigrat@gmail.com> wrote: >> If eh_entries is equal to (or greater than) eh_max, the operation of >> inserting new extent_idx will make number of entries overflow. >> So check eh_entries before inserting the new extent_idx. >> >> Signed-off-by: Robin Dong <sanbai@taobao.com> >> --- >> �fs/ext4/extents.c | � 18 ++++++++++-------- >> �1 files changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index eb63c7b..792e77e 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c [snip] >> � � � �if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) { > condition ix > EXT_LAST_INDEX(curp->p_hdr) can not be true. Right? > May be we can remove this if-statement in this patch. > > Yongqiang. [snip] Good suggestion. But I suggest us to remove it a little bit later. When we do meta data checksum, the last index/extent record might be used for checksum, the above checking might still be helpful for bug probing. Thanks. Coly -- 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
On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <sandeen@redhat.com> wrote: > On 6/23/11 3:47 AM, Robin Dong wrote: >> If eh_entries is equal to (or greater than) eh_max, the operation of >> inserting new extent_idx will make number of entries overflow. >> So check eh_entries before inserting the new extent_idx. > > Do you have any testcase you can share which shows this bug? I am not sure if Robin has any test case. According to code, I think there is no bug case. Because this function is called by ext4_ext_split() and ext4_ext_split() is called only if the index block has free space. I think the right logic should be as this patch shows, that is, we should lookup the capacity before insertion. Yongqiang. > > Thanks, > -Eric > >> Signed-off-by: Robin Dong <sanbai@taobao.com> >> --- >> fs/ext4/extents.c | 18 ++++++++++-------- >> 1 files changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index eb63c7b..792e77e 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -776,6 +776,16 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, >> logical, le32_to_cpu(curp->p_idx->ei_block)); >> return -EIO; >> } >> + >> + if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) >> + >= le16_to_cpu(curp->p_hdr->eh_max))) { >> + EXT4_ERROR_INODE(inode, >> + "eh_entries %d >= eh_max %d!", >> + le16_to_cpu(curp->p_hdr->eh_entries), >> + le16_to_cpu(curp->p_hdr->eh_max)); >> + return -EIO; >> + } >> + >> len = EXT_MAX_INDEX(curp->p_hdr) - curp->p_idx; >> if (logical > le32_to_cpu(curp->p_idx->ei_block)) { >> /* insert after */ >> @@ -805,14 +815,6 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, >> ext4_idx_store_pblock(ix, ptr); >> le16_add_cpu(&curp->p_hdr->eh_entries, 1); >> >> - if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) >> - > le16_to_cpu(curp->p_hdr->eh_max))) { >> - EXT4_ERROR_INODE(inode, >> - "eh_entries %d > eh_max %d!", >> - le16_to_cpu(curp->p_hdr->eh_entries), >> - le16_to_cpu(curp->p_hdr->eh_max)); >> - return -EIO; >> - } >> if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) { >> EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!"); >> return -EIO; > > -- > 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 >
2011/6/24 Yongqiang Yang <xiaoqiangnk@gmail.com>: > On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <sandeen@redhat.com> wrote: >> On 6/23/11 3:47 AM, Robin Dong wrote: >>> If eh_entries is equal to (or greater than) eh_max, the operation of >>> inserting new extent_idx will make number of entries overflow. >>> So check eh_entries before inserting the new extent_idx. >> >> Do you have any testcase you can share which shows this bug? > I am not sure if Robin has any test case. > > According to code, I think there is no bug case. Because this > function is called by ext4_ext_split() and ext4_ext_split() is called > only if the index block has free space. > > I think the right logic should be as this patch shows, that is, we > should lookup the capacity before insertion. Exactly! :-)
On Fri, 24 Jun 2011, Robin Dong wrote: > 2011/6/24 Yongqiang Yang <xiaoqiangnk@gmail.com>: > > On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <sandeen@redhat.com> wrote: > >> On 6/23/11 3:47 AM, Robin Dong wrote: > >>> If eh_entries is equal to (or greater than) eh_max, the operation of > >>> inserting new extent_idx will make number of entries overflow. > >>> So check eh_entries before inserting the new extent_idx. > >> > >> Do you have any testcase you can share which shows this bug? > > I am not sure if Robin has any test case. > > > > According to code, I think there is no bug case. Because this > > function is called by ext4_ext_split() and ext4_ext_split() is called > > only if the index block has free space. > > > > I think the right logic should be as this patch shows, that is, we > > should lookup the capacity before insertion. > > Exactly! :-) Hi Robin, this is the reason why I asked you to provide better commit description with better reasoning for this change. Since it is not immediately clear from the patch itself why you did the change, it saves time for everyone (just FYI for the next time:). Thanks! -Lukas
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index eb63c7b..792e77e 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -776,6 +776,16 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, logical, le32_to_cpu(curp->p_idx->ei_block)); return -EIO; } + + if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) + >= le16_to_cpu(curp->p_hdr->eh_max))) { + EXT4_ERROR_INODE(inode, + "eh_entries %d >= eh_max %d!", + le16_to_cpu(curp->p_hdr->eh_entries), + le16_to_cpu(curp->p_hdr->eh_max)); + return -EIO; + } + len = EXT_MAX_INDEX(curp->p_hdr) - curp->p_idx; if (logical > le32_to_cpu(curp->p_idx->ei_block)) { /* insert after */ @@ -805,14 +815,6 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode, ext4_idx_store_pblock(ix, ptr); le16_add_cpu(&curp->p_hdr->eh_entries, 1); - if (unlikely(le16_to_cpu(curp->p_hdr->eh_entries) - > le16_to_cpu(curp->p_hdr->eh_max))) { - EXT4_ERROR_INODE(inode, - "eh_entries %d > eh_max %d!", - le16_to_cpu(curp->p_hdr->eh_entries), - le16_to_cpu(curp->p_hdr->eh_max)); - return -EIO; - } if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) { EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!"); return -EIO;
If eh_entries is equal to (or greater than) eh_max, the operation of inserting new extent_idx will make number of entries overflow. So check eh_entries before inserting the new extent_idx. Signed-off-by: Robin Dong <sanbai@taobao.com> --- fs/ext4/extents.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)