Re: [PATCH] ocfs2: validate extent list in filecheck repair
From: Heming Zhao
Date: Mon Sep 28 2026 - 01:18:54 EST
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;
}
```
Thanks,
Heming
>
> > +
> > /*
> > * 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;
>