Re: [RFC PATCH v2 17/41] quota: don't account UUID and GUID owners

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] quota: dqget() checks qid_has_mapping() before qid_is_legacy()
- [High] quota: stale dquot retention in dquot_transfer() for UUID and GUID owners
- [Medium] quota: dquot_initialize_needed() stays true for UUID and GUID owners
--

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

quota: don't account UUID and GUID owners

This commit adds a qid_is_legacy() helper and updates dqget() to return
-ESRCH for UUID and GUID owners so that non-legacy 128-bit identifiers are
not accounted in 32-bit quota files.

> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 1c78c695d0ddf..62f333f8243f1 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -954,6 +954,9 @@ struct dquot *dqget(struct super_block *sb, struct kqid qid)
>  
>  	if (!qid_has_mapping(sb->s_user_ns, qid))
>  		return ERR_PTR(-EINVAL);
> +	/* Quota files hold 32 bit ids: like quota off for UUIDs and GUIDs */
> +	if (!qid_is_legacy(qid))
> +		return ERR_PTR(-ESRCH);
>  
>          if (!sb_has_quota_active(sb, qid.type))
>  		return ERR_PTR(-ESRCH);

[Severity: High]
Can the !qid_is_legacy(qid) check here ever be reached for a UUID or GUID
owner?

Prior to this check, dqget() calls qid_has_mapping(sb->s_user_ns, qid):

include/linux/quota.h:qid_has_mapping()
	return from_kqid(ns, qid) != (qid_t) -1;

from_kqid() delegates to from_kuid() and from_kgid(), which unconditionally
return (uid_t)-1 and (gid_t)-1 for non-legacy identifiers even in
init_user_ns:

kernel/user_namespace.c:from_kuid()
	if (!uid_is_legacy(kuid))
		return (uid_t) -1;

Because qid_has_mapping() always returns false for UUID and GUID owners,
dqget() returns ERR_PTR(-EINVAL) before reaching !qid_is_legacy(qid).
Callers such as __dquot_initialize() and dquot_transfer() only treat -ESRCH
as non-fatal and propagate -EINVAL, causing file creation, open for write,
and ownership changes involving UUID or GUID owners to fail on
quota-enabled filesystems.

Should the !qid_is_legacy(qid) check be placed before qid_has_mapping()?

[Severity: High]
When an inode is chowned from a legacy 32-bit owner to a UUID or GUID owner,
will leaving the inode charged to the previous owner in dquot_transfer()
leak quota accounting once the inode is evicted from the icache?

In dquot_transfer() -> __dquot_transfer(), returning -ESRCH from dqget()
sets transfer_to[cnt] = NULL:

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

This skips uncharging transfer_from[cnt] and leaves i_dquot(inode)[cnt]
pointing to the previous owner's dquot while inode->i_uid or inode->i_gid is
updated to the UUID or GUID.

While the inode remains cached, subsequent block allocations by the new
owner are still charged to the previous owner's dquot via
__dquot_alloc_space() and can fail with -EDQUOT against the previous
owner's limits.

Once the inode is evicted from memory, dquot_drop() -> __dquot_drop() drops
the in-memory i_dquot(inode)[cnt] reference without uncharging the quota.
When the inode is later re-read from disk, __dquot_initialize() sees the
UUID or GUID owner and leaves i_dquot(inode)[cnt] as NULL. Subsequent
truncation or unlink (__dquot_free_space(), dquot_free_inode()) will then
never decrement the previous owner's dquot, permanently leaking those blocks
and inodes in the on-disk quota file (or double-charging if chowned back to
a legacy owner).

Should __dquot_transfer() uncharge transfer_from[cnt] and clear dquots[cnt]
when transferring ownership to an unquoted UUID or GUID owner?

[Severity: Medium]
How does returning -ESRCH for UUID and GUID owners interact with
dquot_initialize_needed()?

When dqget() returns -ESRCH while sb_has_quota_active(sb, qid.type) is true,
__dquot_initialize() leaves i_dquot(inode)[cnt] as NULL. However,
dquot_initialize_needed() (as well as dqinit_needed()) assumes that any NULL
slot for an active quota type means the inode still needs quota
initialization:

fs/quota/dquot.c:dquot_initialize_needed()
	dquots = i_dquot(inode);
	for (i = 0; i < MAXQUOTAS; i++)
		if (!dquots[i] && sb_has_quota_active(inode->i_sb, i))
			return true;

For an inode with a UUID or GUID owner or group, dquot_initialize_needed()
remains permanently true even after dquot_initialize() has been called.

In ext4, this causes __ext4_expand_extra_isize() to always bail out with
-EAGAIN:

fs/ext4/inode.c:__ext4_expand_extra_isize()
	if (dquot_initialize_needed(inode))
		return -EAGAIN;

It also triggers WARN_ON_ONCE(dquot_initialize_needed(inode)) in
ext4_xattr_block_set() when EXT4_XATTR_DEBUG is enabled, and causes every
call to __dquot_initialize() on the inode to bypass the fast path and
acquire dq_data_lock.

Should dquot_initialize_needed(), dqinit_needed(), and the loop in
__dquot_initialize() also account for non-legacy identifiers?

[ ... ]

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