[PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[pipishuo]

After a failure in the route ext4_restore_inline_data ->
ext4_create_inline_data -> ext4_mark_iloc_dirty, the inode will still
be considered as having inline data. However, in ext4_restore_inline_data,
the inline data is not actually written to the inode. When ext4_readdir
is subsequently called,ext4_has_inline_data will pass the check
and proceed to read the incorrect inline data.

Signed-off-by: pipishuo <1289151713@qq.com>
---
 fs/ext4/dir.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 17edd678fa87..6117a798541a 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -22,6 +22,7 @@
  *
  */
 
+#include <asm-generic/errno-base.h>
 #include <linux/fs.h>
 #include <linux/buffer_head.h>
 #include <linux/filelock.h>
@@ -159,6 +160,9 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 	}
 
 	if (ext4_has_inline_data(inode)) {
+		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
+			return -EINVAL;
+
 		int has_inline_data = 1;
 		err = ext4_read_inline_dir(file, ctx,
 					   &has_inline_data);
-- 
2.47.3

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning `-EINVAL` from `ext4_readdir` on a corrupted or inconsistent directory state breaks userspace expectations and bypasses filesystem error handling.
--

--- Patch [1]: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
commit 419471105d79565b3d6563ce7e941b266526160b
Author: pipishuo <1289151713@qq.com>

Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

This commit adds a check for EXT4_STATE_MAY_INLINE_DATA in ext4_readdir to
prevent reading incorrect inline data if the inode's inline state is
inconsistent after a failure in ext4_restore_inline_data.

> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 8d7b81e6948e7..857cf02742c0c 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c

[ ... ]

> @@ -160,6 +161,9 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  	}
>  
>  	if (ext4_has_inline_data(inode)) {
> +		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> +			return -EINVAL;
> +

[Severity: High]
Does returning -EINVAL here break userspace expectations and bypass the
standard filesystem corruption handling?

If an operation (like ext4_convert_inline_data_nolock) fails and leaves
EXT4_INODE_INLINE_DATA set without EXT4_STATE_MAY_INLINE_DATA, this
indicates an internally corrupted or inconsistent directory state.

By returning -EINVAL directly, this code bypasses the standard
ext4_error_file() error handler, which prevents the filesystem from taking
corrective action (such as remounting read-only).

Additionally, returning -EINVAL from a readdir (getdents64) operation is
typically interpreted by userspace as a buffer size issue, which completely
misleads tools like ls. Should this instead invoke EXT4_ERROR_FILE() and
return -EFSCORRUPTED?

>  		int has_inline_data = 1;
>  		err = ext4_read_inline_dir(file, ctx,
>  					   &has_inline_data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_321031C6B9D5D092C8978B46D9F0BBA18607@qq.com?part=1

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

On Thu, Aug 13, 2026 at 08:49:27AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Returning `-EINVAL` from `ext4_readdir` on a corrupted or inconsistent directory state breaks userspace expectations and bypasses filesystem error handling.
> --
> 
> --- Patch [1]: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
> commit 419471105d79565b3d6563ce7e941b266526160b
> Author: pipishuo <1289151713@qq.com>
> 
> Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
> 
> This commit adds a check for EXT4_STATE_MAY_INLINE_DATA in ext4_readdir to
> prevent reading incorrect inline data if the inode's inline state is
> inconsistent after a failure in ext4_restore_inline_data.
> 
> > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> > index 8d7b81e6948e7..857cf02742c0c 100644
> > --- a/fs/ext4/dir.c
> > +++ b/fs/ext4/dir.c
> 
> [ ... ]
> 
> > @@ -160,6 +161,9 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> >  	}
> >  
> >  	if (ext4_has_inline_data(inode)) {
> > +		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> > +			return -EINVAL;
> > +
> 
> [Severity: High]
> Does returning -EINVAL here break userspace expectations and bypass the
> standard filesystem corruption handling?
> 
> If an operation (like ext4_convert_inline_data_nolock) fails and leaves
> EXT4_INODE_INLINE_DATA set without EXT4_STATE_MAY_INLINE_DATA, this
> indicates an internally corrupted or inconsistent directory state.
> 
> By returning -EINVAL directly, this code bypasses the standard
> ext4_error_file() error handler, which prevents the filesystem from taking
> corrective action (such as remounting read-only).
> 
> Additionally, returning -EINVAL from a readdir (getdents64) operation is
> typically interpreted by userspace as a buffer size issue, which completely
> misleads tools like ls. Should this instead invoke EXT4_ERROR_FILE() and
> return -EFSCORRUPTED?
Thanks for the reviews. I'll address all the comments and send v2 soon.
> 
> >  		int has_inline_data = 1;
> >  		err = ext4_read_inline_dir(file, ctx,
> >  					   &has_inline_data);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/tencent_321031C6B9D5D092C8978B46D9F0BBA18607@qq.com?part=1

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Theodore Tso]

On Thu, Aug 13, 2026 at 04:33:57PM -0500, pipishuo wrote:
> After a failure in the route ext4_restore_inline_data ->
> ext4_create_inline_data -> ext4_mark_iloc_dirty, the inode will still
> be considered as having inline data.

How did you run across this?  Was this something that was actually
happening?  If so, what was triggering the error?

Or was this something that was found via AI?

I'm fairly sure this patch was created using AI.  This useless
inclusion was a bit of a tip-off:

> +#include <asm-generic/errno-base.h>

As far as the fix is concerned, there's a much better way of
implementing the functionality in ext4_restore_inline_data(), which is
to just capture a copy of the inode (since the inline data is stored
in the inode), and if we need to back out a failed conversion in
ext4_convert_inline_data_nolock(), we can just copy the old contents
of the inode back in place.

We can also do the conversion in a much safer way, by allocating the
data block first, and writing a copy of the data to the data block,
and only if that is successful, do we update the extent tree or
indirect block mapping in the inode.  Of course, this would require a
rewrite of ext4_convert_inline_data_nolock(), but this would allow us
to fix up the other aspects of the error handling which is not quite
right.

						- Ted

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

On Thu, Aug 13, 2026 at 11:31:03PM -0400, Theodore Tso wrote:
> On Thu, Aug 13, 2026 at 04:33:57PM -0500, pipishuo wrote:
> > After a failure in the route ext4_restore_inline_data ->
> > ext4_create_inline_data -> ext4_mark_iloc_dirty, the inode will still
> > be considered as having inline data.
> 
> How did you run across this?  Was this something that was actually
> happening?  If so, what was triggering the error?
> 
> Or was this something that was found via AI?
I didn't have a reproducer. I was reviewing the inline data error paths 
and noticed the interaction with ext4_readdir looked fragile
so I attempted to make it more robust.
> 
> I'm fairly sure this patch was created using AI.  This useless
> inclusion was a bit of a tip-off:
> 
> > +#include <asm-generic/errno-base.h>
It was automatically inserted by my editor's include suggestion feature,
and I missed it in review. That's entirely my fault.
> 
> As far as the fix is concerned, there's a much better way of
> implementing the functionality in ext4_restore_inline_data(), which is
> to just capture a copy of the inode (since the inline data is stored
> in the inode), and if we need to back out a failed conversion in
> ext4_convert_inline_data_nolock(), we can just copy the old contents
> of the inode back in place.
Because ext4_convert_inline_data_nolock() has already called
ext4_destroy_inline_data_nolock(), the inode is no longer in the inline
state. So we need ext4_create_inline_data() to rebuild the inline state.
> 
> We can also do the conversion in a much safer way, by allocating the
> data block first, and writing a copy of the data to the data block,
> and only if that is successful, do we update the extent tree or
> indirect block mapping in the inode.  Of course, this would require a
> rewrite of ext4_convert_inline_data_nolock(), but this would allow us
> to fix up the other aspects of the error handling which is not quite
> right. 
I agree with your direction. I'll take some time to study the code 
and work on a proper v2 that rewrites ext4_convert_inline_data_nolock()
following that model.
> 
> 						- Ted

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Theodore Tso]

On Fri, Aug 14, 2026 at 04:40:02PM -0500, shuo chen wrote:
> Because ext4_convert_inline_data_nolock() has already called
> ext4_destroy_inline_data_nolock(), the inode is no longer in the inline
> state. So we need ext4_create_inline_data() to rebuild the inline state.

