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

["jiale yao" <19888972804@163.com>]

Thanks for the review. I agree that this should be treated as an
extension of online filecheck rather than a fix for d56a8f32e4c6.

I will prepare a v2 with the following changes:

- Drop the Fixes and stable tags and describe the change as additional
   filecheck validation.
- Extract the existing extent-list checks from
   ocfs2_validate_inode_block() into a small shared helper instead of
   duplicating them.
- Remove enum ocfs2_extent_list_status.
- Keep the repair policy unchanged: reject an invalid l_count, while
   allowing the existing code to clamp l_next_free_rec to l_count.

Thanks,
Jiale


At 2026-09-28 15:39:14, "Heming Zhao" <heming.zhao@suse.com> 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.
>
>Agree
>
>In my experience, users run online file checks only when inode data is corrupted,
>or when fsck/debugfs has already reported an inode issue prior to the check.
>
>The Fixes tag should only be used for the patch that fixes the unintended panic
>and read-only issue introduced by commit d56a8f32e4c6.
>
>- Heming
>
>> 
>> > Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> > ---
>> >  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
>> 
>> > +
>> >  /*
>> >   * here's how inodes get read from disk:
>> >   * iget5_locked -> find_actor -> OCFS2_FIND_ACTOR
>> > @@ -1835,6 +1870,33 @@ static int ocfs2_filecheck_validate_inode_block(struct super_block *sb,
>> >  		goto bail;
>> >  	}
>> >  
>> > +	switch (ocfs2_check_extent_list(sb, di)) {
>> > +	case OCFS2_EXTENT_LIST_OK:
>> > +		break;
>> > +	case OCFS2_EXTENT_LIST_ZERO_COUNT:
>> > +		mlog(ML_ERROR,
>> > +		     "Filecheck: invalid dinode #%llu: extent list l_count is zero\n",
>> > +		     (unsigned long long)bh->b_blocknr);
>> > +		rc = -OCFS2_FILECHECK_ERR_INVALIDINO;
>> > +		goto bail;
>> > +	case OCFS2_EXTENT_LIST_OVERSIZED:
>> > +		mlog(ML_ERROR,
>> > +		     "Filecheck: invalid dinode #%llu: extent list l_count %u exceeds max %u\n",
>> > +		     (unsigned long long)bh->b_blocknr,
>> > +		     le16_to_cpu(di->id2.i_list.l_count),
>> > +		     ocfs2_extent_recs_per_inode(sb));
>> > +		rc = -OCFS2_FILECHECK_ERR_INVALIDINO;
>> > +		goto bail;
>> > +	case OCFS2_EXTENT_LIST_BAD_NEXT_FREE:
>> > +		mlog(ML_ERROR,
>> > +		     "Filecheck: invalid dinode #%llu: extent list l_next_free_rec %u exceeds l_count %u\n",
>> > +		     (unsigned long long)bh->b_blocknr,
>> > +		     le16_to_cpu(di->id2.i_list.l_next_free_rec),
>> > +		     le16_to_cpu(di->id2.i_list.l_count));
>> > +		rc = -OCFS2_FILECHECK_ERR_INVALIDINO;
>> > +		goto bail;
>> > +	}
>> > +
>> >  	if (ocfs2_dinode_has_size_without_clusters(sb, di)) {
>> >  		if (S_ISDIR(le16_to_cpu(di->i_mode)))
>> >  			mlog(ML_ERROR,
>> > @@ -1893,6 +1955,15 @@ static int ocfs2_filecheck_repair_inode_block(struct super_block *sb,
>> >  		return -OCFS2_FILECHECK_ERR_VALIDFLAG;
>> >  	}
>> >  
>> > +	switch (ocfs2_check_extent_list(sb, di)) {
>> > +	case OCFS2_EXTENT_LIST_OK:
>> > +	case OCFS2_EXTENT_LIST_BAD_NEXT_FREE:
>> > +		break;
>> > +	case OCFS2_EXTENT_LIST_ZERO_COUNT:
>> > +	case OCFS2_EXTENT_LIST_OVERSIZED:
>> > +		return -OCFS2_FILECHECK_ERR_INVALIDINO;
>> > +	}
>> > +
>> >  	if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) {
>> >  		di->i_blkno = cpu_to_le64(bh->b_blocknr);
>> >  		changed = 1;
>>