Re: [RFC PATCH v2 32/41] ext4: let uid_r be turned on while the filesystem is mounted

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ext4: ext4_ioctl_set_tune_sb() mishandles uid_r when already set on disk
--

commit 18ffac15f27841cea0033ca0f9c30bacf945f9ce
Author: Jan Koester <jan.koester@tuxist.de>

ext4: let uid_r be turned on while the filesystem is mounted

This commit allows the uid_r feature (SB_I_UID_R) to be enabled on a mounted
ext4 filesystem during remount or via the EXT4_IOC_SET_TUNE_SB_PARAM ioctl.
When enabled via the ioctl, it also raises s_want_extra_isize to reserve
space for the uid_r fields in new inodes.

> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c8387e6a2c6e9..89bf1d36ec3db 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c

[ ... ]

> @@ -1382,6 +1383,13 @@ static void ext4_sb_setparams(struct ext4_sb_info *sbi,
>  		if (params->set_feature_incompat_mask &
>  		    EXT4_FEATURE_INCOMPAT_CSUM_SEED)
>  			es->s_checksum_seed = cpu_to_le32(sbi->s_csum_seed);
> +		/* as tune2fs does: room for the uid_r fields in new inodes */
> +		if ((params->set_feature_ro_compat_mask &
> +		     EXT4_FEATURE_RO_COMPAT_UID_R) &&
> +		    ext4_has_feature_extra_isize(sbi->s_sb) &&
> +		    le16_to_cpu(es->s_want_extra_isize) < EXT4_EXTRA_ISIZE_UID_R)
> +			es->s_want_extra_isize =
> +				cpu_to_le16(EXT4_EXTRA_ISIZE_UID_R);
>  	}
>  	if (params->set_flags & EXT4_TUNE_FL_FORCE_FSCK)
>  		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);

[ ... ]

> @@ -1461,6 +1470,18 @@ static int ext4_ioctl_set_tune_sb(struct file *filp,
>  		     ~EXT4_TUNE_CLEAR_RO_COMPAT_SUPP))
>  			return -EOPNOTSUPP;
>  
> +		/*
> +		 * uid_r may already be on disk but not in use yet, when it
> +		 * was set while the filesystem was mounted: take that up too.
> +		 */
> +		if (params.set_feature_ro_compat_mask &
> +		    EXT4_FEATURE_RO_COMPAT_UID_R) {
> +			if (sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE +
> +						EXT4_EXTRA_ISIZE_UID_R)
> +				return -EINVAL;
> +			enabling_uid_r = true;
> +		}
> +
>  		/*
>  		 * Filter out the features that are already set from
>  		 * the set_mask.

[Severity: Medium]
In fs/ext4/ioctl.c, ext4_ioctl_set_tune_sb() intends to activate uid_r even
when EXT4_FEATURE_RO_COMPAT_UID_R is already set in es->s_feature_ro_compat
on disk.

However, when the caller passes EXT4_TUNE_FL_FEATURES rather than
EXT4_TUNE_FL_EDIT_FEATURES, ext4_ioctl_set_tune_sb() derives
params.set_feature_ro_compat_mask earlier by masking out features already
present in es->s_feature_ro_compat:

	if (params.set_flags & EXT4_TUNE_FL_FEATURES) {
		...
		params.set_feature_ro_compat_mask =
			params.feature_ro_compat &
			~le32_to_cpu(es->s_feature_ro_compat);

If EXT4_FEATURE_RO_COMPAT_UID_R is already set in es->s_feature_ro_compat,
that bit is cleared from params.set_feature_ro_compat_mask before reaching
the check above, so enabling_uid_r remains false, the sbi->s_inode_size
check is skipped, and ext4_enable_uid_r(sb) is never called.

Additionally, even when EXT4_TUNE_FL_EDIT_FEATURES is used directly,
ext4_ioctl_set_tune_sb() clears already-set features from
params.set_feature_ro_compat_mask immediately after setting enabling_uid_r:

		params.set_feature_ro_compat_mask &=
			~le32_to_cpu(es->s_feature_ro_compat);

When ext4_update_superblocks_fn() then invokes ext4_sb_setparams(),
params->set_feature_ro_compat_mask no longer has EXT4_FEATURE_RO_COMPAT_UID_R
set, so es->s_want_extra_isize is not raised to EXT4_EXTRA_ISIZE_UID_R on
disk in the superblocks.

Does this prevent EXT4_IOC_SET_TUNE_SB_PARAM from enabling uid_r via
EXT4_TUNE_FL_FEATURES and from updating es->s_want_extra_isize on disk when
EXT4_FEATURE_RO_COMPAT_UID_R is already set in es->s_feature_ro_compat?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=32