There are other ways to restore the inline state than calling ext4_create_inline_data().

Cheers,

					- Ted

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

On Fri, Aug 14, 2026 at 09:57:33AM -0400, Theodore Tso wrote:
> On Fri, Aug 14, 2026 at 04:40:02PM -0500, shuo chen wrote:
> > Because ext4_convert_inline_data_nolock() has already called
> > ext4_destroy_inline_data_nolock(), the inode is no longer in the inline
> > state. So we need ext4_create_inline_data() to rebuild the inline state.
> 
> There are other ways to restore the inline state than calling ext4_create_inline_data().
I get it — keep a backup of the inode and restore on rollback.
But I prefer your second (safer) approach — allocate first, then convert.
I'll send a v2 following that.
> 
> Cheers,
> 
> 					- Ted

[PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

> We can also do the conversion in a much safer way, by allocating the
> data block first, and writing a copy of the data to the data block,
> and only if that is successful, do we update the extent tree or
> indirect block mapping in the inode.  Of course, this would require a
> rewrite of ext4_convert_inline_data_nolock(), but this would allow us
> to fix up the other aspects of the error handling which is not quite
> right.
Rewrite ext4_convert_inline_data_nolock() as suggested.

---
v1 -> v2:
- Rewrite ext4_convert_inline_data_nolock() to allocate blocks before
  destroying inline data
- Link to v1: https://lore.kernel.org/linux-ext4/tencent_7A0B62235E38C5907A080BE3D56A59C9FD06@qq.com/T/#t：
---
Signed-off-by: shuo chen <1289151713@qq.com>
---
 fs/ext4/inline.c | 113 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 80 insertions(+), 33 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 8045e4ff270c..2a9299ea4146 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -14,6 +14,7 @@
 #include "ext4.h"
 #include "xattr.h"
 #include "truncate.h"
+#include "ext4_extents.h"
 
 #define EXT4_XATTR_SYSTEM_DATA	"data"
 #define EXT4_MIN_INLINE_DATA_SIZE	((sizeof(__le32) * EXT4_N_BLOCKS))
@@ -1070,21 +1071,67 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
 	return 0;
 }
 
-static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
-				     struct ext4_iloc *iloc,
-				     void *buf, int inline_size)
+static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode,
+				      ext4_fsblk_t block, unsigned int len)
 {
-	int ret;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct ext4_xattr_ibody_find is = {
+		.s = { .not_found = 0, },
+	};
+	struct ext4_xattr_info i = {
+		.name_index = EXT4_XATTR_INDEX_SYSTEM,
+		.name = EXT4_XATTR_SYSTEM_DATA,
+		.value = NULL,
+		.value_len = 0,
+	};
+	int error;
 
-	ret = ext4_create_inline_data(handle, inode, inline_size);
-	if (ret) {
-		ext4_msg(inode->i_sb, KERN_EMERG,
-			"error restoring inline_data for inode -- potential data loss! (inode %llu, error %d)",
-			inode->i_ino, ret);
-		return;
+	down_write(&ei->i_data_sem);
+	error = ext4_get_inode_loc(inode, &is.iloc);
+	if (error) {
+		up_write(&ei->i_data_sem);
+		return error;
 	}
-	ext4_write_inline_data(inode, iloc, buf, 0, inline_size);
-	ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+	error = ext4_xattr_ibody_find(inode, &i, &is);
+	if (error)
+		goto out;
+	BUFFER_TRACE(is.iloc.bh, "get_write_access");
+	error = ext4_journal_get_write_access(handle, inode->i_sb, is.iloc.bh, EXT4_JTR_NONE);
+	if (error)
+		goto out;
+	error = ext4_xattr_ibody_set(handle, inode, &i, &is);
+	if (error)
+		goto out;
+	memset((void *)ext4_raw_inode(&is.iloc)->i_block,
+		0, EXT4_MIN_INLINE_DATA_SIZE);
+	memset(ei->i_data, 0, EXT4_MIN_INLINE_DATA_SIZE);
+
+	if (ext4_has_feature_extents(inode->i_sb) &&
+		(S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode))) {
+		ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
+		ext4_ext_tree_init(handle, inode);
+		struct ext4_ext_path *path = ext4_find_extent(inode, 0, NULL, 0);
+		struct ext4_extent newex;
+
+		newex.ee_block = cpu_to_le32(0);
+		newex.ee_len = cpu_to_le16(len);
+		ext4_ext_store_pblock(&newex, block);
+		ext4_ext_insert_extent(handle, inode, path, &newex, 0);
+	} else {
+		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
+	}
+	ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
+	get_bh(is.iloc.bh);
+	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
+	EXT4_I(inode)->i_inline_off = 0;
+	EXT4_I(inode)->i_inline_size = 0;
+	ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+out:
+	brelse(is.iloc.bh);
+	if (error == -ENODATA)
+		error = 0;
+	up_write(&ei->i_data_sem);
+	return error;
 }
 
 static int ext4_convert_inline_data_nolock(handle_t *handle,
@@ -1094,8 +1141,9 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	int error;
 	void *buf = NULL;
 	struct buffer_head *data_bh = NULL;
-	struct ext4_map_blocks map;
 	int inline_size;
+	ext4_fsblk_t newblock = 0;
+	struct ext4_allocation_request ar;
 
 	inline_size = ext4_get_inline_size(inode);
 	buf = kmalloc(inline_size, GFP_NOFS);
@@ -1120,25 +1168,22 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 			goto out;
 	}
 
-	error = ext4_destroy_inline_data_nolock(handle, inode);
-	if (error)
-		goto out;
-
-	map.m_lblk = 0;
-	map.m_len = 1;
-	map.m_flags = 0;
-	error = ext4_map_blocks(handle, inode, &map, EXT4_GET_BLOCKS_CREATE);
+	memset(&ar, 0, sizeof(ar));
+	ar.inode = inode;
+	ar.logical = 0;
+	ar.len = 1;
+	if (S_ISREG(inode->i_mode))
+		ar.flags = EXT4_MB_HINT_DATA;
+	else
+		ar.flags = 0;
+	newblock = ext4_mb_new_blocks(handle, &ar, &error);
 	if (error < 0)
-		goto out_restore;
-	if (!(map.m_flags & EXT4_MAP_MAPPED)) {
-		error = -EIO;
-		goto out_restore;
-	}
+		goto out;
 
-	data_bh = sb_getblk(inode->i_sb, map.m_pblk);
+	data_bh = sb_getblk(inode->i_sb, newblock);
 	if (!data_bh) {
 		error = -ENOMEM;
-		goto out_restore;
+		goto out_bh;
 	}
 
 	lock_buffer(data_bh);
@@ -1147,7 +1192,7 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	if (error) {
 		unlock_buffer(data_bh);
 		error = -EIO;
-		goto out_restore;
+		goto out_bh;
 	}
 	memset(data_bh->b_data, 0, inode->i_sb->s_blocksize);
 
@@ -1171,12 +1216,14 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 			error = ext4_mark_inode_dirty(handle, inode);
 	}
 
-out_restore:
 	if (error)
-		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
-
-out:
+		goto out_bh;
+	error = ext4_set_inline_data_block(handle, inode, newblock, ar.len);
+out_bh:
+	if (error)
+		ext4_free_blocks(handle, inode, data_bh, newblock, ar.len, EXT4_FREE_BLOCKS_FORGET);
 	brelse(data_bh);
+out:
 	kfree(buf);
 	return error;
 }
-- 
2.47.3

