diff mbox series

[RFC,3/3] mnt_idmapping: inline all low-level helpers

Message ID 20250416-work-mnt_idmap-s_user_ns-v1-3-273bef3a61ec@kernel.org (mailing list archive)
State New
Headers show
Series mnt_idmapping: avoid pointer chase & inline low-level helpers | expand

Commit Message

Christian Brauner April 16, 2025, 1:17 p.m. UTC 
Let's inline all low-level helpers and use likely()/unlikely() to help
the compiler along.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/mnt_idmapping.c            | 145 -----------------------------------------
 include/linux/mnt_idmapping.h | 147 +++++++++++++++++++++++++++++++++++++++---
 kernel/user_namespace.c       |   2 +
 3 files changed, 141 insertions(+), 153 deletions(-)

Comments

Linus Torvalds April 16, 2025, 3:04 p.m. UTC | #1 
On Wed, 16 Apr 2025 at 06:17, Christian Brauner <brauner@kernel.org> wrote:
>
> Let's inline all low-level helpers and use likely()/unlikely() to help
> the compiler along.

Hmm. This looks like it might be a mistake - code generation will
probably not be great, because you still end up calling a real
function for some of the cases (ie from_kuid() in the actual real
translation case), so all the register allocation etc issues with
having a function call largely do remain.

Yes, it inlines things into generic_permission(), and that will avoid
the function call overhead for the common case (good), but it also
does make for bigger code generation. And it doesn't actually *change*
the code - it ends up doing all the same accesses, just the
instruction flow is slightly different.

So I think you'd actually be better off with *just* the IOP_USERNS
patch, and only inlining *that* fast-path case, and keep the other
cases out-of-line.

IOW - instead of only checking IOP_USERNS only in i_user_ns() and
making it return 'init_user_ns' without doing the pointer following, I
think you should make just our *existing* inlined i_uid_into_vfsuid()
helper be the minimal inlined wrapper around just the IOP_USERNS
logic.

Because right now the problem with i_uid_into_vfsuid() is two-fold

 - it does that pointer chasing by calling i_user_ns(inode)

 - it then calls make_vfsuid() which does lots of pointless extra work
in the common case

and I think both should be fixed.

Btw, make_vfsuid() itself is kind of odd. It does:

        if (idmap == &nop_mnt_idmap)
                return VFSUIDT_INIT(kuid);
        if (idmap == &invalid_mnt_idmap)
                return INVALID_VFSUID;
        if (initial_idmapping(fs_userns))
                uid = __kuid_val(kuid);
        else
                uid = from_kuid(fs_userns, kuid);
        if (uid == (uid_t)-1)
                return INVALID_VFSUID;
        return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));

and honestly, that looks just horrendous for the actual simple cases.
I think it's a historical accident, but the

                return VFSUIDT_INIT(kuid);

and the

                uid = __kuid_val(kuid);
        ....
        return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));

things are actually the same exact "no mapping" code for the case we
care about most. We shouldn't even do that

        if (uid == (uid_t)-1)
                return INVALID_VFSUID;

case at all for that case, because the no-mapping situation is that
INVALID_VFSUID *is* (uid_t)-1, so all of this is entirely pointless.

So I think the inlined fast-case should be that

        if (idmap == &nop_mnt_idmap || initial_idmapping(fs_userns))
                return VFSUIDT_INIT(kuid);

code, and then the 'initial_idmapping()' thing should check the
IOP_USERNS bit explicitly, and never use the i_user_ns() helper at all
etc.

That case should then be "likely()", and the rest can remain out-of-line.

IOW: instead of inlining all the helpers, just make the *one* helper
that we already have (and is already a trivial inline function) be
much more targeted, and make that fast-case much more explicit.

Hmm?

                   Linus
diff mbox series

Patch

diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
index 5c7e1db8fef8..ba1f752b6fa7 100644
--- a/fs/mnt_idmapping.c
+++ b/fs/mnt_idmapping.c
@@ -10,13 +10,6 @@ 
 
 #include "internal.h"
 
