Re: [RFC PATCH v2 31/41] ext4: add the uid_r feature for UUID and GUID owners

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ext4: unverified extra_isize expansion in ext4_make_room_for_id_r()
- [Medium] ext4: UUID/GUID owners lost in ext4_xattr_inode_create() and ext4_ext_migrate()
- [Medium] ext4: debug_want_extra_isize can set s_want_extra_isize too small for uid_r
- [Medium] ext4: NO_UID32 clobbers 0xffffffff sentinel in ext4_fill_raw_inode()
- [Medium] ext4: race in ext4_make_room_for_id_r() with concurrent inode expansion
- [Medium] ext4: unsynchronized 20-byte i_uid and i_gid accesses in ext4_fill_raw_inode()
--

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

ext4: add the uid_r feature for UUID and GUID owners

This commit adds the read-only compatible uid_r feature to ext4, extending
struct ext4_inode with i_uid_r_type, i_gid_r_type, i_uid_r, and i_gid_r to
store 16-byte UUID and GUID owners and groups while storing 0xffffffff in
the classic i_uid and i_gid fields. It also sets SB_I_UID_R on the
superblock and expands existing inodes in ext4_setattr() when assigning a
UUID or GUID owner or group.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 26f0f9714f03c..bed8d535a6e4e 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4780,6 +4780,75 @@ static int ext4_inode_blocks_set(struct ext4_inode *raw_inode,
>  	return 0;
>  }
>  
> +/*
> + * uid_r: a UUID or GUID owner and group live in i_uid_r and i_gid_r of
> + * inodes with room for them; i_uid and i_gid then hold 0xffffffff.
> + */
> +static int ext4_inode_set_id_r(struct inode *inode, struct ext4_inode *raw_inode)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	bool fits = EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
> +		    EXT4_FITS_IN_INODE(raw_inode, ei, i_gid_r);
> +	bool legacy = uid_is_legacy(inode->i_uid) && gid_is_legacy(inode->i_gid);
> +
> +	if (!legacy && (!ext4_has_feature_uid_r(inode->i_sb) || !fits))
> +		return -EOVERFLOW;
> +	if (!fits || !ext4_has_feature_uid_r(inode->i_sb))
> +		return 0;
> +
> +	raw_inode->i_id_r_reserved = 0;
> +	if (uid_is_legacy(inode->i_uid)) {
> +		raw_inode->i_uid_r_type = EXT4_ID_R_LEGACY;
> +		memset(raw_inode->i_uid_r, 0, sizeof(raw_inode->i_uid_r));
> +	} else {
> +		BUILD_BUG_ON(KID_UUID != EXT4_ID_R_UUID ||
> +			     KID_GUID != EXT4_ID_R_GUID);
> +		raw_inode->i_uid_r_type = inode->i_uid.type;
> +		memcpy(raw_inode->i_uid_r, inode->i_uid.uuid.b,
> +		       sizeof(raw_inode->i_uid_r));
> +	}
> +	if (gid_is_legacy(inode->i_gid)) {
> +		raw_inode->i_gid_r_type = EXT4_ID_R_LEGACY;
> +		memset(raw_inode->i_gid_r, 0, sizeof(raw_inode->i_gid_r));
> +	} else {
> +		raw_inode->i_gid_r_type = inode->i_gid.type;
> +		memcpy(raw_inode->i_gid_r, inode->i_gid.uuid.b,
> +		       sizeof(raw_inode->i_gid_r));
> +	}
> +	return 0;
> +}

[Severity: Medium]
With UUID and GUID support, kuid_t and kgid_t are 20-byte structures, so
reads and writes of inode->i_uid and inode->i_gid are multi-word and not
atomic.

In fs/ext4/inode.c, ext4_setattr() updates inode->i_uid and inode->i_gid
under inode->i_rwsem without holding EXT4_I(inode)->i_raw_lock:

fs/ext4/inode.c:ext4_setattr() {
    ...
    i_uid_update(idmap, attr, inode);
    i_gid_update(idmap, attr, inode);
    error = ext4_mark_inode_dirty(handle, inode);
}

Concurrently, paths that dirty the inode without holding inode->i_rwsem
(such as touch_atime() -> ext4_dirty_inode(), or unwritten extent
conversion in ext4_end_io_rsv_work() -> ext4_mark_inode_dirty()) call
ext4_do_update_inode() -> ext4_fill_raw_inode() -> ext4_inode_set_id_r()
under ei->i_raw_lock, and __ext4_get_inode_loc() also calls
ext4_fill_raw_inode() under lock_buffer(bh).