Re: [PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Dangling extent tree reference causes double-allocation risk in the error path of block conversion.
- [High] The return value of ext4_find_extent() is passed to ext4_ext_insert_extent() without checking for ERR_PTR, leading to a kernel panic on failure.
- [High] The dynamically allocated ext4_ext_path array is leaked, and the return value of ext4_ext_insert_extent() is ignored, suppressing insertion failures.
- [High] If deleting the inline data xattr returns -ENODATA, the function prematurely returns success, skipping extent insertion and leaking the newly allocated file block.
- [High] Destructive modifications to inline data before validating extent tree insertion can lead to unrecoverable data loss.
- [Medium] The commit message claims to modify ext4_readdir, but no such changes are present in the patch.
--

--- Patch [1]: [PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
commit e51d3b5fd4decc28517230e5645f25ee40f9b2fb
Author: shuo chen <1289151713@qq.com>
Subject: Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

The commit rewrites ext4_convert_inline_data_nolock() to allocate blocks
before destroying inline data, aiming for a safer conversion.

Link: https://lore.kernel.org/linux-ext4/tencent_7A0B62235E38C5907A080BE3D56A59C9FD06@qq.com/T/#t：

> Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Severity: Medium]
The commit title mentions adding a check in ext4_readdir(), but the patch
only modifies ext4_convert_inline_data_nolock() in fs/ext4/inline.c. Are
there missing changes, or should the commit message be updated to reflect
the actual modifications?

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..1fad2a4454ecc 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> +static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode,
> +				      ext4_fsblk_t block, unsigned int len)
> +{
[ ... ]
> +	error = ext4_journal_get_write_access(handle, inode->i_sb, is.iloc.bh, EXT4_JTR_NONE);
> +	if (error)
> +		goto out;
> +	error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> +	if (error)
> +		goto out;

[Severity: High]
If the file's inline data fits entirely in i_block, the system data xattr
will not exist, and ext4_xattr_ibody_set() will return -ENODATA. This jumps
to the out label where the error is cleared to 0. Does this cause the extent
insertion to be completely skipped?

If ext4_convert_inline_data_nolock() receives a success code without the
extent being inserted, could it fail to free the newly allocated block and
leak it permanently?

> +	memset((void *)ext4_raw_inode(&is.iloc)->i_block,
> +		0, EXT4_MIN_INLINE_DATA_SIZE);
> +	memset(ei->i_data, 0, EXT4_MIN_INLINE_DATA_SIZE);

[Severity: High]
This irreversibly destroys the existing inline data before we attempt to
insert the new extent into the tree. If ext4_ext_insert_extent() fails, we
return an error and the caller frees the newly allocated block. Doesn't this
mean both the old inline data and the new block are lost, causing
unrecoverable data loss?

> +
> +	if (ext4_has_feature_extents(inode->i_sb) &&
> +		(S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode))) {
> +		ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
> +		ext4_ext_tree_init(handle, inode);
> +		struct ext4_ext_path *path = ext4_find_extent(inode, 0, NULL, 0);

[Severity: High]
Can ext4_find_extent() return an error pointer like ERR_PTR(-ENOMEM)?

Since the returned path is not checked with IS_ERR(), passing it directly to
ext4_ext_insert_extent() could result in a fatal NULL or ERR pointer
dereference.

> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(len);
> +		ext4_ext_store_pblock(&newex, block);
> +		ext4_ext_insert_extent(handle, inode, path, &newex, 0);

[Severity: High]
It looks like ext4_find_extent() dynamically allocates the path array, but it
is never freed here with ext4_free_ext_path(). Does this leak memory on every
execution?

Additionally, ext4_ext_insert_extent() can fail and return an ERR_PTR or
reallocate the path. By ignoring its return value, we might suppress errors
like -ENOSPC and fail to capture the updated path pointer, leaving the inode
corrupted without its data mapping.

[ ... ]
> @@ -1180,12 +1225,14 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
>  			error = ext4_mark_inode_dirty(handle, inode);
>  	}
>  
> -out_restore:
>  	if (error)
> -		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
> -
> -out:
> +		goto out_bh;
> +	error = ext4_set_inline_data_block(handle, inode, newblock, ar.len);
> +out_bh:
> +	if (error)
> +		ext4_free_blocks(handle, inode, data_bh, newblock, ar.len, EXT4_FREE_BLOCKS_FORGET);

[Severity: Critical]
If ext4_set_inline_data_block() successfully inserts the block into the
extent tree but fails later (for example, in ext4_mark_iloc_dirty()), it
returns an error.

The caller will then execute this path and call ext4_free_blocks() on the
newly allocated block.

Since the insertion into the extent tree is not rolled back, the block
remains mapped in the inode's extent tree but is marked as free in the
filesystem's block bitmap. Could this lead to a double-allocation
vulnerability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_BDF7C28D876E422418FB57474B765F0C1A09@qq.com?part=1

[syzbot ci] Re: Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[syzbot ci]

syzbot ci has tested the following series

