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

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

On Mon, Sep 28, 2026 at 05:56:27PM +0800, Joseph Qi wrote:
> 
> 
> 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@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
> > 
> > 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

Got it and agreed.

- Heming