linux-ext4 - 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: <ZpZDSMFbziWq5xOK@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Tue, 16 Jul 2024 15:24:31 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Baokun Li <libaokun@...weicloud.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>
Subject: Re: [PATCH 02/20] ext4: prevent partial update of the extents path


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

In this case, can we have something like below:

-----------------
ext4_ext_remove_space
  err = ext4_ext_rm_idx (error, path[0].p_bh inconsistent but verified)
  /* 
   * we release buffers of the path but path[0].p_bh is not cleaned up
   * due to other references to it (possible?)
   */

... at a later point...:

ext4_find_extent
  bh = read_extent_tree_block() 
    /* 
     * we get the bh that was left inconsistent previously
     * since its verified, we dont check it again corrupting
     * the lookup
     */

-----------------

Is the above scenario possible? Or would the path[0].p_bh that was
corrupted previously always be reread during the subsequent
ext4_find_extent() lookup?

Thanks again,
Ojaswin