[v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
https://lore.kernel.org/all/tencent_BDF7C28D876E422418FB57474B765F0C1A09@qq.com
* [PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

and found the following issue:
WARNING in invalidate_bh_lru

Full report is available here:
https://ci.syzbot.org/series/74cf3860-255d-49bb-b3df-51f8ee44a69f

***

WARNING in invalidate_bh_lru

tree:      linux-next
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next
base:      1351c159c59b04195647917c5a5f0e5467f44bb0
arch:      amd64
compiler:  Debian clang version 22.1.8 (++20260613092233+e80beda6e255-1~exp1~20260613092250.77), Debian LLD 22.1.8
config:    https://ci.syzbot.org/builds/500e2ced-dbe6-480e-affe-813e984ba307/config
syz repro: https://ci.syzbot.org/findings/875c08a7-1950-4014-851b-0d660dfa2413/syz_repro

------------[ cut here ]------------
VFS: brelse: Trying to free free buffer
WARNING: fs/buffer.c:1147 at __brelse fs/buffer.c:1147 [inline], CPU#0: udevd/5048
WARNING: fs/buffer.c:1147 at brelse include/linux/buffer_head.h:326 [inline], CPU#0: udevd/5048
WARNING: fs/buffer.c:1147 at __invalidate_bh_lrus fs/buffer.c:1506 [inline], CPU#0: udevd/5048
WARNING: fs/buffer.c:1147 at invalidate_bh_lru+0xfa/0x1b0 fs/buffer.c:1519, CPU#0: udevd/5048
Modules linked in:
CPU: 0 UID: 0 PID: 5048 Comm: udevd Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:__brelse fs/buffer.c:1147 [inline]
RIP: 0010:brelse include/linux/buffer_head.h:326 [inline]
RIP: 0010:__invalidate_bh_lrus fs/buffer.c:1506 [inline]
RIP: 0010:invalidate_bh_lru+0xfa/0x1b0 fs/buffer.c:1519
Code: f7 be 04 00 00 00 e8 85 c0 d8 ff f0 41 ff 0e eb 1e e8 8a 21 6b ff 80 3c 2b 00 75 20 eb 26 e8 7d 21 6b ff 48 8d 3d e6 bf 01 0e <67> 48 0f b9 3a 4c 89 fd 4f 8d 3c 2c 80 3c 2b 00 74 08 4c 89 ff e8
RSP: 0018:ffffc90000007f38 EFLAGS: 00010006
RAX: ffffffff825be013 RBX: 1ffff11024206b2d RCX: ffff888172139dc0
RDX: 0000000000010000 RSI: 0000000000000000 RDI: ffffffff905da000
RBP: 0000000000000000 R08: ffff8881078c875b R09: 1ffff11020f190eb
R10: dffffc0000000000 R11: ffffed1020f190ec R12: ffff888121035960
R13: 0000000000000008 R14: ffff8881078c8758 R15: dffffc0000000000
FS:  00007f6c32242c80(0000) GS:ffff88818d952000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc1515eb7c0 CR3: 000000016c5ac000 CR4: 00000000000006f0
Call Trace:
 <IRQ>
 csd_do_func kernel/smp.c:136 [inline]
 __flush_smp_call_function_queue+0x32c/0xa20 kernel/smp.c:580
 __sysvec_call_function_single+0x9a/0x3d0 arch/x86/kernel/smp.c:272
 instr_sysvec_call_function_single arch/x86/kernel/smp.c:267 [inline]
 sysvec_call_function_single+0x9e/0xc0 arch/x86/kernel/smp.c:267
 </IRQ>
 <TASK>
 asm_sysvec_call_function_single+0x1a/0x20 arch/x86/include/asm/idtentry.h:681
RIP: 0010:lock_is_held_type+0x106/0x150 kernel/locking/lockdep.c:5945
Code: 1a 00 00 b8 ff ff ff ff 65 0f c1 05 54 4c 9c 07 83 f8 01 75 25 9c 58 a9 00 02 00 00 75 39 41 f7 c4 00 02 00 00 74 01 fb 89 d8 <5b> 41 5c 41 5d 41 5e 41 5f 5d e9 1b 00 03 00 cc 90 0f 0b 90 48 c7
RSP: 0018:ffffc9000624f818 EFLAGS: 00000206
RAX: 0000000000000001 RBX: 0000000000000001 RCX: 0000000000000046
RDX: 0000000000000000 RSI: ffffffff8e4ae37e RDI: ffffffff8c4bbd80
RBP: 00000000ffffffff R08: ffffc9000020daa7 R09: 1ffff92000041b54
R10: dffffc0000000000 R11: fffff52000041b55 R12: 0000000000000246
R13: ffff888172139dc0 R14: ffffffff8eb59c60 R15: 0000000000000000
 __d_lookup+0x170/0x790 fs/dcache.c:2612
 lookup_fast+0x82/0x5d0 fs/namei.c:1878
 walk_component fs/namei.c:2278 [inline]
 link_path_walk+0x71f/0x1910 fs/namei.c:2656
 path_openat+0x236/0x3830 fs/namei.c:4859
 do_file_open+0x23e/0x4a0 fs/namei.c:4892
 do_sys_openat2+0x115/0x200 fs/open.c:1368
 do_sys_open fs/open.c:1374 [inline]
 __do_sys_openat fs/open.c:1390 [inline]
 __se_sys_openat fs/open.c:1385 [inline]
 __x64_sys_openat+0x138/0x170 fs/open.c:1385
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f6c3231a477
Code: 10 00 00 00 44 8b 54 24 e0 48 89 44 24 c0 48 8d 44 24 d0 48 89 44 24 c8 44 89 c2 4c 89 ce bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 76 10 48 8b 15 82 69 0d 00 f7 d8 64 89 02 48 83
RSP: 002b:00007ffe8da39558 EFLAGS: 00000287 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 000055a56911c8f0 RCX: 00007f6c3231a477
RDX: 0000000000090800 RSI: 000055a569100840 RDI: 00000000ffffff9c
RBP: 000055a569196980 R08: 0000000000090800 R09: 000055a569100840
R10: 0000000000000000 R11: 0000000000000287 R12: 000055a569100840
R13: 00000000000000ff R14: 000055a53ac761c4 R15: 0000000000000000
 </TASK>
----------------
Code disassembly (best guess):
   0:	f7 be 04 00 00 00    	idivl  0x4(%rsi)
   6:	e8 85 c0 d8 ff       	call   0xffd8c090
   b:	f0 41 ff 0e          	lock decl (%r14)
   f:	eb 1e                	jmp    0x2f
  11:	e8 8a 21 6b ff       	call   0xff6b21a0
  16:	80 3c 2b 00          	cmpb   $0x0,(%rbx,%rbp,1)
  1a:	75 20                	jne    0x3c
  1c:	eb 26                	jmp    0x44
  1e:	e8 7d 21 6b ff       	call   0xff6b21a0
  23:	48 8d 3d e6 bf 01 0e 	lea    0xe01bfe6(%rip),%rdi        # 0xe01c010
* 2a:	67 48 0f b9 3a       	ud1    (%edx),%rdi <-- trapping instruction
  2f:	4c 89 fd             	mov    %r15,%rbp
  32:	4f 8d 3c 2c          	lea    (%r12,%r13,1),%r15
  36:	80 3c 2b 00          	cmpb   $0x0,(%rbx,%rbp,1)
  3a:	74 08                	je     0x44
  3c:	4c 89 ff             	mov    %r15,%rdi
  3f:	e8                   	.byte 0xe8


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

To test a fix for this bug, please reply with `#syz test`
(on a separate line) and attach the patch to the email.

Notes:
- The patch will be applied on top of the tested series (as an
  incremental fix).
- To test a new version of the whole series, please send it directly
  to syzbot@lists.linux.dev.
- Arguments like custom git repos and branches are not supported.

Re: [PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Theodore Tso]

Given the issues that were reported by the Sashiko and Syzbot issues,
I commend that you try using the kvm-xfstests[1] test appliance or the
related gce-xfstests[2] test appliance if you have access to Google
Compute Engine

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
[2] https://thunk.org/gce-xfstests

Since you are making changes to inline_data feature, you'd use it
soemthing like this:

% cd /usr/src/linux  # replace with your kernel sources
% install-kconfig
% kbuild
% kvm-xfstests -c ext4/inline -g auto

You can also add some options to install-kconfig to enable extra
debugging, of which the most interesting are --kasan and --lockdep.  I
would recommend doing your initial testing using just the plain
install-kconfig, and then once you are getting clean run, do separate
runs with the Kernel Address Sanitizer and the runtime locking
correctness validator.

The way I do my testing using gce-xfstests is to use a command like
this: "gce-xfstests ltm -c ext4/all -g auto" which will launch 12
VM's, for a variety of ext4 test scenarios (e.g., with bigalloc, fast
commit, 1k block sizes, etc.)  You can of course try running
"kvm-xfstests -c ext4/all -g auto" but this will likely take around
24-36 hours depending on how fast your local storage test devices.
(Using gce-xfstest's lightweight test manager takes around 2 hours of
wall clock time, since the test VM's run in parallel.)

Cheers,

					- Ted

Re: [PATCH v2] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

On Sun, Aug 16, 2026 at 11:12:33PM -0400, Theodore Tso wrote:
> Since you are making changes to inline_data feature, you'd use it
> soemthing like this:
> 
> % cd /usr/src/linux  # replace with your kernel sources
> % install-kconfig
> % kbuild
> % kvm-xfstests -c ext4/inline -g auto

Thank you very much for your detailed guidance on the testing process.
I will fix the issues reported by Sashiko and Syzbot.

> 
> You can also add some options to install-kconfig to enable extra
> debugging, of which the most interesting are --kasan and --lockdep.  I
> would recommend doing your initial testing using just the plain
> install-kconfig, and then once you are getting clean run, do separate
> runs with the Kernel Address Sanitizer and the runtime locking
> correctness validator.

I'll run the tests following your recommended sequence.

> 
> The way I do my testing using gce-xfstests is to use a command like
> this: "gce-xfstests ltm -c ext4/all -g auto" which will launch 12
> VM's, for a variety of ext4 test scenarios (e.g., with bigalloc, fast
> commit, 1k block sizes, etc.)  You can of course try running
> "kvm-xfstests -c ext4/all -g auto" but this will likely take around
> 24-36 hours depending on how fast your local storage test devices.
> (Using gce-xfstest's lightweight test manager takes around 2 hours of
> wall clock time, since the test VM's run in parallel.)

Regarding gce-xfstests, I don't have Google Cloud access at the moment,
but I'll also try running "kvm-xfstests -c ext4/all -g auto" locally to
cover more scenarios.

I'll send the v3 patch along with the test results once everything is
ready.

[PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

From: pipishuo <1289151713@qq.com>

---
v2 -> v3:
-Add error recovery for ext4_find_extent() and ext4_ext_insert_extent()
-Add additional error handling code
---

Signed-off-by: pipishuo <1289151713@qq.com>
---
 fs/ext4/inline.c | 179 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 139 insertions(+), 40 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 8045e4ff270c..507106c72e82 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -14,6 +14,7 @@
 #include "ext4.h"
 #include "xattr.h"
 #include "truncate.h"
+#include "ext4_extents.h"
 
 #define EXT4_XATTR_SYSTEM_DATA	"data"
 #define EXT4_MIN_INLINE_DATA_SIZE	((sizeof(__le32) * EXT4_N_BLOCKS))
@@ -1070,21 +1071,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
 	return 0;
 }
 
-static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
-				     struct ext4_iloc *iloc,
-				     void *buf, int inline_size)
+static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode, ext4_fsblk_t block,
+					unsigned int len, struct buffer_head *bh)
 {
-	int ret;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct ext4_xattr_ibody_find is = {
+		.s = { .not_found = 0, },
+	};
+	struct ext4_xattr_info i = {
+		.name_index = EXT4_XATTR_INDEX_SYSTEM,
+		.name = EXT4_XATTR_SYSTEM_DATA,
+		.value = NULL,
+		.value_len = 0,
+	};
+	int error;
+	int inode_size;
+	void *inode_buf = NULL;
+	struct ext4_inode *raw_inode;
 
-	ret = ext4_create_inline_data(handle, inode, inline_size);
-	if (ret) {
-		ext4_msg(inode->i_sb, KERN_EMERG,
-			"error restoring inline_data for inode -- potential data loss! (inode %llu, error %d)",
-			inode->i_ino, ret);
-		return;
+	inode_size = EXT4_INODE_SIZE(inode->i_sb);
+	inode_buf = kmalloc(inode_size, GFP_NOFS);
+	if (!inode_buf) {
+		error = -ENOMEM;
+		down_write(&ei->i_data_sem);
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		up_write(&ei->i_data_sem);
+		return error;
 	}
-	ext4_write_inline_data(inode, iloc, buf, 0, inline_size);
-	ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+	down_write(&ei->i_data_sem);
+	error = ext4_get_inode_loc(inode, &is.iloc);
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		up_write(&ei->i_data_sem);
+		kfree(inode_buf);
+		return error;
+	}
+	raw_inode = ext4_raw_inode(&is.iloc);
+	BUFFER_TRACE(is.iloc.bh, "get_write_access");
+	error = ext4_journal_get_write_access(handle, inode->i_sb, is.iloc.bh, EXT4_JTR_NONE);
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		goto out;
+	}
+	memcpy(inode_buf, (void *)raw_inode, inode_size);
+	memset((void *)raw_inode->i_block, 0, EXT4_MIN_INLINE_DATA_SIZE);
+	memset(ei->i_data, 0, EXT4_MIN_INLINE_DATA_SIZE);
+	if (ext4_has_feature_extents(inode->i_sb) &&
+		(S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode))) {
+		ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
+		ext4_ext_tree_init(handle, inode);
+		struct ext4_ext_path *path = ext4_find_extent(inode, 0, NULL, 0);
+
+		if (IS_ERR(path)) {
+			error = PTR_ERR(path);
+			goto recovery;
+		}
+		struct ext4_extent newex;
+
+		newex.ee_block = cpu_to_le32(0);
+		newex.ee_len = cpu_to_le16(1);
+		ext4_ext_store_pblock(&newex, block);
+		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);
+		if (IS_ERR(path)) {
+			error = PTR_ERR(path);
+			if (error == -EDQUOT || error == -ENOSPC) {
+				goto recovery;
+			} else {
+				ext4_forget(handle, 0, inode, bh, block);
+				goto nofree;
+			}
+		} else {
+			ext4_free_ext_path(path);
+		}
+	} else {
+		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
+	}
+	error = ext4_xattr_ibody_find(inode, &i, &is);
+	if (error)
+		goto recovery;
+	if (!is.s.not_found)
+		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
+recovery:
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+nofree:
+		memcpy((void *)raw_inode, inode_buf, inode_size);
+		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
+		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
+	} else {
+		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
+		get_bh(is.iloc.bh);
+		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
+		EXT4_I(inode)->i_inline_off = 0;
+		EXT4_I(inode)->i_inline_size = 0;
+		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+		if (S_ISDIR(inode->i_mode)) {
+			i_size_write(inode, inode->i_sb->s_blocksize);
+			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
+		}
+		brelse(bh);
+	}
+out:
+	brelse(is.iloc.bh);
+	up_write(&ei->i_data_sem);
+	kfree(inode_buf);
+	return error;
 }
 
 static int ext4_convert_inline_data_nolock(handle_t *handle,
@@ -1094,8 +1189,11 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	int error;
 	void *buf = NULL;
 	struct buffer_head *data_bh = NULL;
-	struct ext4_map_blocks map;
 	int inline_size;
+	ext4_fsblk_t newblock = 0;
+	struct ext4_allocation_request ar;
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	unsigned int allocated_block;
 
 	inline_size = ext4_get_inline_size(inode);
 	buf = kmalloc(inline_size, GFP_NOFS);
@@ -1120,25 +1218,22 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 			goto out;
 	}
 
