[PATCH] ocfs2: validate extent list in filecheck repair

[PATCH] ocfs2: validate extent list in filecheck repair

[Jiale Yao]

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")
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;
+}
+
 /*
  * 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;
-- 
2.34.1

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

[Joseph Qi]

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

> +
>  /*
>   * 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;

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

[Heming Zhao]

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

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

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

[Joseph Qi]

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

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

[Heming Zhao]

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

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

[Heming Zhao]

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

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

[jiale yao]

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