Re: [PATCH v4 9/9] ext4: protect WRITE_ZEROES written extents with orphan list

Hi, Ted!

On 7/23/2026 1:07 AM, Theodore Tso wrote:
> On Tue, Jul 14, 2026 at 08:42:03AM -0500, [email protected] wrote:
>>> @@ -4701,6 +4729,11 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>>>  		pagecache_isize_extended(inode, old_size, epos);
>>>  
>>>  	return ret ? ret : ret2;
>>> +
>>> +out:
>>> +	if (orphan && inode->i_nlink)
>>> +		ext4_orphan_del(NULL, inode);
>>
>> [Severity: Critical]
>> Does passing a NULL handle to ext4_orphan_del() here corrupt the on-disk
>> orphan list?
>>
>> If ext4_convert_unwritten_extents() fails on the first iteration, epos is 0.
>> The code skips starting a new journal handle, jumps to out: and calls
>> ext4_orphan_del(NULL, inode).
>>
>> A NULL handle removes the inode from the in-memory list but leaves it
>> permanently linked in the on-disk list.  If this inode is later unlinked and
>> freed, could this sever the rest of the on-disk orphan list and lead to severe
>> filesystem corruption during recovery if the inode is reallocated?
> 
> This looks like a valid problem; it's possible to get to this point in
> the code:
> 
>         if (!epos || !new_size)
>                 goto out;
> 
> where we've already added the inode to the orphan list, and we will
> then leave the inode on the orphan list.
> 
> I think we need to fix this by checking to see if orphan == true, and
> if so, we need to start a handle, and then call ext4_orphan_del() with
> that handle, and stop it afterwards.
> 
> Do you agree?

After thorough analysis, I agree that this is indeed a real issue.
However, we cannot simply initiate a handle to remove the orphan list,
because ext4_convert_unwritten_extents() may have already converted
some extents to written even when it returns an error (in which case
epos will be 0). If we remove the on-disk orphan list under such
circumstances, it will lead to filesystem corruption.

Therefore, I think the correct approach is to modify
ext4_convert_unwritten_extents() so that it returns the actual
conversion length, and only returns an error code when no conversion has
taken place at all. This way, ext4_alloc_file_blocks() can use the
returned conversion length to decide whether to add the orphan list
(within the same handle as the conversion operation).

What do you think?

> 
> If so, are you OK if I just drop this last patch in the series, and
> you can resubmit with the fix?

Sure, please drop this patch for now. I'll send a separate series to
address this issue.

Thanks,
Yi.

> 
> Thaks,
> 
> 						- Ted
>