-	error = ext4_destroy_inline_data_nolock(handle, inode);
-	if (error)
-		goto out;
-
-	map.m_lblk = 0;
-	map.m_len = 1;
-	map.m_flags = 0;
-	error = ext4_map_blocks(handle, inode, &map, EXT4_GET_BLOCKS_CREATE);
+	memset(&ar, 0, sizeof(ar));
+	ar.inode = inode;
+	ar.logical = 0;
+	ar.len = 1;
+	if (S_ISREG(inode->i_mode))
+		ar.flags = EXT4_MB_HINT_DATA;
+	else
+		ar.flags = 0;
+	newblock = ext4_mb_new_blocks(handle, &ar, &error);
 	if (error < 0)
-		goto out_restore;
-	if (!(map.m_flags & EXT4_MAP_MAPPED)) {
-		error = -EIO;
-		goto out_restore;
-	}
-
-	data_bh = sb_getblk(inode->i_sb, map.m_pblk);
+		goto out;
+	allocated_block = EXT4_C2B(sbi, ar.len);
+	data_bh = sb_getblk(inode->i_sb, newblock);
 	if (!data_bh) {
 		error = -ENOMEM;
-		goto out_restore;
+		goto out_bh;
 	}
 
 	lock_buffer(data_bh);
@@ -1147,7 +1242,7 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	if (error) {
 		unlock_buffer(data_bh);
 		error = -EIO;
-		goto out_restore;
+		goto out_bh;
 	}
 	memset(data_bh->b_data, 0, inode->i_sb->s_blocksize);
 
@@ -1159,24 +1254,28 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 						   inode, data_bh);
 	} else {
 		unlock_buffer(data_bh);
-		inode->i_size = inode->i_sb->s_blocksize;
-		i_size_write(inode, inode->i_sb->s_blocksize);
-		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
-
 		error = ext4_init_dirblock(handle, inode, data_bh,
 			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
 			  buf + EXT4_INLINE_DOTDOT_SIZE,
 			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
-		if (!error)
-			error = ext4_mark_inode_dirty(handle, inode);
 	}
+out_bh:
+	if (error) {
+		int flags = 0;
 
-out_restore:
-	if (error)
-		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
+		if (data_bh)
+			flags |= EXT4_FREE_BLOCKS_FORGET;
+		struct ext4_inode_info *ei = EXT4_I(inode);
 
+		down_write(&ei->i_data_sem);
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);
+		up_write(&ei->i_data_sem);
+	} else {
+		error = ext4_set_inline_data_block(handle, inode,
+				newblock, allocated_block, data_bh);
+	}
 out:
-	brelse(data_bh);
 	kfree(buf);
 	return error;
 }
-- 
2.47.3

Re: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Data blocks are permanently leaked in the block bitmap when `ext4_ext_insert_extent` fails with an error other than EDQUOT or ENOSPC.
- [High] The extent insertion hardcodes a length of 1, breaking bigalloc support and leaking blocks if the cluster size is greater than the block size.
- [High] Error recovery path triggers a BUG_ON(count > 1) in ext4_free_blocks for regular files or bigalloc clusters.
- [High] Raw inode memory is reverted on error without marking the buffer dirty, leading to an inconsistent journal state.
- [High] The on-disk inode's size is not updated when converting an inline directory, leaving the directory with a truncated size on disk.
- [Medium] The patch implementation completely contradicts the commit message.
--

--- Patch [1]: [PATCH] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
commit 60b81af153ab511900118d63a628e53fb1229564
Author: pipishuo <1289151713@qq.com>
Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

This commit refactors inline data conversion and adds error recovery for
extent insertion during block allocation.

> Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[Severity: Medium]
The commit message claims to add a check in ext4_readdir, but the diff
completely ignores ext4_readdir and instead refactors inline data
conversion in ext4_set_inline_data_block. Can the commit message be
updated to reflect the actual changes?

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482b..259a6af13b0f6 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
[ ... ]
> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(1);

[Severity: High]
Does hardcoding the extent length to 1 break bigalloc support? If the
cluster size is greater than the block size, or if multiple blocks are
allocated, the len parameter is ignored. Will the additional allocated
blocks marked in the bitmap be permanently leaked since they aren't
added to the extent tree?

> +		ext4_ext_store_pblock(&newex, block);
> +		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);
> +		if (IS_ERR(path)) {
> +			error = PTR_ERR(path);
> +			if (error == -EDQUOT || error == -ENOSPC) {
> +				goto recovery;
> +			} else {
> +				ext4_forget(handle, 0, inode, bh, block);
> +				goto nofree;

[Severity: High]
If ext4_ext_insert_extent() fails with an error other than EDQUOT or
ENOSPC, the code jumps to nofree. Does this permanently leak the
allocated data blocks in the block bitmap, since the ext4_free_blocks()
call in the recovery block is bypassed?

> +			}
> +		} else {
> +			ext4_free_ext_path(path);
> +		}
[ ... ]
> +recovery:
> +	if (error) {
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
> +nofree:
> +		memcpy((void *)raw_inode, inode_buf, inode_size);
> +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);

