Re: generic_permission() optimization

["Theodore Ts'o" <tytso@mit.edu>]

On Sat, Apr 12, 2025 at 06:26:09PM +0200, Mateusz Guzik wrote:
> > 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

Linus, what problems did you run into?

> >         /* 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.

Linus, were you running into false positives or false negatives?  You
said "failed miserably", which seems to imply that you ran into cases
where cache_no_acl() got called when the inode actually had an ACL ---
e.g., a false positive.

That's different from what Mateuz is reporting which is that most
inodes w/o ACL were getting cache_no_acl(), but some cases
cache_no_acl() was't getting called when it should.  That is, a flase
negative:

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

So the test which we're talking about:

> 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);
> 

tests if the inode does not have an extended attribute.  Posix ACL's
are stored as xattr's --- but if there are any extended attributes (or
in some cases, inline data), in order to authoratatively determine
whether there is an ACL or not will require iterating over all of the
extended attributes.  This can be rather heavyweight, so doing it
unconditionally any time we do an iget() is probably not warranted.

Still, if this works 99.99% of the time, given that most peple don't
enable inline_data, and most folks aren't setting extended attributes,
it should be fine.  Of course, given that SELinux's security ID's are
stored as extended attriutes, at that point this optimization won't
work.  But if you are using SELinux, you probably don't care about
performance anyway.  :-)

					- Ted