[PATCH 0/2] two nits for path lookup

[PATCH 0/2] two nits for path lookup

[Mateusz Guzik]

since path looku is being looked at, two extra nits from me:

1. some trivial jump avoidance in inode_permission()

2. but more importantly avoiding a memory access which is most likely a
cache miss when descending into devcgroup_inode_permission()

the file seems to have no maintainer fwiw

anyhow I'm confident the way forward is to add IOP_FAST_MAY_EXEC (or
similar) to elide inode_permission() in the common case to begin with.
There are quite a few branches which straight up don't need execute.
On top of that btrfs has a permission hook only to check for MAY_WRITE,
which in case of path lookup is not set. With the above flag the call
will be avoided.

Mateusz Guzik (2):
  fs: touch up predicts in inode_permission()
  device_cgroup: avoid access to ->i_rdev in the common case in
    devcgroup_inode_permission()

 fs/namei.c                    | 10 +++++-----
 include/linux/device_cgroup.h |  7 ++++---
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH 1/2] fs: touch up predicts in inode_permission()

[Mateusz Guzik]

The routine only encounters errors when people try to access things they
can't, which is a negligible amount of calls.

The only questionable bit might be the pre-existing predict around
MAY_WRITE. Currently the routine is predominantly used for MAY_EXEC, so
this makes some sense.

I verified this straightens out the asm.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/namei.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index daebc307c1a3..cff69c12d6fd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -571,14 +571,14 @@ int inode_permission(struct mnt_idmap *idmap,
 	int retval;
 
 	retval = sb_permission(inode->i_sb, inode, mask);
-	if (retval)
+	if (unlikely(retval))
 		return retval;
 
 	if (unlikely(mask & MAY_WRITE)) {
 		/*
 		 * Nobody gets write access to an immutable file.
 		 */
-		if (IS_IMMUTABLE(inode))
+		if (unlikely(IS_IMMUTABLE(inode)))
 			return -EPERM;
 
 		/*
@@ -586,16 +586,16 @@ int inode_permission(struct mnt_idmap *idmap,
 		 * written back improperly if their true value is unknown
 		 * to the vfs.
 		 */
-		if (HAS_UNMAPPED_ID(idmap, inode))
+		if (unlikely(HAS_UNMAPPED_ID(idmap, inode)))
 			return -EACCES;
 	}
 
 	retval = do_inode_permission(idmap, inode, mask);
-	if (retval)
+	if (unlikely(retval))
 		return retval;
 
 	retval = devcgroup_inode_permission(inode, mask);
-	if (retval)
+	if (unlikely(retval))
 		return retval;
 
 	return security_inode_permission(inode, mask);
-- 
2.48.1

[PATCH 2/2] device_cgroup: avoid access to ->i_rdev in the common case in devcgroup_inode_permission()

[Mateusz Guzik]

The routine gets called for every path component during lookup.
->i_mode is going to be cached on account of permission checks, while
->i_rdev is an area which is most likely cache-cold.

gcc 14.2 is kind enough to emit one branch:
	movzwl (%rbx),%eax
	mov    %eax,%edx
	and    $0xb000,%dx
	cmp    $0x2000,%dx
	je     11bc <inode_permission+0xec>

This patch is lazy in that I don't know if the ->i_rdev branch makes
any sense with the newly added mode check upfront. I am not changing any
semantics here though.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 include/linux/device_cgroup.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/device_cgroup.h b/include/linux/device_cgroup.h
index d02f32b7514e..0864773a57e8 100644
--- a/include/linux/device_cgroup.h
+++ b/include/linux/device_cgroup.h
@@ -18,15 +18,16 @@ static inline int devcgroup_inode_permission(struct inode *inode, int mask)
 {
 	short type, access = 0;
 
+	if (likely(!S_ISBLK(inode->i_mode) && !S_ISCHR(inode->i_mode)))
+		return 0;
+
 	if (likely(!inode->i_rdev))
 		return 0;
 
 	if (S_ISBLK(inode->i_mode))
 		type = DEVCG_DEV_BLOCK;
-	else if (S_ISCHR(inode->i_mode))
+	else /* S_ISCHR by the test above */
 		type = DEVCG_DEV_CHAR;
-	else
-		return 0;
 
 	if (mask & MAY_WRITE)
 		access |= DEVCG_ACC_WRITE;
-- 
2.48.1

Re: [PATCH 0/2] two nits for path lookup

[Mateusz Guzik]

On Thu, Apr 17, 2025 at 12:16 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> since path looku is being looked at, two extra nits from me:
>
> 1. some trivial jump avoidance in inode_permission()
>
> 2. but more importantly avoiding a memory access which is most likely a
> cache miss when descending into devcgroup_inode_permission()
>
> the file seems to have no maintainer fwiw
>
> anyhow I'm confident the way forward is to add IOP_FAST_MAY_EXEC (or
> similar) to elide inode_permission() in the common case to begin with.
> There are quite a few branches which straight up don't need execute.

.. the bit would be set if everyone has the x perm on the inode, there
are no acls and the thing is a directory

The perm to check being MAY_EXEC elides the MAY_WRITE check in sb_permission().
The bit only showing up on directories means this is not a device,
eliding  devcgroup_inode_permission()
The bit being set means there is no need to separately check for the
mode and acls.

I have hooks in the same spot as security_* callbacks for setattr and
setacl + a CONFIG_DEBUG_VFS-guarded runtime check that the bit is only
set if there are indeed no acls and the mode grants x for everyone. It
also handles races against setattr/getacl. I just need to clean this
up + do more testing.

> On top of that btrfs has a permission hook only to check for MAY_WRITE,
> which in case of path lookup is not set. With the above flag the call
> will be avoided.
>
> Mateusz Guzik (2):
>   fs: touch up predicts in inode_permission()
>   device_cgroup: avoid access to ->i_rdev in the common case in
>     devcgroup_inode_permission()
>
>  fs/namei.c                    | 10 +++++-----
>  include/linux/device_cgroup.h |  7 ++++---
>  2 files changed, 9 insertions(+), 8 deletions(-)
>
> --
> 2.48.1
>


-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 0/2] two nits for path lookup

