linux-kernel - Re: [PATCH 02/20] ext4: prevent partial update of the extents path

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9fcb1d52-520f-425f-8b83-debeda423483@huaweicloud.com>
Date: Wed, 17 Jul 2024 14:11:27 +0800
From: Baokun Li <libaokun@...weicloud.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
 yi.zhang@...wei.com, yangerkun@...wei.com, Baokun Li <libaokun1@...wei.com>,
 zhanchengbin <zhanchengbin1@...wei.com>, Baokun Li <libaokun@...weicloud.com>
Subject: Re: [PATCH 02/20] ext4: prevent partial update of the extents path

On 2024/7/17 13:29, Ojaswin Mujoo wrote:
> On Tue, Jul 16, 2024 at 07:54:43PM +0800, Baokun Li wrote:
>> Hi Ojaswin,
>>
>> On 2024/7/16 17:54, Ojaswin Mujoo wrote:
>>>>> But the journal will ensure the consistency of the extents path after
>>>>> this patch.
>>>>>
>>>>> When ext4_ext_get_access() or ext4_ext_dirty() returns an error in
>>>>> ext4_ext_rm_idx() and ext4_ext_correct_indexes(), this may cause
>>>>> the extents tree to be inconsistent. But the inconsistency just
>>>>> exists in memory and doesn't land on disk.
>>>>>
>>>>> For ext4_ext_get_access(), the handle must have been aborted
>>>>> when it returned an error, as follows:
>>>> ext4_ext_get_access
>>>>    ext4_journal_get_write_access
>>>>     __ext4_journal_get_write_access
>>>>      err = jbd2_journal_get_write_access
>>>>      if (err)
>>>>        ext4_journal_abort_handle
>>>>> For ext4_ext_dirty(), since path->p_bh must not be null and handle
>>>>> must be valid, handle is aborted anyway when an error is returned:
>>>> ext4_ext_dirty
>>>>    __ext4_ext_dirty
>>>>     if (path->p_bh)
>>>>       __ext4_handle_dirty_metadata
>>>>        if (ext4_handle_valid(handle))
>>>>          err = jbd2_journal_dirty_metadata
>>>>           if (!is_handle_aborted(handle) && WARN_ON_ONCE(err))
>>>>             ext4_journal_abort_handle
>>>>> Thus the extents tree will only be inconsistent in memory, so only
>>>>> the verified bit of the modified buffer needs to be cleared to avoid
>>>>> these inconsistent data being used in memory.
>>>>>
>>>> Regards,
>>>> Baokun
>>> Thanks for the explanation Baokun, so basically we only have the
>>> inconsitency in the memory.
>>>
>>> I do have a followup questions:
>>>
>>> So in the above example, after we have the error, we'll have the buffer
>>> for depth=0 marked as valid but pointing to the wrong ei_block.
>> It looks wrong here. When there is an error, the ei_block of the
>> unmodified buffer with depth=0 is the correct one, it is indeed
>> 'valid' and it is consistent with the disk. Only buffers that were
> Hey Baokun,
>
> Ahh I see now, I was looking at it the wrong way. So basically since
> depth 1 to 4 is inconsistent to the disk we mark then non verified so
> then subsequent lookups can act accordingly.
>
> Thanks for the explanation! I am in the middle of testing this patchset
> with xfstests on a POWERPC system with 64k page size. I'll let you know
> how that goes!
>
> Regards,
> Ojaswin

Hi Ojaswin,

Thank you for the test and feedback!

Cheers,
Baokun