* [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
@ 2011-06-23 8:47 Robin Dong
2011-06-23 9:00 ` Yongqiang Yang
2011-06-23 14:57 ` Eric Sandeen
0 siblings, 2 replies; 7+ messages in thread
From: Robin Dong @ 2011-06-23 8:47 UTC (permalink / raw)
To: linux-ext4; +Cc: Robin Dong
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))) {
EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!");
return -EIO;
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-23 8:47 [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx Robin Dong
@ 2011-06-23 9:00 ` Yongqiang Yang
2011-06-23 16:51 ` Coly Li
2011-06-23 14:57 ` Eric Sandeen
1 sibling, 1 reply; 7+ messages in thread
From: Yongqiang Yang @ 2011-06-23 9:00 UTC (permalink / raw)
To: Robin Dong; +Cc: linux-ext4, Robin Dong
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
>
--
Best Wishes
Yongqiang Yang
--
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] 7+ messages in thread* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-23 9:00 ` Yongqiang Yang
@ 2011-06-23 16:51 ` Coly Li
0 siblings, 0 replies; 7+ messages in thread
From: Coly Li @ 2011-06-23 16:51 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: Robin Dong, linux-ext4, Robin Dong
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-23 8:47 [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx Robin Dong
2011-06-23 9:00 ` Yongqiang Yang
@ 2011-06-23 14:57 ` Eric Sandeen
2011-06-24 6:49 ` Yongqiang Yang
1 sibling, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2011-06-23 14:57 UTC (permalink / raw)
To: Robin Dong; +Cc: linux-ext4, Robin Dong
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;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-23 14:57 ` Eric Sandeen
@ 2011-06-24 6:49 ` Yongqiang Yang
2011-06-24 8:27 ` Robin Dong
0 siblings, 1 reply; 7+ messages in thread
From: Yongqiang Yang @ 2011-06-24 6:49 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Robin Dong, linux-ext4, Robin Dong
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
>
--
Best Wishes
Yongqiang Yang
--
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] 7+ messages in thread* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-24 6:49 ` Yongqiang Yang
@ 2011-06-24 8:27 ` Robin Dong
2011-06-24 8:39 ` Lukas Czerner
0 siblings, 1 reply; 7+ messages in thread
From: Robin Dong @ 2011-06-24 8:27 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: Eric Sandeen, linux-ext4, Robin Dong
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! :-)
--
--
Best Regard
Robin Dong
>
> 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
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>
--
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] 7+ messages in thread* Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx
2011-06-24 8:27 ` Robin Dong
@ 2011-06-24 8:39 ` Lukas Czerner
0 siblings, 0 replies; 7+ messages in thread
From: Lukas Czerner @ 2011-06-24 8:39 UTC (permalink / raw)
To: Robin Dong; +Cc: Yongqiang Yang, Eric Sandeen, linux-ext4, Robin Dong
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1145 bytes --]
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-06-24 8:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-23 8:47 [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx Robin Dong
2011-06-23 9:00 ` Yongqiang Yang
2011-06-23 16:51 ` Coly Li
2011-06-23 14:57 ` Eric Sandeen
2011-06-24 6:49 ` Yongqiang Yang
2011-06-24 8:27 ` Robin Dong
2011-06-24 8:39 ` Lukas Czerner
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).