Re: [RFC PATCH v2 24/41] shmem, ramfs: allow UUID and GUID owners

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: shmem_fill_super() omits SB_I_UID_R when CONFIG_TMPFS is disabled
- [Medium] mm: shmem: broken tmpfs quota accounting and transfer with SB_I_UID_R
--

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

shmem, ramfs: allow UUID and GUID owners

This commit sets SB_I_UID_R on ramfs and shmem superblocks so that inodes on
these in-memory filesystems can be owned by UUID and GUID values.

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 848316eaa7f4f..b54775e0c3102 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4988,6 +4988,8 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>  	}
>  	sb->s_export_op = &shmem_export_ops;
>  	sb->s_flags |= SB_NOSEC;
> +	/* owners live in the inode only: UUIDs and GUIDs need no format */
> +	sb->s_iflags |= SB_I_UID_R;

[Severity: Medium]
Should SB_I_UID_R be set outside the #ifdef CONFIG_TMPFS block in
mm/shmem.c:shmem_fill_super()?

In shmem_fill_super(), this assignment is placed inside the
#ifdef CONFIG_TMPFS branch:

	sb->s_export_op = &shmem_export_ops;
	sb->s_flags |= SB_NOSEC;
	/* owners live in the inode only: UUIDs and GUIDs need no format */
	sb->s_iflags |= SB_I_UID_R;
...
#else
	sb->s_flags |= SB_NOUSER;
#endif /* CONFIG_TMPFS */
	sb->s_d_flags |= DCACHE_DONTCACHE;

When CONFIG_SHMEM=y and CONFIG_TMPFS=n, shmem_init() mounts shm_mnt via
kern_mount(&shmem_fs_type) -> shmem_fill_super(), taking the #else branch
without setting SB_I_UID_R. Inodes allocated on shm_mnt via
shmem_get_inode() (such as memfd_create() files) still use
shmem_inode_operations with .setattr = shmem_setattr. Calling fchown_r() or
fchownat_r() with a UUID or GUID on those file descriptors will then fail
with -EOVERFLOW in fs/attr.c:notify_change():

	if (ia_valid & ATTR_UID &&
	    !sb_can_own_kuid(inode->i_sb, vfsuid_into_kuid(attr->ia_vfsuid)))
		return -EOVERFLOW;
	if (ia_valid & ATTR_GID &&
	    !sb_can_own_kgid(inode->i_sb, vfsgid_into_kgid(attr->ia_vfsgid)))
		return -EOVERFLOW;

[Severity: Medium]
How does setting SB_I_UID_R interact with tmpfs quotas when
CONFIG_TMPFS_QUOTA is enabled?

The commit message notes that quotas of tmpfs do not account such owners,
because dqget() returns -ESRCH for UUID and GUID owners or groups. However,
allowing UUID and GUID ownership on a quota-enabled tmpfs mount appears to
cause three issues in the quota paths:

1. In mm/shmem.c:shmem_get_inode(), dquot_initialize(inode) leaves
SHMEM_I(inode)->i_dquot[cnt] as NULL when dqget() returns -ESRCH, allowing
UUID/GUID users and groups to bypass default tmpfs mount hard limits
(usrquota_block_hardlimit, usrquota_inode_hardlimit,
grpquota_block_hardlimit, and grpquota_inode_hardlimit).

2. When an existing inode owned by a classic UID/GID A is chowned via
chown_r() or fchown_r() to a UUID/GUID U, mm/shmem.c:shmem_setattr() calls
dquot_transfer() before setattr_copy():

	if (i_uid_needs_update(idmap, attr, inode) ||
	    i_gid_needs_update(idmap, attr, inode)) {
		error = dquot_transfer(idmap, inode, attr);
		if (error)
			return error;
	}

	setattr_copy(idmap, inode, attr);

In fs/quota/dquot.c:dquot_transfer(), dqget() returns -ESRCH for U and sets
transfer_to[cnt] = NULL:

		dquot = dqget(sb, make_kqid_uid(kuid));
		if (IS_ERR(dquot)) {
			if (PTR_ERR(dquot) != -ESRCH) {
				ret = PTR_ERR(dquot);
				goto out_put;
			}
			dquot = NULL;
		}
		transfer_to[USRQUOTA] = dquot;

Then fs/quota/dquot.c:__dquot_transfer() treats a NULL transfer_to[cnt] as
no UID/GID change and skips detaching the old dquot:

	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
		/*
		 * Skip changes for same uid or gid or for turned off quota-type.
		 */
		if (!transfer_to[cnt])
			continue;

As a result, inode->i_uid or inode->i_gid is updated to U, but
SHMEM_I(inode)->i_dquot[cnt] remains attached to classic owner A's dquot, so
subsequent block allocations via shmem_inode_acct_blocks() ->
dquot_alloc_block_nodirty() continue charging A's quota.

3. If that inode is later chowned back from UUID/GUID U to the original
classic UID/GID A, i_uid_needs_update() or i_gid_needs_update() is true, so
dquot_transfer() sets transfer_to[cnt] = dquot(A) while transfer_from[cnt] is
still dquot(A). Because __dquot_transfer() does not check for
transfer_to[cnt] == transfer_from[cnt], it calls dquot_add_inodes() and
dquot_add_space() on dquot(A) before subtracting from transfer_from[cnt],
temporarily double-charging dquot(A) for the file's usage and failing with
-EDQUOT if A's hard limit cannot fit twice the file's usage.

>  
>  #if IS_ENABLED(CONFIG_UNICODE)
>  	if (!ctx->encoding && ctx->strict_encoding) {

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