-/*
- * Outside of this file vfs{g,u}id_t are always created from k{g,u}id_t,
- * never from raw values. These are just internal helpers.
- */
-#define VFSUIDT_INIT_RAW(val) (vfsuid_t){ val }
-#define VFSGIDT_INIT_RAW(val) (vfsgid_t){ val }
-
 /*
  * Carries the initial idmapping of 0:0:4294967295 which is an identity
  * mapping. This means that {g,u}id 0 is mapped to {g,u}id 0, {g,u}id 1 is
@@ -36,144 +29,6 @@  struct mnt_idmap invalid_mnt_idmap = {
 };
 EXPORT_SYMBOL_GPL(invalid_mnt_idmap);
 
-/**
- * make_vfsuid - map a filesystem kuid according to an idmapping
- * @idmap: the mount's idmapping
- * @fs_userns: the filesystem's idmapping
- * @kuid : kuid to be mapped
- *
- * Take a @kuid and remap it from @fs_userns into @idmap. Use this
- * function when preparing a @kuid to be reported to userspace.
- *
- * If initial_idmapping() determines that this is not an idmapped mount
- * we can simply return @kuid unchanged.
- * If initial_idmapping() tells us that the filesystem is not mounted with an
- * idmapping we know the value of @kuid won't change when calling
- * from_kuid() so we can simply retrieve the value via __kuid_val()
- * directly.
- *
- * Return: @kuid mapped according to @idmap.
- * If @kuid has no mapping in either @idmap or @fs_userns INVALID_UID is
- * returned.
- */
-
-vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
-		     struct user_namespace *fs_userns,
-		     kuid_t kuid)
-{
-	uid_t uid;
-
-	if (idmap == &nop_mnt_idmap)
-		return VFSUIDT_INIT(kuid);
-	if (idmap == &invalid_mnt_idmap)
-		return INVALID_VFSUID;
-	if (initial_idmapping(fs_userns))
-		uid = __kuid_val(kuid);
-	else
-		uid = from_kuid(fs_userns, kuid);
-	if (uid == (uid_t)-1)
-		return INVALID_VFSUID;
-	return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));
-}
-EXPORT_SYMBOL_GPL(make_vfsuid);
-
-/**
- * make_vfsgid - map a filesystem kgid according to an idmapping
- * @idmap: the mount's idmapping
- * @fs_userns: the filesystem's idmapping
- * @kgid : kgid to be mapped
- *
- * Take a @kgid and remap it from @fs_userns into @idmap. Use this
- * function when preparing a @kgid to be reported to userspace.
- *
- * If initial_idmapping() determines that this is not an idmapped mount
- * we can simply return @kgid unchanged.
- * If initial_idmapping() tells us that the filesystem is not mounted with an
- * idmapping we know the value of @kgid won't change when calling
- * from_kgid() so we can simply retrieve the value via __kgid_val()
- * directly.
- *
- * Return: @kgid mapped according to @idmap.
- * If @kgid has no mapping in either @idmap or @fs_userns INVALID_GID is
- * returned.
- */
-vfsgid_t make_vfsgid(struct mnt_idmap *idmap,
-		     struct user_namespace *fs_userns, kgid_t kgid)
-{
-	gid_t gid;
-
-	if (idmap == &nop_mnt_idmap)
-		return VFSGIDT_INIT(kgid);
-	if (idmap == &invalid_mnt_idmap)
-		return INVALID_VFSGID;
-	if (initial_idmapping(fs_userns))
-		gid = __kgid_val(kgid);
-	else
-		gid = from_kgid(fs_userns, kgid);
-	if (gid == (gid_t)-1)
-		return INVALID_VFSGID;
-	return VFSGIDT_INIT_RAW(map_id_down(&idmap->gid_map, gid));
-}
-EXPORT_SYMBOL_GPL(make_vfsgid);
-
-/**
- * from_vfsuid - map a vfsuid into the filesystem idmapping
- * @idmap: the mount's idmapping
- * @fs_userns: the filesystem's idmapping
- * @vfsuid : vfsuid to be mapped
- *
- * Map @vfsuid into the filesystem idmapping. This function has to be used in
- * order to e.g. write @vfsuid to inode->i_uid.
- *
- * Return: @vfsuid mapped into the filesystem idmapping
- */
-kuid_t from_vfsuid(struct mnt_idmap *idmap,
-		   struct user_namespace *fs_userns, vfsuid_t vfsuid)
-{
-	uid_t uid;
-
-	if (idmap == &nop_mnt_idmap)
-		return AS_KUIDT(vfsuid);
-	if (idmap == &invalid_mnt_idmap)
-		return INVALID_UID;
-	uid = map_id_up(&idmap->uid_map, __vfsuid_val(vfsuid));
-	if (uid == (uid_t)-1)
-		return INVALID_UID;
-	if (initial_idmapping(fs_userns))
-		return KUIDT_INIT(uid);
-	return make_kuid(fs_userns, uid);
-}
-EXPORT_SYMBOL_GPL(from_vfsuid);
-
-/**
- * from_vfsgid - map a vfsgid into the filesystem idmapping
- * @idmap: the mount's idmapping
- * @fs_userns: the filesystem's idmapping
- * @vfsgid : vfsgid to be mapped
- *
- * Map @vfsgid into the filesystem idmapping. This function has to be used in
- * order to e.g. write @vfsgid to inode->i_gid.
- *
- * Return: @vfsgid mapped into the filesystem idmapping
- */
-kgid_t from_vfsgid(struct mnt_idmap *idmap,
-		   struct user_namespace *fs_userns, vfsgid_t vfsgid)
-{
-	gid_t gid;
-
-	if (idmap == &nop_mnt_idmap)
-		return AS_KGIDT(vfsgid);
-	if (idmap == &invalid_mnt_idmap)
-		return INVALID_GID;
-	gid = map_id_up(&idmap->gid_map, __vfsgid_val(vfsgid));
-	if (gid == (gid_t)-1)
-		return INVALID_GID;
-	if (initial_idmapping(fs_userns))
-		return KGIDT_INIT(gid);
-	return make_kgid(fs_userns, gid);
-}
-EXPORT_SYMBOL_GPL(from_vfsgid);
-
 #ifdef CONFIG_MULTIUSER
 /**
  * vfsgid_in_group_p() - check whether a vfsuid matches the caller's groups
diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
index 4410672c2828..e6fb905c39bd 100644
--- a/include/linux/mnt_idmapping.h
+++ b/include/linux/mnt_idmapping.h
@@ -140,22 +140,153 @@  static inline bool vfsgid_eq_kgid(vfsgid_t vfsgid, kgid_t kgid)
 #define AS_KUIDT(val) (kuid_t){ __vfsuid_val(val) }
 #define AS_KGIDT(val) (kgid_t){ __vfsgid_val(val) }
 
+/*
+ * Outside of this file vfs{g,u}id_t are always created from k{g,u}id_t,
+ * never from raw values. These are just internal helpers.
+ */
+#define VFSUIDT_INIT_RAW(val) (vfsuid_t){ val }
+#define VFSGIDT_INIT_RAW(val) (vfsgid_t){ val }
+
 int vfsgid_in_group_p(vfsgid_t vfsgid);
 
 struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
 void mnt_idmap_put(struct mnt_idmap *idmap);
 
-vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
-		     struct user_namespace *fs_userns, kuid_t kuid);
+/**
+ * make_vfsuid - map a filesystem kuid according to an idmapping
+ * @idmap: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kuid : kuid to be mapped
+ *
+ * Take a @kuid and remap it from @fs_userns into @idmap. Use this
+ * function when preparing a @kuid to be reported to userspace.
+ *
+ * If initial_idmapping() determines that this is not an idmapped mount
+ * we can simply return @kuid unchanged.
+ * If initial_idmapping() tells us that the filesystem is not mounted with an
+ * idmapping we know the value of @kuid won't change when calling
+ * from_kuid() so we can simply retrieve the value via __kuid_val()
+ * directly.
+ *
+ * Return: @kuid mapped according to @idmap.
+ * If @kuid has no mapping in either @idmap or @fs_userns INVALID_UID is
+ * returned.
+ */
+static inline vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
+				   struct user_namespace *fs_userns,
+				   kuid_t kuid)
+{
+	uid_t uid;
+
+	if (likely(idmap == &nop_mnt_idmap))
+		return VFSUIDT_INIT(kuid);
+	if (unlikely(idmap == &invalid_mnt_idmap))
+		return INVALID_VFSUID;
+	if (likely(initial_idmapping(fs_userns)))
+		uid = __kuid_val(kuid);
+	else
+		uid = from_kuid(fs_userns, kuid);
+	if (unlikely(uid == (uid_t)-1))
+		return INVALID_VFSUID;
+	return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));
+}
 
