On Thu, 31 Oct 2024 at 12:34, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> So I'd rather start with just the cheap inode-only "ACL is clearly not
> there" check, and later if we find that the ACL_NOT_CACHED case is
> problematic do we look at that.
Actually, if I switch the tests around so that I do the permission bit
check first, it becomes very natural to just check IS_POSIXACL() at
the end (where we're about to go to the slow case, which will be
touching i_sb anyway).
Plus I can actually improve code generation by not shifting the mode
bits down into the low bits, but instead spreading the requested
permission bits around.
The "spread bits around" becomes a simple constant multiplication with
just three bits set, and the compiler will actually generate much
better code (you can do it with two consecutive 'lea' instructions).
The expression for this ends up looking a bit like line noise, so a
comment explaining each step is a good idea.
IOW, here's a rewritten patch that does it that way around, and thus
deals with IS_POSIXACL() very naturally and seems to generate quite
good code for me.
Of course, while I actually tested the previous version after sending
it out, this new version is entirely untested. Again.
Linus
fs/namei.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/fs/namei.c b/fs/namei.c
index 4a4a22a08ac2..80f9a14ca9a5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -326,6 +326,25 @@ static int check_acl(struct mnt_idmap *idmap,
return -EAGAIN;
}
+/*
+ * Very quick optimistic "we know we have no ACL's" check.
+ *
+ * Note that this is purely for ACL_TYPE_ACCESS, and purely
+ * for the "we have cached that there are no ACLs" case.
+ *
+ * If this returns true, we know there are no ACLs. But if
+ * it returns false, we might still not have ACLs (it could
+ * be the is_uncached_acl() case).
+ */
+static inline bool no_acl_inode(struct inode *inode)
+{
+#ifdef CONFIG_FS_POSIX_ACL
+ return likely(!READ_ONCE(inode->i_acl));
+#else
+ return true;
+#endif
+}
+
/**
* acl_permission_check - perform basic UNIX permission checking
* @idmap: idmap of the mount the inode was found from
@@ -348,6 +367,28 @@ static int acl_permission_check(struct mnt_idmap *idmap,
unsigned int mode = inode->i_mode;
vfsuid_t vfsuid;
+ /*
+ * Common cheap case: everybody has the requested
+ * rights, and there are no ACLs to check. No need
+ * to do any owner/group checks in that case.
+ *
+ * - 'mask&7' is the requested permission bit set
+ * - multiplying by 0111 spreads them out to all of ugo
+ * - '& ~mode' looks for missing inode permission bits
+ * - the '!' is for "no missing permissions"
+ *
+ * After that, we just need to check that there are no
+ * ACL's on the inode - do the 'IS_POSIXACL()' check last
+ * because it will dereference the ->i_sb pointer and we
+ * want to avoid that if at all possible.
+ */
+ if (!((mask & 7)*0111 & ~mode)) {
+ if (no_acl_inode(inode))
+ return 0;
+ if (!IS_POSIXACL(inode))
+ return 0;
+ }
+
/* Are we the owner? If so, ACL's don't matter */
vfsuid = i_uid_into_vfsuid(idmap, inode);
if (likely(vfsuid_eq_kuid(vfsuid, current_fsuid()))) {
