Re: [PATCH 1/8] mnt_idmapping: add kmnt{g,u}id_t

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux