Patchwork ext4: Check extent overflow with the right range.

login
register
mail settings
Submitter Tao Ma
Date Oct. 17, 2011, 8:45 a.m.
Message ID <1318841128-4540-1-git-send-email-tm@tao.ma>
Download mbox | patch
Permalink /patch/120114/
State Superseded
Headers show

Comments

Tao Ma - Oct. 17, 2011, 8:45 a.m.
From: Tao Ma <boyu.mt@taobao.com>

In 4fd30c033, we move the range check before we change ix to avoid the memory stamp.
But actually we should check against the EXT_MAX_INDEX, not EXT_LAST_INDEX.

So this patch revert the old patch and adds a new check before we setting ix.

Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
Ted, since 4fd30c033 is in your dev branch, I am fine to integrate these 2 patches to one.

 fs/ext4/extents.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Yongqiang Yang - Oct. 17, 2011, 9:10 a.m.
On Mon, Oct 17, 2011 at 4:45 PM, Tao Ma <tm@tao.ma> wrote:
> From: Tao Ma <boyu.mt@taobao.com>
>
> In 4fd30c033, we move the range check before we change ix to avoid the memory stamp.
> But actually we should check against the EXT_MAX_INDEX, not EXT_LAST_INDEX.
>
> So this patch revert the old patch and adds a new check before we setting ix.
>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
> Ted, since 4fd30c033 is in your dev branch, I am fine to integrate these 2 patches to one.
>
>  fs/ext4/extents.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 2dff31e..5c48612 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -779,8 +779,8 @@ static int ext4_ext_insert_index(handle_t *handle, struct inode *inode,
>                ix = curp->p_idx;
>        }
>
> -       if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) {
> -               EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!");
> +       if (unlikely(ix > EXT_MAX_INDEX(curp->p_hdr))) {
> +               EXT4_ERROR_INODE(inode, "ix > EXT_MAX_INDEX!");
>                return -EIO;
>        }
Hi Tao,

I think original LAST_INDEX check aims to verify all index entries are
contiguous, because max entries bas been checked by eh_entries >=
ex_max in previous code.
>
> @@ -788,6 +788,11 @@ 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(ix > EXT_LAST_INDEX(curp->p_hdr))) {
> +               EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!");
> +               return -EIO;
> +       }
> +
Actually I do not think we need a verification here.  The code is
short and we know that index entries are contiguous.


Yongqiang.
>        err = ext4_ext_dirty(handle, inode, curp);
>        ext4_std_error(inode->i_sb, err);
>
> --
> 1.6.3.3.334.g916e1.dirty
>
> --
> 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
>
Theodore Ts'o - Oct. 17, 2011, 2:16 p.m.
On Mon, Oct 17, 2011 at 04:45:28PM +0800, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> In 4fd30c033, we move the range check before we change ix to avoid the memory stamp.
> But actually we should check against the EXT_MAX_INDEX, not EXT_LAST_INDEX.
> 
> So this patch revert the old patch and adds a new check before we setting ix.
> 
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
> ---
> Ted, since 4fd30c033 is in your dev branch, I am fine to integrate these 2 patches to one.


Hi Tao,

I have integrated this with the the existing patch in dev branch (since
it isn't in the master branch yet, and hence hasn't been locked down).

I'm wondering though how you originally ran into this case, though.
If there is no more room in the tree node, we should have never
entered ext4_ext_extent_index() in the first place.  Was this a
theoretical concern or did you actually see this happen.  And if so,
was it due to a file system corruption, or another bug someplace else.

Thanks,

						- Ted
--
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
Tao Ma - Oct. 18, 2011, 2:27 a.m.
Hi Ted,
On 10/17/2011 10:16 PM, Ted Ts'o wrote:
> On Mon, Oct 17, 2011 at 04:45:28PM +0800, Tao Ma wrote:
>> From: Tao Ma <boyu.mt@taobao.com>
>>
>> In 4fd30c033, we move the range check before we change ix to avoid the memory stamp.
>> But actually we should check against the EXT_MAX_INDEX, not EXT_LAST_INDEX.
>>
>> So this patch revert the old patch and adds a new check before we setting ix.
>>
>> Cc: "Theodore Ts'o" <tytso@mit.edu>
>> Signed-off-by: Tao Ma <boyu.mt@taobao.com>
>> ---
>> Ted, since 4fd30c033 is in your dev branch, I am fine to integrate these 2 patches to one.
> 
> 
> Hi Tao,
> 
> I have integrated this with the the existing patch in dev branch (since
> it isn't in the master branch yet, and hence hasn't been locked down).
> 
> I'm wondering though how you originally ran into this case, though.
> If there is no more room in the tree node, we should have never
> entered ext4_ext_extent_index() in the first place.  Was this a
> theoretical concern or did you actually see this happen.  And if so,
> was it due to a file system corruption, or another bug someplace else.
We have met with some corruption, but not sure whether it is related to
this or not. To be honest, we went through the code and find that this
may be a possible cause. So if you think it could never happen, we are
OK to drop this patch.

Thanks
Tao
--
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
Theodore Ts'o - Oct. 18, 2011, 3:07 p.m.
On Tue, Oct 18, 2011 at 10:27:32AM +0800, Tao Ma wrote:
> We have met with some corruption, but not sure whether it is related to
> this or not. To be honest, we went through the code and find that this
> may be a possible cause. So if you think it could never happen, we are
> OK to drop this patch.

I have no problem adding sanity checks; it makes for more robust code.
I just wanted to make sure there wasn't a real bug hiding somewhere
that wasn't getting addressed.

Thanks,

					- Ted
--
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

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 2dff31e..5c48612 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -779,8 +779,8 @@  static int ext4_ext_insert_index(handle_t *handle, struct inode *inode,
 		ix = curp->p_idx;
 	}
 
-	if (unlikely(ix > EXT_LAST_INDEX(curp->p_hdr))) {
-		EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!");
+	if (unlikely(ix > EXT_MAX_INDEX(curp->p_hdr))) {
+		EXT4_ERROR_INODE(inode, "ix > EXT_MAX_INDEX!");
 		return -EIO;
 	}
 
@@ -788,6 +788,11 @@  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(ix > EXT_LAST_INDEX(curp->p_hdr))) {
+		EXT4_ERROR_INODE(inode, "ix > EXT_LAST_INDEX!");
+		return -EIO;
+	}
+
 	err = ext4_ext_dirty(handle, inode, curp);
 	ext4_std_error(inode->i_sb, err);