| Message ID | 20220620134947.2772863-2-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce dedicated type for idmapped mounts | expand |
On Mon, Jun 20, 2022 at 8:50 AM Christian Brauner <brauner@kernel.org> wrote: > > Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types > are just simple wrapper structs around regular {g,u}id_t types. Thanks for working on this., I haven't actually perused the series yet, but wanted to just immediately react to "please don't make random-letter type names". "gid" is something people understand. It's a thing. "kgid" kind of made sense, in that it's the "kernel view of the gid", and it was still short and fairly legible. "kmntgid" is neither short, legible, or makes sense. For one thing, the "k" part no longer makes any sense. It's not about the "kernel view of the gid" any more. Sure, it's "kernel" in the sense that any kernel code is "kernel", but it's no longer some kind of "unified kernel view of a user-namespace gid". So the "k" in "kgid" doesn't make sense because it's a kernel thing, but more as a negative: "it is *not* the user visible gid". So instead of changing all our old "git_t" definitions to be "ugid_t" (for "user visible gid") the "kgid_t" thing was done. As a result: when you translate it to the mount namespace, I do not believe that the "k" makes sense any more, because now the point to distinguish it from "user gids" no longer exists. So it's just one random letter. In a long jumble of letters that isn't really very legible or pronounceable. If it didn't have that 'i' in it, I would think it's a IBM mnemonic (and I use the word "mnemonic" ironically) for some random assembler instruction. They used up all the vowels they were willing to use for the "eieio" instructions, and all other instruction names are a jumble of random consonants. So please try to make the type names less of a random jumble of letters picked from a bag. That "kmnt[gu]id" pattern exists elsewhere too, in the conversion functions etc, so it's not just the type name, but more of a generic "please don't use letter-jumble names". Maybe just "mnt_[gu]id"" instead of "kmnt[gu]id" would be better. But even that smells wrong to me. Isn't it really "the guid/uid seen by the filesystem after the mount mapping"? Wouldn't it be nice to name by the same "seen by users" and "seen by kernel" to be "seen by filesystrem"? So wouln't a name like "fs_[gu]id_t" make even more sense? I dunno. Maybe I'm thinking about this the wrong way, but I wish the names would be more explanatory. My personal mental image is that the user namespaces map a traditional uid into the "kernel id space", and then the mount id mappings map into the "filesystem id space". Which is why to me that "k" doesn't make sense, and the "mnt" doesn't really make tons of sense either (the mount is the thing that _maps_ the id spaces, but it's not the end result of said mapping). IOW, I get the feeling that you've named the result with the mapping, not with the end use. That would be like naming "kuid" by the mapping (usernamespace), not the end result (the kernel namespace). But maybe it's just me that is confused here. Particularly since I didn't really more than *very* superficially and quickly scan the patches. Linus
On Mon, Jun 20, 2022 at 09:28:00AM -0500, Linus Torvalds wrote: > On Mon, Jun 20, 2022 at 8:50 AM Christian Brauner <brauner@kernel.org> wrote: > > > > Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types > > are just simple wrapper structs around regular {g,u}id_t types. > > Thanks for working on this., > > I haven't actually perused the series yet, but wanted to just > immediately react to "please don't make random-letter type names". > > "gid" is something people understand. It's a thing. > > "kgid" kind of made sense, in that it's the "kernel view of the gid", > and it was still short and fairly legible. > > "kmntgid" is neither short, legible, or makes sense. > > For one thing, the "k" part no longer makes any sense. It's not about > the "kernel view of the gid" any more. Sure, it's "kernel" in the > sense that any kernel code is "kernel", but it's no longer some kind > of "unified kernel view of a user-namespace gid". > > So the "k" in "kgid" doesn't make sense because it's a kernel thing, > but more as a negative: "it is *not* the user visible gid". > > So instead of changing all our old "git_t" definitions to be "ugid_t" > (for "user visible gid") the "kgid_t" thing was done. > > As a result: when you translate it to the mount namespace, I do not > believe that the "k" makes sense any more, because now the point to > distinguish it from "user gids" no longer exists. So it's just one > random letter. In a long jumble of letters that isn't really very > legible or pronounceable. > > If it didn't have that 'i' in it, I would think it's a IBM mnemonic > (and I use the word "mnemonic" ironically) for some random assembler > instruction. They used up all the vowels they were willing to use for > the "eieio" instructions, and all other instruction names are a jumble > of random consonants. > > So please try to make the type names less of a random jumble of > letters picked from a bag. > > That "kmnt[gu]id" pattern exists elsewhere too, in the conversion > functions etc, so it's not just the type name, but more of a generic > "please don't use letter-jumble names". > > Maybe just "mnt_[gu]id"" instead of "kmnt[gu]id" would be better. > > But even that smells wrong to me. Isn't it really "the guid/uid seen > by the filesystem after the mount mapping"? Wouldn't it be nice to > name by the same "seen by users" and "seen by kernel" to be "seen by > filesystrem"? So wouln't a name like "fs_[gu]id_t" make even more > sense? > > I dunno. Maybe I'm thinking about this the wrong way, but I wish the > names would be more explanatory. My personal mental image is that the > user namespaces map a traditional uid into the "kernel id space", and > then the mount id mappings map into the "filesystem id space". Which Yes. Basically without idmapped mounts if the caller's idmapping and the filesystem's idmapping contain the same kuid then the uid/gid passed in from userspace can be represented in the filesystem idmapping and thus ultimately on-disk. That's the very basic model. If the caller uses an idmapped mount then the caller's idmapping and the filesystem's idmapping are connected via the mount's idmapping. Iow, the mount remaps the caller's kuid to a different kuid in the filesystem's idmapping. > is why to me that "k" doesn't make sense, and the "mnt" doesn't really > make tons of sense either (the mount is the thing that _maps_ the id > spaces, but it's not the end result of said mapping). Yeah, fair point. > > IOW, I get the feeling that you've named the result with the mapping, > not with the end use. That would be like naming "kuid" by the mapping > (usernamespace), not the end result (the kernel namespace). > > But maybe it's just me that is confused here. Particularly since I > didn't really more than *very* superficially and quickly scan the > patches. Originally I called that kfs{g,u}id_t but I was never happy with that either... I think either vfs{g,u}id_t or fs_{g,u}id_t makes sense.
On Mon, Jun 20, 2022 at 10:25 AM Christian Brauner <brauner@kernel.org> wrote: > > Originally I called that kfs{g,u}id_t but I was never happy with that > either... I think either vfs{g,u}id_t or fs_{g,u}id_t makes sense. vfs[gu]id sounds good to me. That way we avoid the confusion with our current "cred->fsuid" thing due to the access() system call. Linus
diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h index ee5a217de2a8..8dbaef494e02 100644 --- a/include/linux/mnt_idmapping.h +++ b/include/linux/mnt_idmapping.h @@ -13,6 +13,122 @@ struct user_namespace; */ extern struct user_namespace init_user_ns; +typedef struct { + uid_t val; +} kmntuid_t; + +typedef struct { + gid_t val; +} kmntgid_t; + +#ifdef CONFIG_MULTIUSER +static inline uid_t __kmntuid_val(kmntuid_t uid) +{ + return uid.val; +} + +static inline gid_t __kmntgid_val(kmntgid_t gid) +{ + return gid.val; +} +#else +static inline uid_t __kmntuid_val(kmntuid_t uid) +{ + return 0; +} + +static inline gid_t __kmntgid_val(kmntgid_t gid) +{ + return 0; +} +#endif + +static inline bool kmntuid_valid(kmntuid_t uid) +{ + return __kmntuid_val(uid) != (uid_t) -1; +} + +static inline bool kmntgid_valid(kmntgid_t gid) +{ + return __kmntgid_val(gid) != (gid_t) -1; +} + +/** + * kuid_eq_kmntuid - check whether kuid and kmntuid have the same value + * @kuid: the kuid to compare + * @kmntuid: the kmntuid to compare + * + * Check whether @kuid and @kmntuid have the same values. + * + * Return: true if @kuid and @kmntuid have the same value, false if not. + */ +static inline bool kuid_eq_kmntuid(kuid_t kuid, kmntuid_t kmntuid) +{ + return __kmntuid_val(kmntuid) == __kuid_val(kuid); +} + +/** + * kgid_eq_kmntgid - check whether kgid and kmntgid have the same value + * @kgid: the kgid to compare + * @kmntgid: the kmntgid to compare + * + * Check whether @kgid and @kmntgid have the same values. + * + * Return: true if @kgid and @kmntgid have the same value, false if not. + */ +static inline bool kgid_eq_kmntgid(kgid_t kgid, kmntgid_t kmntgid) +{ + return __kmntgid_val(kmntgid) == __kgid_val(kgid); +} + +static inline bool kmntuid_eq(kmntuid_t left, kmntuid_t right) +{ + return __kmntuid_val(left) == __kmntuid_val(right); +} + +static inline bool kmntgid_eq(kmntgid_t left, kmntgid_t right) +{ + return __kmntgid_val(left) == __kmntgid_val(right); +} + +/* + * kmnt{g,u}ids are created from k{g,u}ids. + * We don't allow them to be created from regular {u,g}id. + */ +#define KMNTUIDT_INIT(val) (kmntuid_t){ __kuid_val(val) } +#define KMNTGIDT_INIT(val) (kmntgid_t){ __kgid_val(val) } + +#define INVALID_KMNTUID KMNTUIDT_INIT(INVALID_UID) +#define INVALID_KMNTGID KMNTGIDT_INIT(INVALID_GID) + +/* + * Allow a kmnt{g,u}id to be used as a k{g,u}id where we want to compare + * whether the mapped value is identical to value of a k{g,u}id. + */ +#define AS_KUIDT(val) (kuid_t){ __kmntuid_val(val) } +#define AS_KGIDT(val) (kgid_t){ __kmntgid_val(val) } + +#ifdef CONFIG_MULTIUSER +/** + * kmntgid_in_group_p() - check whether a kmntuid matches the caller's groups + * @kmntgid: the mnt gid to match + * + * This function can be used to determine whether @kmntuid matches any of the + * caller's groups. + * + * Return: 1 if kmntuid matches caller's groups, 0 if not. + */ +static inline int kmntgid_in_group_p(kmntgid_t kmntgid) +{ + return in_group_p(AS_KGIDT(kmntgid)); +} +#else +static inline int kmntgid_in_group_p(kmntgid_t kmntgid) +{ + return 1; +} +#endif + /** * initial_idmapping - check whether this is the initial mapping * @ns: idmapping to check @@ -157,6 +273,43 @@ static inline kuid_t mapped_kuid_user(struct user_namespace *mnt_userns, return make_kuid(fs_userns, uid); } +/** + * kmntuid_to_kuid - map a kmntuid into the filesystem idmapping + * @mnt_userns: the mount's idmapping + * @fs_userns: the filesystem's idmapping + * @kmntuid : kmntuid to be mapped + * + * Map @kmntuid into the filesystem idmapping. This function has to be used in + * order to e.g. write @kmntuid to inode->i_uid. + * + * Return: @kmntuid mapped into the filesystem idmapping + */ +static inline kuid_t kmntuid_to_kuid(struct user_namespace *mnt_userns, + struct user_namespace *fs_userns, + kmntuid_t kmntuid) +{ + return mapped_kuid_user(mnt_userns, fs_userns, AS_KUIDT(kmntuid)); +} + +/** + * kmntuid_has_mapping - check whether a kmntuid maps into the filesystem + * @mnt_userns: the mount's idmapping + * @fs_userns: the filesystem's idmapping + * @kmntuid: kmntuid to be mapped + * + * Check whether @kmntuid has a mapping in the filesystem idmapping. Use this + * function to check whether the filesystem idmapping has a mapping for + * @kmntuid. + * + * Return: true if @kmntuid has a mapping in the filesystem, false if not. + */ +static inline bool kmntuid_has_mapping(struct user_namespace *mnt_userns, + struct user_namespace *fs_userns, + kmntuid_t kmntuid) +{ + return uid_valid(kmntuid_to_kuid(mnt_userns, fs_userns, kmntuid)); +} + /** * mapped_kgid_user - map a user kgid into a mnt_userns * @mnt_userns: the mount's idmapping @@ -193,6 +346,43 @@ static inline kgid_t mapped_kgid_user(struct user_namespace *mnt_userns, return make_kgid(fs_userns, gid); } +/** + * kmntgid_to_kgid - map a kmntgid into the filesystem idmapping + * @mnt_userns: the mount's idmapping + * @fs_userns: the filesystem's idmapping + * @kmntgid : kmntgid to be mapped + * + * Map @kmntgid into the filesystem idmapping. This function has to be used in + * order to e.g. write @kmntgid to inode->i_gid. + * + * Return: @kmntgid mapped into the filesystem idmapping + */ +static inline kgid_t kmntgid_to_kgid(struct user_namespace *mnt_userns, + struct user_namespace *fs_userns, + kmntgid_t kmntgid) +{ + return mapped_kgid_user(mnt_userns, fs_userns, AS_KGIDT(kmntgid)); +} + +/** + * kmntgid_has_mapping - check whether a kmntgid maps into the filesystem + * @mnt_userns: the mount's idmapping + * @fs_userns: the filesystem's idmapping + * @kmntgid: kmntgid to be mapped + * + * Check whether @kmntgid has a mapping in the filesystem idmapping. Use this + * function to check whether the filesystem idmapping has a mapping for + * @kmntgid. + * + * Return: true if @kmntgid has a mapping in the filesystem, false if not. + */ +static inline bool kmntgid_has_mapping(struct user_namespace *mnt_userns, + struct user_namespace *fs_userns, + kmntgid_t kmntgid) +{ + return gid_valid(kmntgid_to_kgid(mnt_userns, fs_userns, kmntgid)); +} + /** * mapped_fsuid - return caller's fsuid mapped up into a mnt_userns * @mnt_userns: the mount's idmapping
Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types are just simple wrapper structs around regular {g,u}id_t types. They allows to establish a type safety boundary between {g,u}ids on idmapped mounts and {g,u}ids as they are represented in filesystems themselves. A kmnt{g,u}id_t is always created from a k{g,u}id_t, never directly from a {g,u}id_t as idmapped mounts remap a given {g,u}id according to the mount's idmapping. This is expressed in the KMNT{G,U}IDT_INIT() macros. A kmnt{g,u}id_t may be used as a k{g,u}id_t via AS_K{G,U}IDT(). This often happens when we need to check whether a {g,u}id mapped according to an idmapped mount is identical to a given k{g,u}id_t. For an example, see kmntgid_in_group_p() which determines whether the value of kmntgid_t matches the value of any of the caller's groups. Similar logic is expressed in the k{g,u}id_eq_kmnt{g,u}id(). The kmnt{g,u}id_to_k{g,u}id() helpers map a given kmnt{g,u}id_t from the mount's idmapping into the filesystem idmapping. They make it possible to update a filesystem object such as inode->i_{g,u}id with the correct value. This makes it harder to accidently write a wrong {g,u}id anwywhere. The kmnt{g,u}id_has_mapping() helpers check whether a given kmnt{g,u}id_t can be represented in the filesystem idmapping. All new helpers are nops on non-idmapped mounts. I've done work on this roughly 7 months ago but dropped it to focus on the testsuite. Linus brought this up independently just last week and it's time to move this along (see [1]). [1]: https://lore.kernel.org/lkml/CAHk-=win6+ahs1EwLkcq8apqLi_1wXFWbrPf340zYEhObpz4jA@mail.gmail.com Cc: Seth Forshee <sforshee@digitalocean.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Aleksa Sarai <cyphar@cyphar.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Al Viro <viro@zeniv.linux.org.uk> CC: linux-fsdevel@vger.kernel.org Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- include/linux/mnt_idmapping.h | 190 ++++++++++++++++++++++++++++++++++ 1 file changed, 190 insertions(+)