[Severity: High]
This reverts the raw_inode memory on error, but is the buffer ever
marked dirty again? Because ext4_ext_tree_init() previously marked the
buffer dirty in the active transaction, doesn't overwriting the memory
here without calling ext4_mark_iloc_dirty() lead to an inconsistent
journal state?

> +	} else {
> +		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> +		get_bh(is.iloc.bh);
> +		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> +		EXT4_I(inode)->i_inline_off = 0;
> +		EXT4_I(inode)->i_inline_size = 0;
> +		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> +		if (S_ISDIR(inode->i_mode)) {
> +			i_size_write(inode, inode->i_sb->s_blocksize);
> +			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> +		}

[Severity: High]
Because ext4_mark_iloc_dirty() is called before the S_ISDIR size
updates, does this cause the new size to be missed in the buffer update?
Will the on-disk inode retain the old truncated inline data size?

> +		brelse(bh);
> +	}
[ ... ]
> @@ -1103,8 +1198,11 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
[ ... ]
> +out_bh:
> +	if (error) {
> +		int flags = 0;
>  
> -out_restore:
> -	if (error)
> -		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
> +		if (data_bh)
> +			flags |= EXT4_FREE_BLOCKS_FORGET;
> +		struct ext4_inode_info *ei = EXT4_I(inode);
>  
> +		down_write(&ei->i_data_sem);
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);

[Severity: High]
Does this trigger a BUG_ON in ext4_free_blocks() on error paths? If we
allocate multiple blocks (e.g., when bigalloc is enabled or
EXT4_MB_HINT_DATA is used), allocated_block can be > 1.

Calling ext4_free_blocks() with a non-NULL data_bh, a count > 1, and the
EXT4_FREE_BLOCKS_FORGET flag will hit the BUG_ON(count > 1) in
ext4_free_blocks().

> +		up_write(&ei->i_data_sem);
> +	} else {
> +		error = ext4_set_inline_data_block(handle, inode,
> +				newblock, allocated_block, data_bh);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_7FDF0CFD84843032AE9A95DCBD443746860A@qq.com?part=1

[PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

From: pipishuo <1289151713@qq.com>

Resend as v3 because I forgot to add the version number
---
v2 -> v3:
-Add error recovery for ext4_find_extent() and ext4_ext_insert_extent()
-Add additional error handling code
---

Signed-off-by: pipishuo <1289151713@qq.com>
---
 fs/ext4/inline.c | 179 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 139 insertions(+), 40 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 8045e4ff270c..507106c72e82 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -14,6 +14,7 @@
 #include "ext4.h"
 #include "xattr.h"
 #include "truncate.h"
+#include "ext4_extents.h"
 
 #define EXT4_XATTR_SYSTEM_DATA	"data"
 #define EXT4_MIN_INLINE_DATA_SIZE	((sizeof(__le32) * EXT4_N_BLOCKS))
@@ -1070,21 +1071,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
 	return 0;
 }
 
-static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
-				     struct ext4_iloc *iloc,
-				     void *buf, int inline_size)
+static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode, ext4_fsblk_t block,
+					unsigned int len, struct buffer_head *bh)
 {
-	int ret;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct ext4_xattr_ibody_find is = {
+		.s = { .not_found = 0, },
+	};
+	struct ext4_xattr_info i = {
+		.name_index = EXT4_XATTR_INDEX_SYSTEM,
+		.name = EXT4_XATTR_SYSTEM_DATA,
+		.value = NULL,
+		.value_len = 0,
+	};
+	int error;
+	int inode_size;
+	void *inode_buf = NULL;
+	struct ext4_inode *raw_inode;
 
-	ret = ext4_create_inline_data(handle, inode, inline_size);
-	if (ret) {
-		ext4_msg(inode->i_sb, KERN_EMERG,
-			"error restoring inline_data for inode -- potential data loss! (inode %llu, error %d)",
-			inode->i_ino, ret);
-		return;
+	inode_size = EXT4_INODE_SIZE(inode->i_sb);
+	inode_buf = kmalloc(inode_size, GFP_NOFS);
+	if (!inode_buf) {
+		error = -ENOMEM;
+		down_write(&ei->i_data_sem);
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		up_write(&ei->i_data_sem);
+		return error;
 	}
-	ext4_write_inline_data(inode, iloc, buf, 0, inline_size);
-	ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+	down_write(&ei->i_data_sem);
+	error = ext4_get_inode_loc(inode, &is.iloc);
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		up_write(&ei->i_data_sem);
+		kfree(inode_buf);
+		return error;
+	}
+	raw_inode = ext4_raw_inode(&is.iloc);
+	BUFFER_TRACE(is.iloc.bh, "get_write_access");
+	error = ext4_journal_get_write_access(handle, inode->i_sb, is.iloc.bh, EXT4_JTR_NONE);
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+		goto out;
+	}
+	memcpy(inode_buf, (void *)raw_inode, inode_size);
+	memset((void *)raw_inode->i_block, 0, EXT4_MIN_INLINE_DATA_SIZE);
+	memset(ei->i_data, 0, EXT4_MIN_INLINE_DATA_SIZE);
+	if (ext4_has_feature_extents(inode->i_sb) &&
+		(S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode))) {
+		ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
+		ext4_ext_tree_init(handle, inode);
+		struct ext4_ext_path *path = ext4_find_extent(inode, 0, NULL, 0);
+
+		if (IS_ERR(path)) {
+			error = PTR_ERR(path);
+			goto recovery;
+		}
+		struct ext4_extent newex;
+
+		newex.ee_block = cpu_to_le32(0);
+		newex.ee_len = cpu_to_le16(1);
+		ext4_ext_store_pblock(&newex, block);
+		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);
+		if (IS_ERR(path)) {
+			error = PTR_ERR(path);
+			if (error == -EDQUOT || error == -ENOSPC) {
+				goto recovery;
+			} else {
+				ext4_forget(handle, 0, inode, bh, block);
+				goto nofree;
+			}
+		} else {
+			ext4_free_ext_path(path);
+		}
+	} else {
+		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
+	}
+	error = ext4_xattr_ibody_find(inode, &i, &is);
+	if (error)
+		goto recovery;
+	if (!is.s.not_found)
+		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
+recovery:
+	if (error) {
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
+nofree:
+		memcpy((void *)raw_inode, inode_buf, inode_size);
+		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
+		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
+	} else {
+		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
+		get_bh(is.iloc.bh);
+		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
+		EXT4_I(inode)->i_inline_off = 0;
+		EXT4_I(inode)->i_inline_size = 0;
+		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+		if (S_ISDIR(inode->i_mode)) {
+			i_size_write(inode, inode->i_sb->s_blocksize);
+			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
+		}
+		brelse(bh);
+	}
+out:
+	brelse(is.iloc.bh);
+	up_write(&ei->i_data_sem);
+	kfree(inode_buf);
+	return error;
 }
 
 static int ext4_convert_inline_data_nolock(handle_t *handle,
@@ -1094,8 +1189,11 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	int error;
 	void *buf = NULL;
 	struct buffer_head *data_bh = NULL;
-	struct ext4_map_blocks map;
 	int inline_size;
+	ext4_fsblk_t newblock = 0;
+	struct ext4_allocation_request ar;
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	unsigned int allocated_block;
 
 	inline_size = ext4_get_inline_size(inode);
 	buf = kmalloc(inline_size, GFP_NOFS);
@@ -1120,25 +1218,22 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 			goto out;
 	}
 
-	error = ext4_destroy_inline_data_nolock(handle, inode);
-	if (error)
-		goto out;
-
-	map.m_lblk = 0;
-	map.m_len = 1;
-	map.m_flags = 0;
-	error = ext4_map_blocks(handle, inode, &map, EXT4_GET_BLOCKS_CREATE);
+	memset(&ar, 0, sizeof(ar));
+	ar.inode = inode;
+	ar.logical = 0;
+	ar.len = 1;
+	if (S_ISREG(inode->i_mode))
+		ar.flags = EXT4_MB_HINT_DATA;
+	else
+		ar.flags = 0;
+	newblock = ext4_mb_new_blocks(handle, &ar, &error);
 	if (error < 0)
