Re: [PATCH V3] Add EXT4_STATE_MAY_INLINE_DATA check in ext4_readdir

[shuo chen <1289151713@qq.com>]

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