[RFC PATCH v2 31/41] ext4: add the uid_r feature for UUID and GUID owners
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Thu Oct 22 03:17:57 UTC 2026
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 at 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
More information about the Linux-security-module-archive
mailing list