-		goto out_restore;
-	if (!(map.m_flags & EXT4_MAP_MAPPED)) {
-		error = -EIO;
-		goto out_restore;
-	}
-
-	data_bh = sb_getblk(inode->i_sb, map.m_pblk);
+		goto out;
+	allocated_block = EXT4_C2B(sbi, ar.len);
+	data_bh = sb_getblk(inode->i_sb, newblock);
 	if (!data_bh) {
 		error = -ENOMEM;
-		goto out_restore;
+		goto out_bh;
 	}
 
 	lock_buffer(data_bh);
@@ -1147,7 +1242,7 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 	if (error) {
 		unlock_buffer(data_bh);
 		error = -EIO;
-		goto out_restore;
+		goto out_bh;
 	}
 	memset(data_bh->b_data, 0, inode->i_sb->s_blocksize);
 
@@ -1159,24 +1254,28 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 						   inode, data_bh);
 	} else {
 		unlock_buffer(data_bh);
-		inode->i_size = inode->i_sb->s_blocksize;
-		i_size_write(inode, inode->i_sb->s_blocksize);
-		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
-
 		error = ext4_init_dirblock(handle, inode, data_bh,
 			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
 			  buf + EXT4_INLINE_DOTDOT_SIZE,
 			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
-		if (!error)
-			error = ext4_mark_inode_dirty(handle, inode);
 	}
+out_bh:
+	if (error) {
+		int flags = 0;
 
-out_restore:
-	if (error)
-		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
+		if (data_bh)
+			flags |= EXT4_FREE_BLOCKS_FORGET;
+		struct ext4_inode_info *ei = EXT4_I(inode);
 
+		down_write(&ei->i_data_sem);
+		ext4_discard_preallocations(inode);
+		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);
+		up_write(&ei->i_data_sem);
+	} else {
+		error = ext4_set_inline_data_block(handle, inode,
+				newblock, allocated_block, data_bh);
+	}
 out:
-	brelse(data_bh);
 	kfree(buf);
 	return error;
 }
-- 
2.47.3

Re: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Misordered metadata update causes corrupt directory `i_size` on disk.
- [Critical] Unconditional BUG_ON() crash in `ext4_free_blocks()` on error paths for allocations > 1 block.
- [High] Permanent block leak when `ext4_ext_insert_extent` fails with an error other than EDQUOT or ENOSPC.
- [High] Concurrent inode metadata changes are overwritten during error recovery.
- [High] Permanent space leak due to hardcoded extent length of 1 block.
- [High] Missing `i_blocks` accounting for newly allocated physical blocks.
- [Medium] The commit message describes a completely different change.
--

--- Patch [1]: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
commit 1cb52ec17c1b86718dd26a2d224b02e260c85e40
Author: pipishuo <1289151713@qq.com>
Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

The commit message describes adding a state check in ext4_readdir, but the
patch refactors ext4_convert_inline_data_nolock and introduces a new block
allocation mechanism for inline data conversion.

>     Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
>     
>     Resend as v3 because I forgot to add the version number

[Severity: Medium]
Is the commit message mismatched with the actual code changes? The subject
and message describe adding a state check in ext4_readdir, but the patch
entirely refactors ext4_convert_inline_data_nolock and introduces a new
block allocation mechanism for inline data conversion without mentioning it.

> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
[ ... ]
> @@ -1079,21 +1080,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
>  	return 0;
>  }
>  
> -static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
> -				     struct ext4_iloc *iloc,
> -				     void *buf, int inline_size)
> +static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode, ext4_fsblk_t block,
> +					unsigned int len, struct buffer_head *bh)
>  {
[ ... ]
> +		struct ext4_extent newex;
> +
> +		newex.ee_block = cpu_to_le32(0);
> +		newex.ee_len = cpu_to_le16(1);

[Severity: High]
Does this permanently leak physical blocks if the allocator returns more
than 1 block? When ext4_mb_new_blocks() returns a larger allocation
(e.g., due to cluster normalization on bigalloc filesystems), hardcoding
the extent length to 1 abandons the extra blocks. Should this use the
allocated length (len parameter) instead?

> +		ext4_ext_store_pblock(&newex, block);
> +		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);

[Severity: High]
Does this miss accounting for the newly allocated physical blocks?
Manually inserting the extent bypasses the normal ext4_map_blocks()
pipeline, meaning ext4_inode_add_physical_space() is never called to
increment inode->i_blocks. Could this lead to stat() inconsistencies and
fsck failures?

> +		if (IS_ERR(path)) {
> +			error = PTR_ERR(path);
> +			if (error == -EDQUOT || error == -ENOSPC) {
> +				goto recovery;
> +			} else {
> +				ext4_forget(handle, 0, inode, bh, block);
> +				goto nofree;
> +			}
> +		} else {
> +			ext4_free_ext_path(path);
> +		}
> +	} else {
> +		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
> +	}
> +	error = ext4_xattr_ibody_find(inode, &i, &is);
> +	if (error)
> +		goto recovery;
> +	if (!is.s.not_found)
> +		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> +recovery:
> +	if (error) {
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);

[Severity: Critical]
Can this trigger a BUG_ON() crash in ext4_free_blocks() on bigalloc
filesystems? If the length (len) is greater than 1, calling
ext4_free_blocks() with both a valid buffer_head (bh) and the
EXT4_FREE_BLOCKS_FORGET flag will unconditionally hit the
BUG_ON(count > 1) check inside ext4_free_blocks().

> +nofree:

[Severity: High]
Does the jump to the nofree label earlier permanently leak the allocated
blocks if the extent insertion fails with an error other than -EDQUOT or
-ENOSPC? Jumping here bypasses the ext4_free_blocks() call during error
recovery. Because ext4_forget() only removes the buffer from the journal
without freeing the physical blocks in the buddy bitmap, the blocks appear
to be permanently leaked.

> +		memcpy((void *)raw_inode, inode_buf, inode_size);

[Severity: High]
Could this non-atomic memcpy() overwrite concurrent metadata updates?
The i_data_sem protects the block mapping, but background flushers or
other operations might have updated the inode's timestamps or extended
attributes in the raw_inode while we were allocating blocks. Blindly
copying back the old inode_buf could silently discard those changes.

> +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> +	} else {
> +		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> +		get_bh(is.iloc.bh);
> +		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> +		EXT4_I(inode)->i_inline_off = 0;
> +		EXT4_I(inode)->i_inline_size = 0;
> +		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> +		if (S_ISDIR(inode->i_mode)) {
> +			i_size_write(inode, inode->i_sb->s_blocksize);
> +			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> +		}

[Severity: Critical]
Does this permanently save a stale directory size to disk?
The raw metadata buffer is marked dirty via ext4_mark_iloc_dirty()
before the VFS inode size and i_disksize are updated. Since this patch
removed the later ext4_mark_inode_dirty() call, is there a risk the
updated size fields never make it to the on-disk raw_inode?

> +		brelse(bh);
> +	}
[ ... ]
> @@ -1168,24 +1263,28 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
>  						   inode, data_bh);
>  	} else {
>  		unlock_buffer(data_bh);
> -		inode->i_size = inode->i_sb->s_blocksize;
> -		i_size_write(inode, inode->i_sb->s_blocksize);
> -		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> -
>  		error = ext4_init_dirblock(handle, inode, data_bh,
>  			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
>  			  buf + EXT4_INLINE_DOTDOT_SIZE,
>  			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
> -		if (!error)
> -			error = ext4_mark_inode_dirty(handle, inode);
>  	}
> +out_bh:
> +	if (error) {
> +		int flags = 0;
>  
> -out_restore:
> -	if (error)
> -		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
> +		if (data_bh)
> +			flags |= EXT4_FREE_BLOCKS_FORGET;
> +		struct ext4_inode_info *ei = EXT4_I(inode);
>  
> +		down_write(&ei->i_data_sem);
> +		ext4_discard_preallocations(inode);
> +		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);

[Severity: Critical]
Similarly, could this also trigger the BUG_ON(count > 1) in
ext4_free_blocks() on failure? If allocated_block is greater than 1
(e.g., bigalloc) and data_bh is valid, flags will include
EXT4_FREE_BLOCKS_FORGET, which crashes when freeing multiple blocks
with a provided buffer_head.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_5FCFA1D9100C4691D5AA92A597B846A8F007@qq.com?part=1

Re: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen]

