Re: [PATCH] ocfs2: validate extent list in filecheck repair
From: jiale yao
Date: Mon Sep 28 2026 - 04:08:25 EST
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@xxxxxxxx> 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@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
>>
>> > +
>> > /*
>> > * 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;
>>