-vfsgid_t make_vfsgid(struct mnt_idmap *idmap,
-		     struct user_namespace *fs_userns, kgid_t kgid);
+/**
+ * make_vfsgid - map a filesystem kgid according to an idmapping
+ * @idmap: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kgid : kgid to be mapped
+ *
+ * Take a @kgid and remap it from @fs_userns into @idmap. Use this
+ * function when preparing a @kgid to be reported to userspace.
+ *
+ * If initial_idmapping() determines that this is not an idmapped mount
+ * we can simply return @kgid unchanged.
+ * If initial_idmapping() tells us that the filesystem is not mounted with an
+ * idmapping we know the value of @kgid won't change when calling
+ * from_kgid() so we can simply retrieve the value via __kgid_val()
+ * directly.
+ *
+ * Return: @kgid mapped according to @idmap.
+ * If @kgid has no mapping in either @idmap or @fs_userns INVALID_GID is
+ * returned.
+ */
+static inline vfsgid_t make_vfsgid(struct mnt_idmap *idmap,
+				   struct user_namespace *fs_userns,
+				   kgid_t kgid)
+{
+	gid_t gid;
+
+	if (likely(idmap == &nop_mnt_idmap))
+		return VFSGIDT_INIT(kgid);
+	if (unlikely(idmap == &invalid_mnt_idmap))
+		return INVALID_VFSGID;
+	if (likely(initial_idmapping(fs_userns)))
+		gid = __kgid_val(kgid);
+	else
+		gid = from_kgid(fs_userns, kgid);
+	if (unlikely(gid == (gid_t)-1))
+		return INVALID_VFSGID;
+	return VFSGIDT_INIT_RAW(map_id_down(&idmap->gid_map, gid));
+}
 
-kuid_t from_vfsuid(struct mnt_idmap *idmap,
-		   struct user_namespace *fs_userns, vfsuid_t vfsuid);
+/**
+ * from_vfsuid - map a vfsuid into the filesystem idmapping
+ * @idmap: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @vfsuid : vfsuid to be mapped
+ *
+ * Map @vfsuid into the filesystem idmapping. This function has to be used in
+ * order to e.g. write @vfsuid to inode->i_uid.
+ *
+ * Return: @vfsuid mapped into the filesystem idmapping
+ */
+static inline kuid_t from_vfsuid(struct mnt_idmap *idmap,
+				 struct user_namespace *fs_userns,
+				 vfsuid_t vfsuid)
+{
+	uid_t uid;
+
+	if (likely(idmap == &nop_mnt_idmap))
+		return AS_KUIDT(vfsuid);
+	if (unlikely(idmap == &invalid_mnt_idmap))
+		return INVALID_UID;
+	uid = map_id_up(&idmap->uid_map, __vfsuid_val(vfsuid));
+	if (unlikely(uid == (uid_t)-1))
+		return INVALID_UID;
+	if (likely(initial_idmapping(fs_userns)))
+		return KUIDT_INIT(uid);
+	return make_kuid(fs_userns, uid);
+}
 
-kgid_t from_vfsgid(struct mnt_idmap *idmap,
-		   struct user_namespace *fs_userns, vfsgid_t vfsgid);
+/**
+ * from_vfsgid - map a vfsgid into the filesystem idmapping
+ * @idmap: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @vfsgid : vfsgid to be mapped
+ *
+ * Map @vfsgid into the filesystem idmapping. This function has to be used in
+ * order to e.g. write @vfsgid to inode->i_gid.
+ *
+ * Return: @vfsgid mapped into the filesystem idmapping
+ */
+static inline kgid_t from_vfsgid(struct mnt_idmap *idmap,
+				 struct user_namespace *fs_userns,
+				 vfsgid_t vfsgid)
+{
+	gid_t gid;
+
+	if (likely(idmap == &nop_mnt_idmap))
+		return AS_KGIDT(vfsgid);
+	if (unlikely(idmap == &invalid_mnt_idmap))
+		return INVALID_GID;
+	gid = map_id_up(&idmap->gid_map, __vfsgid_val(vfsgid));
+	if (unlikely(gid == (gid_t)-1))
+		return INVALID_GID;
+	if (likely(initial_idmapping(fs_userns)))
+		return KGIDT_INIT(gid);
+	return make_kgid(fs_userns, gid);
+}
 
 /**
  * vfsuid_has_fsmapping - check whether a vfsuid maps into the filesystem
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 682f40d5632d..693c62587571 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -336,6 +336,7 @@  u32 map_id_down(struct uid_gid_map *map, u32 id)
 {
 	return map_id_range_down(map, id, 1);
 }
+EXPORT_SYMBOL_GPL(map_id_down);
 
 /*
  * map_id_up_base - Find idmap via binary search in static extent array.
@@ -402,6 +403,7 @@  u32 map_id_up(struct uid_gid_map *map, u32 id)
 {
 	return map_id_range_up(map, id, 1);
 }
+EXPORT_SYMBOL_GPL(map_id_up);
 
 /**
  *	make_kuid - Map a user-namespace uid pair into a kuid.