On Sat, Aug 22, 2026 at 04:45:43PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [Critical] Misordered metadata update causes corrupt directory `i_size` on disk.
> - [Critical] Unconditional BUG_ON() crash in `ext4_free_blocks()` on error paths for allocations > 1 block.
> - [High] Permanent block leak when `ext4_ext_insert_extent` fails with an error other than EDQUOT or ENOSPC.
> - [High] Concurrent inode metadata changes are overwritten during error recovery.
> - [High] Permanent space leak due to hardcoded extent length of 1 block.
> - [High] Missing `i_blocks` accounting for newly allocated physical blocks.
> - [Medium] The commit message describes a completely different change.
> --
> 
> --- Patch [1]: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir ---
> commit 1cb52ec17c1b86718dd26a2d224b02e260c85e40
> Author: pipishuo <1289151713@qq.com>
> Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
> 
> The commit message describes adding a state check in ext4_readdir, but the
> patch refactors ext4_convert_inline_data_nolock and introduces a new block
> allocation mechanism for inline data conversion.
> 
> >     Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir
> >     
> >     Resend as v3 because I forgot to add the version number
> 
> [Severity: Medium]
> Is the commit message mismatched with the actual code changes? The subject
> and message describe adding a state check in ext4_readdir, but the patch
> entirely refactors ext4_convert_inline_data_nolock and introduces a new
> block allocation mechanism for inline data conversion without mentioning it.
Got it
> 
> > --- a/fs/ext4/inline.c
> > +++ b/fs/ext4/inline.c
> [ ... ]
> > @@ -1079,21 +1080,115 @@ static int ext4_update_inline_dir(handle_t *handle, struct inode *dir,
> >  	return 0;
> >  }
> >  
> > -static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
> > -				     struct ext4_iloc *iloc,
> > -				     void *buf, int inline_size)
> > +static int ext4_set_inline_data_block(handle_t *handle, struct inode *inode, ext4_fsblk_t block,
> > +					unsigned int len, struct buffer_head *bh)
> >  {
> [ ... ]
> > +		struct ext4_extent newex;
> > +
> > +		newex.ee_block = cpu_to_le32(0);
> > +		newex.ee_len = cpu_to_le16(1);
> 
> [Severity: High]
> Does this permanently leak physical blocks if the allocator returns more
> than 1 block? When ext4_mb_new_blocks() returns a larger allocation
> (e.g., due to cluster normalization on bigalloc filesystems), hardcoding
> the extent length to 1 abandons the extra blocks. Should this use the
> allocated length (len parameter) instead?
The excess blocks will be stored in i_prealloc_node, so setting the length
to 1 is fine.
> 
> > +		ext4_ext_store_pblock(&newex, block);
> > +		path = ext4_ext_insert_extent(handle, inode, path, &newex, 0);
> 
> [Severity: High]
> Does this miss accounting for the newly allocated physical blocks?
> Manually inserting the extent bypasses the normal ext4_map_blocks()
> pipeline, meaning ext4_inode_add_physical_space() is never called to
> increment inode->i_blocks. Could this lead to stat() inconsistencies and
> fsck failures?
I don't have a deep understanding of the i_blocks update mechanism, 
but I still believe that i_blocks should not be updated here.
> 
> > +		if (IS_ERR(path)) {
> > +			error = PTR_ERR(path);
> > +			if (error == -EDQUOT || error == -ENOSPC) {
> > +				goto recovery;
> > +			} else {
> > +				ext4_forget(handle, 0, inode, bh, block);
> > +				goto nofree;
> > +			}
> > +		} else {
> > +			ext4_free_ext_path(path);
> > +		}
> > +	} else {
> > +		EXT4_I(inode)->i_data[0] = cpu_to_le32(block);
> > +	}
> > +	error = ext4_xattr_ibody_find(inode, &i, &is);
> > +	if (error)
> > +		goto recovery;
> > +	if (!is.s.not_found)
> > +		error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> > +recovery:
> > +	if (error) {
> > +		ext4_discard_preallocations(inode);
> > +		ext4_free_blocks(handle, inode, bh, block, len, EXT4_FREE_BLOCKS_FORGET);
> 
> [Severity: Critical]
> Can this trigger a BUG_ON() crash in ext4_free_blocks() on bigalloc
> filesystems? If the length (len) is greater than 1, calling
> ext4_free_blocks() with both a valid buffer_head (bh) and the
> EXT4_FREE_BLOCKS_FORGET flag will unconditionally hit the
> BUG_ON(count > 1) check inside ext4_free_blocks().
Got it
> 
> > +nofree:
> 
> [Severity: High]
> Does the jump to the nofree label earlier permanently leak the allocated
> blocks if the extent insertion fails with an error other than -EDQUOT or
> -ENOSPC? Jumping here bypasses the ext4_free_blocks() call during error
> recovery. Because ext4_forget() only removes the buffer from the journal
> without freeing the physical blocks in the buddy bitmap, the blocks appear
> to be permanently leaked.
Got it
> 
> > +		memcpy((void *)raw_inode, inode_buf, inode_size);
> 
> [Severity: High]
> Could this non-atomic memcpy() overwrite concurrent metadata updates?
> The i_data_sem protects the block mapping, but background flushers or
> other operations might have updated the inode's timestamps or extended
> attributes in the raw_inode while we were allocating blocks. Blindly
> copying back the old inode_buf could silently discard those changes.
Got it
> 
> > +		memcpy(ei->i_data, raw_inode->i_block, EXT4_MIN_INLINE_DATA_SIZE);
> > +		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> > +	} else {
> > +		ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> > +		get_bh(is.iloc.bh);
> > +		error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> > +		EXT4_I(inode)->i_inline_off = 0;
> > +		EXT4_I(inode)->i_inline_size = 0;
> > +		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> > +		if (S_ISDIR(inode->i_mode)) {
> > +			i_size_write(inode, inode->i_sb->s_blocksize);
> > +			EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> > +		}
> 
> [Severity: Critical]
> Does this permanently save a stale directory size to disk?
> The raw metadata buffer is marked dirty via ext4_mark_iloc_dirty()
> before the VFS inode size and i_disksize are updated. Since this patch
> removed the later ext4_mark_inode_dirty() call, is there a risk the
> updated size fields never make it to the on-disk raw_inode?
Got it
> 
> > +		brelse(bh);
> > +	}
> [ ... ]
> > @@ -1168,24 +1263,28 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
> >  						   inode, data_bh);
> >  	} else {
> >  		unlock_buffer(data_bh);
> > -		inode->i_size = inode->i_sb->s_blocksize;
> > -		i_size_write(inode, inode->i_sb->s_blocksize);
> > -		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
> > -
> >  		error = ext4_init_dirblock(handle, inode, data_bh,
> >  			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
> >  			  buf + EXT4_INLINE_DOTDOT_SIZE,
> >  			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
> > -		if (!error)
> > -			error = ext4_mark_inode_dirty(handle, inode);
> >  	}
> > +out_bh:
> > +	if (error) {
> > +		int flags = 0;
> >  
> > -out_restore:
> > -	if (error)
> > -		ext4_restore_inline_data(handle, inode, iloc, buf, inline_size);
> > +		if (data_bh)
> > +			flags |= EXT4_FREE_BLOCKS_FORGET;
> > +		struct ext4_inode_info *ei = EXT4_I(inode);
> >  
> > +		down_write(&ei->i_data_sem);
> > +		ext4_discard_preallocations(inode);
> > +		ext4_free_blocks(handle, inode, data_bh, newblock, allocated_block, flags);
> 
> [Severity: Critical]
> Similarly, could this also trigger the BUG_ON(count > 1) in
> ext4_free_blocks() on failure? If allocated_block is greater than 1
> (e.g., bigalloc) and data_bh is valid, flags will include
> EXT4_FREE_BLOCKS_FORGET, which crashes when freeing multiple blocks
> with a provided buffer_head.
Got it
I'll fix these issues and send V4.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/tencent_5FCFA1D9100C4691D5AA92A597B846A8F007@qq.com?part=1