Patchwork [v2] ext4: avoid eh_entries overflow before insert extent_idx

login
register
mail settings
Submitter Robin Dong
Date June 23, 2011, 8:47 a.m.
Message ID <1308818837-5243-1-git-send-email-sanbai@taobao.com>
Download mbox | patch
Permalink /patch/101603/
State Superseded
Headers show

Comments

Robin Dong - June 23, 2011, 8:47 a.m.
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(-)
Yongqiang Yang - June 23, 2011, 9 a.m.
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
>
Eric Sandeen - June 23, 2011, 2:57 p.m.
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
Coly Li - June 23, 2011, 4:51 p.m.
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
Yongqiang Yang - June 24, 2011, 6:49 a.m.
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
>
Robin Dong - June 24, 2011, 8:27 a.m.
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!  :-)
Lukas Czerner - June 24, 2011, 8:39 a.m.
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

Patch

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;