linux-ext4 - Re: generic_permission() optimization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGudoHGxr5gYb0JqPqF_J0MoSAb_qqoF4gaJMEdOhp51yobbLw@mail.gmail.com>
Date: Sat, 12 Apr 2025 18:26:09 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Christian Brauner <brauner@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>, Jan Kara <jack@...e.cz>, 
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: generic_permission() optimization

On Thu, Nov 7, 2024 at 11:49 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Thu, 7 Nov 2024 at 12:22, Mateusz Guzik <mjguzik@...il.com> wrote:
> >
> > How about filesystems maintaing a flag: IOP_EVERYONECANTRAREVERSE?
>
> It's actually just easier if a filesystem just does
>
>         cache_no_acl(inode);
>
> in its read-inode function if it knows it has no ACL's.
>
> Some filesystems already do that, eg btrfs has
>
>         /*
>          * try to precache a NULL acl entry for files that don't have
>          * any xattrs or acls
>          */
>         ....
>         if (!maybe_acls)
>                 cache_no_acl(inode);
>
> in btrfs_read_locked_inode(). If that 'maybe' is just reliable enough,
> that's all it takes.
>
> I tried to do the same thing for ext4, and failed miserably, but
> that's probably because my logic for "maybe_acls" was broken since I'm
> not familiar enough with ext4 at that level, and I made it do just
>
>         /* Initialize the "no ACL's" state for the simple cases */
>         if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) && !ei->i_file_acl)
>                 cache_no_acl(inode);
>
> which doesn't seem to be a strong enough text.
>

[ roping in ext4 people ]

I plopped your snippet towards the end of __ext4_iget:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4008551bbb2d..34189d85e363 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5071,7 +5071,12 @@ struct inode *__ext4_iget(struct super_block
*sb, unsigned long ino,
                goto bad_inode;
        }

+       /* Initialize the "no ACL's" state for the simple cases */
+       if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) && !ei->i_file_acl)
+               cache_no_acl(inode);
+
        brelse(iloc.bh);

bpftrace over a kernel build shows almost everything is sorted out:
 bpftrace -e 'kprobe:security_inode_permission { @[((struct inode
*)arg0)->i_acl] = count(); }'

@[0xffffffffffffffff]: 23810
@[0x0]: 65984202

That's just shy of 66 mln calls where the acls were explicitly set to
empty, compared to less than 24k where it was the default "uncached"
state.

So indeed *something* is missed, but the patch does cover almost everything.

Perhaps the ext4 guys would chime in and see it through? :)

The context is speeding path lookup by avoiding some of the branches
during permission checking.

-- 
Mateusz Guzik <mjguzik gmail.com>