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

[Heming Zhao <heming.zhao@suse.com>]

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;
>