Because ext4_fill_raw_inode() first reads inode->i_uid and inode->i_gid via
i_uid_read() and i_gid_read(), and ext4_inode_set_id_r() then re-reads
inode->i_uid and inode->i_gid multiple times, can a concurrent chown race
with ext4_fill_raw_inode() and write a torn 16-byte UUID/GUID or mismatched
classic i_uid/i_gid and i_uid_r/i_gid_r fields (with a valid metadata
checksum computed over that inconsistent state) into raw_inode?

[ ... ]

> @@ -4792,8 +4861,10 @@ static int ext4_fill_raw_inode(struct inode *inode, struct ext4_inode *raw_inode
>  	err = ext4_inode_blocks_set(raw_inode, ei);
>  
>  	raw_inode->i_mode = cpu_to_le16(inode->i_mode);
> +	/* a UUID or GUID owner: 0xffffffff here, the id in i_uid_r */
>  	i_uid = i_uid_read(inode);
>  	i_gid = i_gid_read(inode);
> +	err = err ?: ext4_inode_set_id_r(inode, raw_inode);

[Severity: Medium]
When an inode has a UUID or GUID owner or group, i_uid_read(inode) and
i_gid_read(inode) return (uid_t)-1 (0xffffffff).

Both ext4_xattr_inode_create() in fs/ext4/xattr.c and ext4_ext_migrate() in
fs/ext4/migrate.c copy the parent inode's owner into a uid_t owner[2] array
using i_uid_read() and i_gid_read() and pass that array to ext4_new_inode():

fs/ext4/xattr.c:ext4_xattr_inode_create() {
    struct inode *ea_inode = NULL;
    uid_t owner[2] = { i_uid_read(inode), i_gid_read(inode) };
    ...
    ea_inode = ext4_new_inode(handle, inode->i_sb->s_root->d_inode,
                              S_IFREG | 0600, NULL, inode->i_ino + 1, owner,
                              EXT4_EA_INODE_FL);
}

fs/ext4/migrate.c:ext4_ext_migrate() {
    ...
    owner[0] = i_uid_read(inode);
    owner[1] = i_gid_read(inode);
    tmp_inode = ext4_new_inode(handle, d_inode(inode->i_sb->s_root),
                               S_IFREG, NULL, goal, owner, 0);
}

In fs/ext4/ialloc.c, __ext4_new_inode() then passes those 32-bit values to
i_uid_write() and i_gid_write() before calling dquot_initialize():

fs/ext4/ialloc.c:__ext4_new_inode() {
    if (owner) {
        inode->i_mode = mode;
        i_uid_write(inode, owner[0]);
        i_gid_write(inode, owner[1]);
    }
    ...
    err = dquot_initialize(inode);
}

Because make_kuid(&init_user_ns, (uid_t)-1) produces INVALID_UID (with
KID_LEGACY and value -1) rather than preserving the UUID or GUID,
qid_has_mapping() in dqget() returns false and causes dquot_initialize() to
fail with -EINVAL when quotas are enabled, or writes out the helper inode
with EXT4_ID_R_LEGACY and i_uid 0xffffffff when quotas are disabled.

Should ext4_xattr_inode_create() and ext4_ext_migrate() preserve the full
kuid_t and kgid_t owner and group when creating helper inodes for a UUID or
GUID owned file?

[ ... ]

> @@ -6006,6 +6079,40 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
>   *
>   * Called with inode->i_rwsem down.
>   */
> +/*
> + * An inode from before uid_r, or with in-inode xattrs in the way, may
> + * lack room for i_uid_r and i_gid_r: make it, before it gets a UUID or
> + * GUID owner or group.
> + */
> +static int ext4_make_room_for_id_r(struct mnt_idmap *idmap,
> +				   struct inode *inode, struct iattr *attr)
> +{
> +	struct ext4_iloc iloc;
> +	bool legacy = true;
> +	int err;
> +
> +	if (attr->ia_valid & ATTR_UID)
> +		legacy = uid_is_legacy(from_vfsuid(idmap, i_user_ns(inode),
> +						   attr->ia_vfsuid));
> +	if (attr->ia_valid & ATTR_GID)
> +		legacy &= gid_is_legacy(from_vfsgid(idmap, i_user_ns(inode),
> +						    attr->ia_vfsgid));
> +	if (legacy)
> +		return 0;
> +	if (!ext4_has_feature_uid_r(inode->i_sb))
> +		return -EOVERFLOW;
> +
> +	err = ext4_get_inode_loc(inode, &iloc);
> +	if (err)
> +		return err;
> +	if (EXT4_FITS_IN_INODE(ext4_raw_inode(&iloc), EXT4_I(inode), i_gid_r)) {
> +		brelse(iloc.bh);
> +		return 0;
> +	}

[Severity: Medium]
In ext4_make_room_for_id_r(), EXT4_FITS_IN_INODE() is checked without
holding EXT4_I(inode)->xattr_sem before calling ext4_expand_extra_isize()
with EXT4_EXTRA_ISIZE_UID_R.

Concurrently, paths that dirty the inode without holding inode->i_rwsem
(such as ext4_dirty_inode(), ext4_page_mkwrite(), or
ext4_end_io_rsv_work()) can call __ext4_mark_inode_dirty() ->
ext4_try_to_expand_extra_isize(), which acquires xattr_sem via
ext4_write_trylock_xattr() and sets EXT4_STATE_NO_EXPAND while xattr_sem is
held.

If ext4_expand_extra_isize() runs during that window, it checks
EXT4_STATE_NO_EXPAND before taking xattr_sem:

fs/ext4/inode.c:ext4_expand_extra_isize() {
    if (ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) {
        brelse(iloc->bh);
        return -EOVERFLOW;
    }
    ...
}

Furthermore, if ext4_expand_extra_isize() passes that check and blocks on
ext4_write_lock_xattr() while ext4_try_to_expand_extra_isize() expands
ei->i_extra_isize to sbi->s_want_extra_isize (when sbi->s_want_extra_isize
exceeds EXT4_EXTRA_ISIZE_UID_R), __ext4_expand_extra_isize() will then see
new_extra_isize < ei->i_extra_isize and return -EINVAL:

fs/ext4/inode.c:__ext4_expand_extra_isize() {
    if ((new_extra_isize < ei->i_extra_isize) ||
        (new_extra_isize < 4) ||
        (new_extra_isize > inode_size - EXT4_GOOD_OLD_INODE_SIZE))
        return -EINVAL;	/* Should never happen */
}

Could this race cause ext4_make_room_for_id_r() to spuriously fail a chown
operation with -EOVERFLOW even when sufficient extra inode space is
available?

> +	err = ext4_expand_extra_isize(inode, EXT4_EXTRA_ISIZE_UID_R, &iloc);
> +	return err ? -EOVERFLOW : 0;
> +}

[Severity: High]
What happens here if ext4_expand_extra_isize() returns 0 without expanding
i_extra_isize all the way to EXT4_EXTRA_ISIZE_UID_R?

When an inode has in-inode extended attributes (EXT4_STATE_XATTR) that
cannot be moved out to free 36 bytes (for example, inline data or an
external xattr block without enough free space), ext4_expand_extra_isize()
-> __ext4_expand_extra_isize() -> ext4_expand_extra_isize_ea() falls back
to s_min_extra_isize (typically 32 bytes) on -ENOSPC and jumps to retry:

fs/ext4/xattr.c:ext4_expand_extra_isize_ea() {
retry:
    isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
    if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
        return 0;
    ...
    if (error) {
        if (error == -ENOSPC && !tried_min_extra_isize &&
            s_min_extra_isize) {
            tried_min_extra_isize++;
            new_extra_isize = s_min_extra_isize;
            error = 0;
            goto retry;
        }
        goto cleanup;
    }
}

When i_extra_isize is already at least s_min_extra_isize (such as 32 bytes
on an inode created before uid_r), ext4_expand_extra_isize_ea() returns 0
at the retry label while i_extra_isize is still smaller than
EXT4_EXTRA_ISIZE_UID_R.

ext4_make_room_for_id_r() then returns 0, so ext4_setattr() updates
inode->i_uid and inode->i_gid in memory to the UUID/GUID and calls
ext4_mark_inode_dirty() -> ext4_do_update_inode() -> ext4_fill_raw_inode()
-> ext4_inode_set_id_r(), which sees !legacy && !fits, returns -EOVERFLOW,
and triggers a false-positive filesystem corruption error:

fs/ext4/inode.c:ext4_do_update_inode() {
    err = ext4_fill_raw_inode(inode, raw_inode);
    spin_unlock(&ei->i_raw_lock);
    if (err) {
        EXT4_ERROR_INODE(inode, "corrupted inode contents");
        goto out_brelse;
    }
}

Should ext4_make_room_for_id_r() verify that i_gid_r actually fits in the
inode after ext4_expand_extra_isize() returns?

[ ... ]

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d636b89090a24..34ac087c32dcb 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4613,9 +4613,17 @@ static int ext4_inode_info_init(struct super_block *sb,
>  		sb->s_time_min = EXT4_TIMESTAMP_MIN;
>  	}
>  
> +	if (ext4_has_feature_uid_r(sb) &&
> +	    sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE + EXT4_EXTRA_ISIZE_UID_R) {
> +		ext4_msg(sb, KERN_ERR, "uid_r needs inodes of at least %zu bytes",
> +			 EXT4_GOOD_OLD_INODE_SIZE + EXT4_EXTRA_ISIZE_UID_R);
> +		return -EINVAL;
> +	}
> +
>  	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -			EXT4_GOOD_OLD_INODE_SIZE;
> +		/* room for the uid_r fields in new inodes only if needed */
> +		sbi->s_want_extra_isize = ext4_has_feature_uid_r(sb) ?
> +			EXT4_EXTRA_ISIZE_UID_R : EXT4_EXTRA_ISIZE_BASE;

[Severity: Medium]
In __ext4_fill_super(), ext4_inode_info_init() initializes
sbi->s_want_extra_isize to EXT4_EXTRA_ISIZE_UID_R, but
parse_apply_sb_mount_options(), ext4_check_opt_consistency(), and
ext4_apply_options() run right afterwards (and on remount in
__ext4_remount()) and overwrite sbi->s_want_extra_isize when the
debug_want_extra_isize mount option is specified.

Because ext4_check_opt_consistency() only checks the upper bound of
ctx->s_want_extra_isize:

fs/ext4/super.c:ext4_check_opt_consistency() {
    if (ctx->s_want_extra_isize >
        (sbi->s_inode_size - EXT4_GOOD_OLD_INODE_SIZE)) {
        ext4_msg(NULL, KERN_ERR,
             "Invalid want_extra_isize %d",
             ctx->s_want_extra_isize);
        return -EINVAL;
    }
}

mounting or remounting a uid_r filesystem with debug_want_extra_isize
smaller than EXT4_EXTRA_ISIZE_UID_R causes __ext4_new_inode() to initialize
ei->i_extra_isize to that smaller value.

If a new inode is then created with a UUID or GUID owner or group, won't
ext4_mark_inode_dirty() -> ext4_do_update_inode() -> ext4_fill_raw_inode()
-> ext4_inode_set_id_r() return -EOVERFLOW and trigger
EXT4_ERROR_INODE(inode, "corrupted inode contents")?

>  		if (ext4_has_feature_extra_isize(sb)) {
>  			unsigned v, max = (sbi->s_inode_size -
>  					   EXT4_GOOD_OLD_INODE_SIZE);
> @@ -5437,6 +5445,10 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	/* HSM events are allowed by default. */
>  	sb->s_iflags |= SB_I_ALLOW_HSM;
>  
> +	/* uid_r: inodes can have UUID and GUID owners */
> +	if (ext4_has_feature_uid_r(sb) && !test_opt(sb, NO_UID32))
> +		sb->s_iflags |= SB_I_UID_R;

[Severity: Medium]
While __ext4_fill_super() avoids setting SB_I_UID_R when NO_UID32 is
enabled at mount time, __ext4_remount() allows enabling NO_UID32 on remount
without clearing SB_I_UID_R, and ext4_inode_get_id_r() loads UUID and GUID
owners into inode->i_uid and inode->i_gid regardless of NO_UID32.

When an inode with a UUID or GUID owner or group is dirtied and written out
in ext4_fill_raw_inode() while NO_UID32 is set, i_uid_read(inode) and
i_gid_read(inode) return 0xffffffff, and the NO_UID32 branch converts them
with fs_high2lowuid() and fs_high2lowgid():

fs/ext4/inode.c:ext4_fill_raw_inode() {
    if (!(test_opt(inode->i_sb, NO_UID32))) {
        ...
    } else {
        raw_inode->i_uid_low = cpu_to_le16(fs_high2lowuid(i_uid));
        raw_inode->i_gid_low = cpu_to_le16(fs_high2lowgid(i_gid));
        raw_inode->i_uid_high = 0;
        raw_inode->i_gid_high = 0;
    }
}

Doesn't this overwrite the on-disk 0xffffffff sentinel in i_uid and i_gid
with 0x0000fffe (65534) while leaving i_uid_r_type and i_gid_r_type set to
EXT4_ID_R_UUID or EXT4_ID_R_GUID?

> +
>  	err = ext4_check_feature_compatibility(sb, es, silent);
>  	if (err)
>  		goto failed_mount;

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