Re: [PATCH] ocfs2: validate extent list in filecheck repair

[Joseph Qi]

On 9/28/26 1:18 PM, Heming Zhao wrote:
> On Mon, Sep 28, 2026 at 09:33:12AM +0800, Joseph Qi wrote:
>>
>>
>> On 9/26/26 9:18 PM, Jiale Yao wrote:
>>> ocfs2_filecheck_validate_inode_block() does not validate the embedded
>>> extent list, while ocfs2_filecheck_repair_inode_block() only clamps
>>> l_next_free_rec to l_count.  The normal inode read path requires l_count
>>> to be non-zero, limits it to the number of extent records that fit in the
>>> inode, and requires l_next_free_rec not to exceed l_count.
>>>
>>> Without the same checks, filecheck can report SUCCESS while leaving an
>>> invalid extent list on disk.  A later read through the normal inode
>>> validation path rejects the inode and makes the filesystem read-only.
>>>
>>> Add a shared helper for the filecheck paths to check these invariants.
>>> The filecheck validator now rejects all three cases.  The repair path
>>> still clamps l_next_free_rec, but refuses to repair zero or oversized
>>> l_count values.
>>>
>>> Fixes: d56a8f32e4c6 ("ocfs2: check/fix inode block for online file check")
>>
>> I'd like make this as a new ability rather than a bug fix.
>>
>>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>>> ---
>>>  fs/ocfs2/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 71 insertions(+)
>>>
>>> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
>>> index 180107a11046..57168ed02915 100644
>>> --- a/fs/ocfs2/inode.c
>>> +++ b/fs/ocfs2/inode.c
>>> @@ -249,6 +249,41 @@ static int ocfs2_dinode_has_extents(struct ocfs2_dinode *di)
>>>  	return 1;
>>>  }
>>>  
>>> +enum ocfs2_extent_list_status {
>>> +	OCFS2_EXTENT_LIST_OK,
>>> +	OCFS2_EXTENT_LIST_ZERO_COUNT,
>>> +	OCFS2_EXTENT_LIST_OVERSIZED,
>>> +	OCFS2_EXTENT_LIST_BAD_NEXT_FREE,
>>> +};
>>> +
>>> +static enum ocfs2_extent_list_status
>>> +ocfs2_check_extent_list(struct super_block *sb, struct ocfs2_dinode *di)
>>> +{
>>> +	struct ocfs2_extent_list *el = &di->id2.i_list;
>>> +	u16 count;
>>> +	u16 next_free;
>>> +
>>> +	if (!ocfs2_dinode_has_extents(di))
>>> +		return OCFS2_EXTENT_LIST_OK;
>>> +
>>> +	count = le16_to_cpu(el->l_count);
>>> +	next_free = le16_to_cpu(el->l_next_free_rec);
>>> +	if (count == 0)
>>> +		return OCFS2_EXTENT_LIST_ZERO_COUNT;
>>> +	/*
>>> +	 * The exact capacity depends on i_xattr_inline_size, another
>>> +	 * unvalidated on-disk field. Inline xattrs only shrink the
>>> +	 * list, so the no-xattr maximum is a safe upper bound that a
>>> +	 * valid l_count never exceeds.
>>> +	 */
>>> +	if (count > ocfs2_extent_recs_per_inode(sb))
>>> +		return OCFS2_EXTENT_LIST_OVERSIZED;
>>> +	if (next_free > count)
>>> +		return OCFS2_EXTENT_LIST_BAD_NEXT_FREE;
>>> +
>>> +	return OCFS2_EXTENT_LIST_OK;
>>> +}
>>
>> This seems a duplicated check in ocfs2_validate_inode_block().
>> So I sugguest just extract the helper from it instead of duplicating
>> a new one.
>>
>> BTW, seems we don't have to introduce enum ocfs2_extent_list_status,
>> to make the checks simple.
>>
>> Thanks,
>> Joseph
> 
> Hi Joseph,
> 
> The code logic is identical for ocfs2_validate_inode_block() and
> ocfs2_filecheck_validate_inode_block(), except for their error handling style.
> It seems possible to merge them into ocfs2_validate_inode_block() by adding
> a bool check parameter to distinguish between the two behaviors, and then remove
> ocfs2_filecheck_validate_inode_block().
> 
> The new code logic: replace ocfs2_error() with mlog(), and then "goto bail".
> At the bail label, handle the two cases separately using "if (check)".
> 
> i.e.:
> //adding a new parameter "check"
> int ocfs2_validate_inode_block(struct super_block *sb,
> 			       struct buffer_head *bh, bool check)
> 
> 
> code change from:
> ```
> 	int rc;
> 
> 	... ...
> 
> 	if (!OCFS2_IS_VALID_DINODE(di))
>                 rc = ocfs2_error(sb, "Invalid dinode #%llu: signature = %.*s\n",
> 		     (unsigned long long)bh->b_blocknr, 7,
>                      di->i_signature);
>                 goto bail;
>         }
> ```
> 
> to
> ```
> 	int rc = 0;
> 
> 	... ...
> 
> 	if (!OCFS2_IS_VALID_DINODE(di))
>                 mlog(sb, "Invalid dinode #%llu: signature = %.*s\n",
> 		     (unsigned long long)bh->b_blocknr, 7,
>                      di->i_signature);
>                 goto bail;
>         }
> 
> 	... ...
> 
> 	return rc;
> 
> bail:
> 	if (check) {
> 		rc = -OCFS2_FILECHECK_ERR_INVALIDINO;
> 	} else {
> 		rc = ocfs2_error(sb, "invalid dinode\n");
> 	}
> 
> 	return rc;
> }
> ```

Ummm... This may make the code messy.
I'd like only abtract the check condition, and let the return code and
log message as in its own validation function.
e.g. ocfs2_dinode_has_size_without_clusters().

Thanks,
Joseph