| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Dec 2012 22:36:33 +0800
From: Forrest Liu <forrestl@...ology.com>
To: Ashish Sangwan <ashishsangwan2@...il.com>
Cc: "Theodore Ts'o" <tytso@....edu>,
ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: fix extent tree corruption that incurred by hole punch
Hi Ashish,
There have a chance to do a trivial update, and this case will be
covered in ext4_ext_remove_space.
+ if (err == 0 && eh->eh_entries == 0 && path[depth].p_bh != NULL) {
err = ext4_ext_rm_idx(handle, inode, path + depth);
+ /* If this was the first extent index in node,
+ * propagates the ei_block updation info to top */
+ if(!err) {
+ --depth;
+ path = path + depth;
+ while (--depth >= 0) {
+ if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
+ break;
+ path--;
+ err = ext4_ext_get_access(handle, inode, path);
+ if (err)
+ break;
trivial update, when (path + 1)->p_idx->eh_entries == 0
+ path->p_idx->ei_block =(path +
1)->p_idx->ei_block ;
+ err = ext4_ext_dirty(handle, inode, path);
+ if (err)
+ break;
+ }
+ }
+ }
+
I will trying to reproduce the problem tomorrow.
Thanks,
Forrest
2012/12/6 Ashish Sangwan <ashishsangwan2@...il.com>:
> On Thu, Dec 6, 2012 at 5:05 PM, Forrest Liu <forrestl@...ology.com> wrote:
>> Hi Ashish,
>> Thanks for your review and fixed hole punch bugs.
>>
>> We can't propogate ei_block only in ext4_ext_rm_leave.
>>
>> There have two cases to call ext4_ext_rm_idx
>> 1) extent block has no entry, ext4_ext_rm_leave calls
>> ext4_ext_rm_idx to release extent block.
>>
>
> 1.1) And when the extent block is removed, the corresponding index
> entry has also to be removed. And if this was the first index entry,
> all the other index entries, are also updated. This updation will
> continue till we reach the root of extent tree OR the currently
> updated index entry is not the first entry in the node.
>
>> 2) extent index block has no entry, ext4_ext_remove_space
>> calls ext4_ext_rm_idx to release extent index block.
>>
>> Consider remove third level of extent index block that indexed by
>> second level of extent index, and the extent index in second level
>> is first entry in block.
>>
> So, I think the above case would be handled by point (1.1) i.e. the
> earlier index updation done from within ext4_ext_rm_leaf.
> The earlier ei_block updation will reach to the top of tree.
>
>> In this case, ei_block will not propogate to top level correctly,
>> if we only do propogation in ext4_ext_rm_leave.
>
> And thanks for pointing the problem. Can you reproduce it with my code
> suggesstion even with depth 3?
> If no, than please resend the patch with suggested changes.
> The intent here is to solve the problem without changing any function
> signature and with minimal changes.
>
> Regards,
> Ashish
>>
>> Regards,
>> Forrest
>>
>> 2012/12/5 Ashish Sangwan <ashishsangwan2@...il.com>:
>>> Sorry 1 typo.
>>> It should be
>>> + path->p_idx->ei_block =(path + 1)->p_idx->ei_block ;
>>> instead of
>>> + path->p_idx->ei_block = border;
>>>
>>> On Wed, Dec 5, 2012 at 1:23 PM, Ashish Sangwan <ashishsangwan2@...il.com> wrote:
>>>> I have tested it and the problem is real.
>>>> It happens when depth => 2 and punch hole completely removes any of
>>>> the first extent indexes at depth 1.
>>>> The change in ei_block is not propagated backwards to index at depth 0.
>>>>
>>>> However, AFAICS, the problematic call to ext4_ext_rm_idx is from
>>>> ext4_ext_rm_leaf and there is no need to change in
>>>> ext4_ext_remove_space or in ext4_ext_rm_idx.
>>>>
>>>> Forrest, what do you think about below change ?
>>>> basically its your code, but its been added to ext4_ext_rm_leaf after
>>>> removing index.
>>>>
>>>> @@ -2448,8 +2464,28 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
>>>>
>>>> /* if this leaf is free, then we should
>>>> * remove it from index block above */
>>>> - if (err == 0 && eh->eh_entries == 0 && path[depth].p_bh != NULL)
>>>> + if (err == 0 && eh->eh_entries == 0 && path[depth].p_bh != NULL) {
>>>> err = ext4_ext_rm_idx(handle, inode, path + depth);
>>>> + /* If this was the first extent index in node,
>>>> + * propagates the ei_block updation info to top */
>>>> + if(!err) {
>>>> + --depth;
>>>> + path = path + depth;
>>>> + while (--depth >= 0) {
>>>> + if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
>>>> + break;
>>>> + path--;
>>>> + err = ext4_ext_get_access(handle, inode, path);
>>>> + if (err)
>>>> + break;
>>>> + path->p_idx->ei_block = border;
>>>> + err = ext4_ext_dirty(handle, inode, path);
>>>> + if (err)
>>>> + break;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>>
>>>> On Wed, Dec 5, 2012 at 11:43 AM, Forrest Liu <forrestl@...ology.com> wrote:
>>>>> 2012/12/4 forrest <forrestl@...ology.com>:
>>>>>> This problem is easily to reproduce
>>>>>>
>>>>>> Create a file with size larger than 1GB.
>>>>>>
>>>>>> dd if=/dev/zero of=/test_file bs=1M count=1024
>>>>>>
>>>>>> Punch every even block in test_file, and then use debugfs to dump
>>>>>> extents, followings is dumped result
>>>>>>
>>>>>> 2/ 2 339/340 231197 - 231197 3917597 - 3917597 1
>>>>>> 2/ 2 340/340 231199 - 231199 3917599 - 3917599 1
>>>>>> 0/ 2 2/ 2 231201 - 262143 3901486 30943
>>>>>> 1/ 2 1/ 46 231201 - 231880 3901488 680
>>>>>> 2/ 2 1/340 231201 - 231201 3917601 - 3917601 1
>>>>>> 2/ 2 2/340 231203 - 231203 3917603 - 3917603 1
>>>>>>
>>>>>> Punch blocks #231779 ~#231201 , to remove extent index, and then use
>>>>>> debugfs to dump extents, followings is dumped result
>>>>>>
>>>>>> 2/ 2 340/340 231199 - 231199 3917599 - 3917599 1
>>>>>> 0/ 2 2/ 2 231201 - 262143 3901486 30943
>>>>>> -------> logical block index didn't update when remove extent index
>>>>>> 1/ 2 1/ 45 231881 - 232560 3901490 680
>>>>>> 2/ 2 1/340 231881 - 231881 3918281 - 3918281 1
>>>>>> 2/ 2 2/340 231883 - 231883 3918283 - 3918283 1
>>>>>
>>>>> After blocks #231202~#231880 had been punch out, theses blocks can't map again.
>>>>> If we do map a block in #231202~#231880, we will get error messages
>>>>> like followings.
>>>>>
>>>>> EXT4-fs error (device md2): ext4_ext_search_left:1239: inode #13: comm
>>>>> flush-9:2: ix (231201) != EXT_FIRST_INDEX (1) (depth 0)!
>>>>> EXT4-fs (md2): delayed block allocation failed for inode 13 at logical
>>>>> offset 231203 with max blocks 1 with error -5
>>>>> EXT4-fs (md2): This should not happen!! Data will be lost
>>>>>
>>>>> Regards,
>>>>> Forrest
>>>>>
>>>>>>
>>>>>> 2012/12/4 Forrest Liu <forrestl@...ology.com>:
>>>>>>> Extent indexes didn't update correctly in ext4_ext_rm_idx, when depth
>>>>>>> of extent tree is greater than 1.
>>>>>>>
>>>>>>> Signed-off-by: Forrest Liu <forrestl@...ology.com>
>>>>>>> ---
>>>>>>> fs/ext4/extents.c | 24 ++++++++++++++++++++----
>>>>>>> 1 files changed, 20 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>>>>>> index d3dd618..b10b8c0 100644
>>>>>>> --- a/fs/ext4/extents.c
>>>>>>> +++ b/fs/ext4/extents.c
>>>>>>> @@ -2190,13 +2190,15 @@ errout:
>>>>>>> * removes index from the index block.
>>>>>>> */
>>>>>>> static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>>>>>>> - struct ext4_ext_path *path)
>>>>>>> + struct ext4_ext_path *path, int depth)
>>>>>>> {
>>>>>>> int err;
>>>>>>> ext4_fsblk_t leaf;
>>>>>>> + __le32 border;
>>>>>>>
>>>>>>> /* free index block */
>>>>>>> - path--;
>>>>>>> + depth--;
>>>>>>> + path = path + depth;
>>>>>>> leaf = ext4_idx_pblock(path->p_idx);
>>>>>>> if (unlikely(path->p_hdr->eh_entries == 0)) {
>>>>>>> EXT4_ERROR_INODE(inode, "path->p_hdr->eh_entries == 0");
>>>>>>> @@ -2221,6 +2223,20 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>>>>>>>
>>>>>>> ext4_free_blocks(handle, inode, NULL, leaf, 1,
>>>>>>> EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
>>>>>>> +
>>>>>>> + border = path->p_idx->ei_block;
>>>>>>> + while (--depth >= 0) {
>>>>>>> + if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
>>>>>>> + break;
>>>>>>> + path--;
>>>>>>> + err = ext4_ext_get_access(handle, inode, path);
>>>>>>> + if (err)
>>>>>>> + break;
>>>>>>> + path->p_idx->ei_block = border;
>>>>>>> + err = ext4_ext_dirty(handle, inode, path);
>>>>>>> + if (err)
>>>>>>> + break;
>>>>>>> + }
>>>>>>> return err;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -2557,7 +2573,7 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
>>>>>>> /* if this leaf is free, then we should
>>>>>>> * remove it from index block above */
>>>>>>> if (err == 0 && eh->eh_entries == 0 && path[depth].p_bh != NULL)
>>>>>>> - err = ext4_ext_rm_idx(handle, inode, path + depth);
>>>>>>> + err = ext4_ext_rm_idx(handle, inode, path, depth);
>>>>>>>
>>>>>>> out:
>>>>>>> return err;
>>>>>>> @@ -2760,7 +2776,7 @@ again:
>>>>>>> /* index is empty, remove it;
>>>>>>> * handle must be already prepared by the
>>>>>>> * truncatei_leaf() */
>>>>>>> - err = ext4_ext_rm_idx(handle, inode, path + i);
>>>>>>> + err = ext4_ext_rm_idx(handle, inode, path, i);
>>>>>>> }
>>>>>>> /* root level has p_bh == NULL, brelse() eats this */
>>>>>>> brelse(path[i].p_bh);
>>>>>>> --
>>>>>>> 1.7.5.4
>>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>>>> the body of a message to majordomo@...r.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists