linux-kernel - Re: [PATCH v2 6/8] ext4: Refactor zeroout path and handle all cases

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] [day] [month] [year] [list]

Message-ID: <aW4ZzaPM1Mb1_iwh@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
Date: Mon, 19 Jan 2026 17:17:25 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Zhang Yi <yizhang089@...il.com>
Cc: linux-ext4@...r.kernel.org, "Theodore Ts'o" <tytso@....edu>,
        Ritesh Harjani <ritesh.list@...il.com>, Zhang Yi <yi.zhang@...wei.com>,
        Jan Kara <jack@...e.cz>, libaokun1@...wei.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/8] ext4: Refactor zeroout path and handle all cases

On Sat, Jan 17, 2026 at 04:00:30PM +0800, Zhang Yi wrote:
> On 1/14/2026 10:57 PM, Ojaswin Mujoo wrote:
> > Currently, zeroout is used as a fallback in case we fail to
> > split/convert extents in the "traditional" modify-the-extent-tree way.
> > This is essential to mitigate failures in critical paths like extent
> > splitting during endio. However, the logic is very messy and not easy to
> > follow. Further, the fragile use of various flags has made it prone to
> > errors.
> > 
> > Refactor zeroout out logic by moving it up to ext4_split_extents().
> > Further, zeroout correctly based on the type of conversion we want, ie:
> > - unwritten to written: Zeroout everything around the mapped range.
> > - written to unwritten: Zeroout only the mapped range.
> > 
> > Also, ext4_ext_convert_to_initialized() now passes
> > EXT4_GET_BLOCKS_CONVERT to make the intention clear.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> 
> Overall looks good to me besides one comment below. Feel free to add after
> fixing it:
> 
> Reviewed-by: Zhang Yi <yi.zhang@...wei.com>
> 
> > ---
> >   fs/ext4/extents.c | 286 ++++++++++++++++++++++++++++++----------------
> >   1 file changed, 188 insertions(+), 98 deletions(-)
> > 
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 54f45b40fe73..70d85f007dc7 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -44,14 +44,6 @@
> >   #define EXT4_EXT_MARK_UNWRIT1	0x2  /* mark first half unwritten */
> >   #define EXT4_EXT_MARK_UNWRIT2	0x4  /* mark second half unwritten */

<...>

> > -			split_flag1 |= map->m_lblk > ee_block ?
> > -				       EXT4_EXT_DATA_PARTIAL_VALID1 :
> > -				       EXT4_EXT_DATA_ENTIRE_VALID1;
> >   		path = ext4_split_extent_at(handle, inode, path,
> >   				map->m_lblk + map->m_len, split_flag1, flags1);
> >   		if (IS_ERR(path))
> > -			return path;
> > +			goto try_zeroout;
> > +
> >   		/*
> >   		 * Update path is required because previous ext4_split_extent_at
> >   		 * may result in split of original leaf or extent zeroout.
> >   		 */
> >   		path = ext4_find_extent(inode, map->m_lblk, path, flags);
> >   		if (IS_ERR(path))
> > -			return path;
> > +			goto try_zeroout;
> > +
> >   		depth = ext_depth(inode);
> >   		ex = path[depth].p_ext;
> >   		if (!ex) {
> > @@ -3426,22 +3475,64 @@ static struct ext4_ext_path *ext4_split_extent(handle_t *handle,
> >   			ext4_free_ext_path(path);
> >   			return ERR_PTR(-EFSCORRUPTED);
> >   		}
> > -		unwritten = ext4_ext_is_unwritten(ex);
> 
> We need to update the 'orig_ee_len' parameter here, otherwise
> it would trigger WARN_ON(ee_len != orig_ee_len) below.
> 
> Thanks,
> Yi.

Hi Yi, thanks for the reviews! 

Yes thats a nice catch, I'll change it in v3.

Regards,
ojaswin

> 
> >   	}
> >   	if (map->m_lblk >= ee_block) {
> > -		split_flag1 = split_flag & EXT4_EXT_DATA_VALID2;
> > +		split_flag1 = 0;
> >   		if (unwritten) {
> >   			split_flag1 |= EXT4_EXT_MARK_UNWRIT1;
> > -			split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT |
> > -						     EXT4_EXT_MARK_UNWRIT2);
> > +			split_flag1 |= split_flag & EXT4_EXT_MARK_UNWRIT2;
> >   		}

<...>