[Linus Torvalds]

On Wed, 16 Apr 2025 at 15:16, Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> since path looku is being looked at, two extra nits from me:

Ack, both look sane to me.

             Linus

Re: [PATCH 0/2] two nits for path lookup

[Christian Brauner]

On Thu, Apr 17, 2025 at 12:16:24AM +0200, Mateusz Guzik wrote:
> since path looku is being looked at, two extra nits from me:
> 
> 1. some trivial jump avoidance in inode_permission()
> 
> 2. but more importantly avoiding a memory access which is most likely a
> cache miss when descending into devcgroup_inode_permission()

Serge did maintain this for a while. But honestly it is an absolute
legacy eyesore from the cgroup v1 days. Somehow it was decided that
device permission management is a good fit for cgroups. Idk, I have
ranted about this in other places at length. No use warming that back
up.

They later decided to reimplement device access management as a
dedicated bpf program type. That imho is another bad design decision.

What should've happend is that device access management should've just
been implemented through the bpf-LSM infrastructure. That way all this
stuff would've gone through security_inode_permission() instead of us
having to have two separate calls in inode_permission().

I would love to kill this call. And cgroup v1 is deprecated and systemd
has dropped any support for it last year as well.

> 
> the file seems to have no maintainer fwiw
> 
> anyhow I'm confident the way forward is to add IOP_FAST_MAY_EXEC (or
> similar) to elide inode_permission() in the common case to begin with.
> There are quite a few branches which straight up don't need execute.

Yes.

> On top of that btrfs has a permission hook only to check for MAY_WRITE,
> which in case of path lookup is not set. With the above flag the call
> will be avoided.
> 
> Mateusz Guzik (2):
>   fs: touch up predicts in inode_permission()
>   device_cgroup: avoid access to ->i_rdev in the common case in
>     devcgroup_inode_permission()
> 
>  fs/namei.c                    | 10 +++++-----
>  include/linux/device_cgroup.h |  7 ++++---
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> -- 
> 2.48.1
> 

Re: [PATCH 0/2] two nits for path lookup

[Christian Brauner]

On Thu, 17 Apr 2025 00:16:24 +0200, Mateusz Guzik wrote:
> since path looku is being looked at, two extra nits from me:
> 
> 1. some trivial jump avoidance in inode_permission()
> 
> 2. but more importantly avoiding a memory access which is most likely a
> cache miss when descending into devcgroup_inode_permission()
> 
> [...]

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[1/2] fs: touch up predicts in inode_permission()
      https://git.kernel.org/vfs/vfs/c/305a4329d07c
[2/2] device_cgroup: avoid access to ->i_rdev in the common case in devcgroup_inode_permission()
      https://git.kernel.org/vfs/vfs/c/328ba0291442