2011-06-23 08:47:28

by Robin Dong

Subject: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

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 <[email protected]>
---
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

2011-06-23 09:00:38

by Yongqiang Yang

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

On Thu, Jun 23, 2011 at 4:47 PM, Robin Dong <[email protected]> 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 <[email protected]>
> ---
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>



--
Best Wishes
Yongqiang Yang

2011-06-23 14:57:32

by Eric Sandeen

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

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 <[email protected]>
> ---
> 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;

2011-06-23 16:49:47

by Coly Li

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

On 2011年06月23日 17:00, Yongqiang Yang Wrote:
> On Thu, Jun 23, 2011 at 4:47 PM, Robin Dong <[email protected]> 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 <[email protected]>
>> ---
>> �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

2011-06-24 06:49:35

by Yongqiang Yang

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <[email protected]> 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 <[email protected]>
>> ---
>> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>



--
Best Wishes
Yongqiang Yang

2011-06-24 08:27:30

by Robin Dong

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

2011/6/24 Yongqiang Yang <[email protected]>:
> On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <[email protected]> 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 <[email protected]>
>>> ---
>>> ?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 [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>

2011-06-24 08:39:45

by Lukas Czerner

Subject: Re: [PATCH v2] ext4: avoid eh_entries overflow before insert extent_idx

On Fri, 24 Jun 2011, Robin Dong wrote:

> 2011/6/24 Yongqiang Yang <[email protected]>:
> > On Thu, Jun 23, 2011 at 10:57 PM, Eric Sandeen <[email protected]> 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