Re: [RFC PATCH v2 03/41] uidgid: add uid_is_legacy() and gid_is_legacy()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] uidgid: uid_is_legacy() and gid_is_legacy() bifurcate kernel UID/GID model
--

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

uidgid: add uid_is_legacy() and gid_is_legacy()

This commit adds the uid_is_legacy() and gid_is_legacy() predicate helpers in
include/linux/uidgid.h to check whether a kuid_t or kgid_t holds a 32-bit id
in preparation for storing 128-bit ids in those types.

> diff --git a/include/linux/uidgid.h b/include/linux/uidgid.h
> index 2dc767e08f544..182c682458cb6 100644
> --- a/include/linux/uidgid.h
> +++ b/include/linux/uidgid.h
> @@ -22,6 +22,20 @@ struct uid_gid_map;
>  #define KUIDT_INIT(value) (kuid_t){ value }
>  #define KGIDT_INIT(value) (kgid_t){ value }
>  
> +/*
> + * Whether an id is a classic 32 bit id. All of them are for now; code that
> + * needs one (to store it in 32 bits, for example) checks with these.
> + */
> +static inline bool uid_is_legacy(kuid_t uid)
> +{
> +	return true;
> +}
> +
> +static inline bool gid_is_legacy(kgid_t gid)
> +{
> +	return true;
> +}
> +
>  #ifdef CONFIG_MULTIUSER
>  static inline uid_t __kuid_val(kuid_t uid)
>  {

[Severity: High]
Introducing uid_is_legacy() and gid_is_legacy() in include/linux/uidgid.h to
bifurcate kernel code paths for 20-byte tagged union kuid_t, kgid_t, vfsuid_t,
and vfsgid_t types raises several architectural and correctness regressions.

kuid_t and kgid_t are designed as zero-overhead 32-bit wrappers passed by
value in a single register and embedded in core structures including struct
inode, struct cred, struct group_info, struct user_struct, struct
posix_acl_entry, and struct kstat. Expanding each id from 4 bytes to 20 bytes
grows struct cred by 128 bytes and struct inode by 32 bytes for every cached
inode, while turning pass-by-value helpers and equality checks into 20-byte
struct copies and __builtin_memcmp() calls.

Expanding inode->i_uid and inode->i_gid beyond 32 bits also breaks the
single-word access assumption required by lockless readers in
acl_permission_check() during rcu pathwalk and in generic_fillattr() during
statx() when racing with updates in setattr_copy() and i_uid_update(), which
can result in torn reads.

Furthermore, splitting kernel subsystems with uid_is_legacy() and
gid_is_legacy() so that __kuid_val() and from_kuid() return (uid_t)-1 and
from_kuid_munged() collapses non-legacy ids into overflowuid and overflowgid
breaks the invariant that every valid kuid_t and kgid_t maps uniformly across
the vfs, filesystems, user namespaces, quotas, netlink credentials, and
userspace interfaces.

Should mapping external 128-bit directory ids to 32-bit ids be handled by
userspace identity daemons and existing kernel id-mapping facilities instead
of bifurcating kuid_t and kgid_t with uid_is_legacy() and gid_is_legacy()?

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