Re: [PATCH] ext4: skip split extent recovery on corruption

[Ojaswin Mujoo]

On Tue, Mar 24, 2026 at 09:40:25AM +0800, hongao wrote:
> Hi Jan, Yi, Ojaswin,
> 
> Thank you for reviewing the patch.
> 
> I will send a v2 that moves the p_ext validation before
> ext4_ext_get_access(), as Yi suggested.
> 
> Also, thanks for the Reviewed-by tags:
> Reviewed-by: Jan Kara <jack@xxxxxxx>
> Reviewed-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Regarding Ojaswin's questions:
> 
> I do not have a local reproducer at the moment. My analysis was based on
> the syzbot report, the crash trace, and code inspection.
> 
> For the p_ext == NULL case, a successful ext4_find_extent() only means
> that we were able to walk the tree down to a leaf. It does not guarantee
> that the leaf still contains a valid extent entry for the target logical
> block.
> 
> path[depth].p_ext is derived from the extent entries stored in that leaf.
> If the leaf metadata is already corrupted, ext4_find_extent() may still
> return a non-error path, but p_ext can be NULL because there is no usable
> extent entry there anymore.
> 
> So in the corruption case, a valid path is not enough to continue the
> recovery path safely. Returning -EFSCORRUPTED is safer than
> dereferencing p_ext and crashing while trying to repair already-broken
> metadata.

Hmm so I looked a bit more and this path can lead to an empty extent:

ext4_ext_insert_extent():
  ...
	// Creates an empty leaf with p_ext == 0
	path = ext4_ext_create_new_leaf(handle, inode, mb_flags, gb_flags,
					path, newext);

  // Errors out
	err = ext4_ext_get_access(handle, inode, path + depth);
	if (err)
		goto errout;

Checking ext4_ext_get_access()

Seems like it can return EROFS when journal is aborted or EIO if journal
detected some IO errors on backing device. I guess we have already
handled these by exiting early in your patch, but sure having an extra
check wont hurt. 

Feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

Regards,
Ojaswin

> 
> Thanks,
> Hongao