Re: [PATCH v14 07/12] ext4: add ext4_dir_entry_len()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ext4: inconsistent dx_get_dx_info() offset in make_indexed_dir()
- [Low] ext4: commit message mismatch with dx_root_limit() and get_dtype()
--

commit e8626c9249ed17bdd04a7586fe5be141bfa96520
Author: Artem Blagodarenko <artem.blagodarenko@gmail.com>

ext4: add ext4_dir_entry_len()

This commit updates dx_root_limit() to calculate the root htree entry limit
using dx_get_dx_info() and masks filetype with EXT4_FT_MASK in get_dtype().

>     ext4: add ext4_dir_entry_len()
>     
>     ext4_dir_entry_len() (added in the previous patch, where
>     dx_get_dx_info() first needs it) computes the required rec_len for
>     a directory entry, taking into account dirdata and casefold+fscrypt
>     hash space.
>     
>     Update dx_root_limit() to use ext4_dir_entry_len() instead of
>     open-coded ext4_dir_rec_len() for '.' and '..' entries.
>     
>     Signed-off-by: Artem Blagodarenko <artem.blagodarenko@gmail.com>
>     Assisted-by: Claude:claude-opus-5-5

[Severity: Low]
This isn't a bug, but does the commit title and message match the changes in
this patch?

As the commit message notes, ext4_dir_entry_len() was already added in the
previous commit (73068d5241d8, "ext4: refactor dx_root to support variable
dirent sizes").

In addition, dx_root_limit() is updated here to call dx_get_dx_info() rather
than ext4_dir_entry_len() directly, and the change to get_dtype() in
fs/ext4/ext4.h (masking filetype with EXT4_FT_MASK before comparing against
EXT4_FT_MAX) is not mentioned in the commit message.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c221bcd562cc9..9953583ad6c3f 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -611,14 +611,21 @@ static inline void dx_set_limit(struct dx_entry *entries, unsigned value)
>  	((struct dx_countlimit *) entries)->limit = cpu_to_le16(value);
>  }
>  
> -static inline unsigned dx_root_limit(struct inode *dir, unsigned infosize)
> +static inline unsigned dx_root_limit(struct inode *dir,
> +	struct ext4_dir_entry_2 *dot_de)
>  {
> -	unsigned int entry_space = dir->i_sb->s_blocksize -
> -			ext4_dir_rec_len(1, NULL) -
> -			ext4_dir_rec_len(2, NULL) - infosize;
> +	struct dx_root_info *info;
> +	unsigned int entry_space, used;
>  
> +	info = dx_get_dx_info(dir, dot_de);
> +	if (IS_ERR(info))
> +		return 0;
> +	used = (char *)info - (char *)dot_de + info->info_length;
>  	if (ext4_has_feature_metadata_csum(dir->i_sb))
> -		entry_space -= sizeof(struct dx_tail);
> +		used += sizeof(struct dx_tail);
> +	if (used > dir->i_sb->s_blocksize)
> +		return 0;
> +	entry_space = dir->i_sb->s_blocksize - used;
>  	return entry_space / sizeof(struct dx_entry);
>  }

[ ... ]

> @@ -2372,10 +2382,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>  		dx_info->hash_version =
>  				EXT4_SB(dir->i_sb)->s_def_hash_version;
>  
> -	entries = (void *)dx_info + sizeof(*dx_info);
> +	entries = (void *)dx_info + dx_info->info_length;
>  	dx_set_block(entries, 1);
>  	dx_set_count(entries, 1);
> -	dx_set_limit(entries, dx_root_limit(dir, sizeof(*dx_info)));
> +	dx_set_limit(entries, dx_root_limit(dir, dot_de));

[Severity: Medium]
Can calling dx_root_limit(dir, dot_de) after dotdot_de->rec_len has been
expanded cause dx_get_dx_info() to resolve a different dx_root_info offset
than the initial call in make_indexed_dir()?

Earlier in make_indexed_dir(), dx_info is computed before modifying the
directory block, while dotdot_de->rec_len still holds its original unindexed
entry length (for example, 12 bytes):

	dx_info = dx_get_dx_info(dir, dot_de);

Inside dx_get_dx_info() -> ext4_dir_entry_len() -> ext4_dirent_get_data_len(),
if dotdot_de->file_type has upper dirdata flag bits set (such as on a crafted
or corrupted image) and the dirdata length byte at offset 11 exceeds the
remaining space in the 12-byte entry, ext4_dirent_get_data_len() sees:

	if (ddh_len == 0 || ddh_len > rec_len - offset)
		return 0;

and returns 0, placing dx_info right after the 12-byte '..' entry.

Then make_indexed_dir() wipes the block starting at de (leaving the bytes of
dotdot_de intact) and expands dotdot_de->rec_len to cover the rest of the
block:

	dotdot_de->rec_len =
		ext4_rec_len_to_disk(blocksize - le16_to_cpu(dot_de->rec_len),
				     blocksize);

When dx_root_limit(dir, dot_de) calls dx_get_dx_info(dir, dot_de) a second
time, rec_len is now blocksize - 12 (e.g. 4084). In ext4_dirent_get_data_len(),
ddh_len > rec_len - offset is no longer true, so it returns a non-zero
dirdata length. That shifts the info pointer inside dx_root_limit() past the
dx_info structure that was just initialized (or causes dx_get_dx_info() to
return ERR_PTR(-EFSCORRUPTED)), writing a wrong or zero limit into entries.

Since both dx_probe() and make_indexed_dir() already have the validated
struct dx_root_info pointer, could dx_root_limit() take info (or its offset)
directly instead of calling dx_get_dx_info() again?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134921.610047-1-ablagodarenko@thelustrecollective.com?part=7