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

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

See v2 in, https://lore.kernel.org/all/20260929132818.259870-1-yaojiale02@163.com/
At 2026-09-28 16:02:47, "jiale yao" <19888972804@163.com> wrote:
>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;
>>>