Re: Looking at profile data once again - avc lookup

From: Paul Moore <paul@paul-moore.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: Looking at profile data once again - avc lookup
Date: Sat, 28 Jan 2023 17:33:34 -0500	[thread overview]
Message-ID: <CAHC9VhR1jRM2K0757sNYS8VvSUxRWOKUJ1unbsZm9LOEM3Up6Q@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whR4KSGqfEodXMwOMdQBt+V2HHMyz6+CobiydnZE+Vq9Q@mail.gmail.com>

On Sat, Jan 28, 2023 at 4:15 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I happened to do my occasional "let's see what a profile for an
> empty kernel build is", which usually highlights some silly thing we
> do in pathname lookup (because most of the cost is just 'make' doing
> various string handling in user space, and doing variations of
> 'lstat()' to look at file timestamps).
>
> And, as always, avc_has_perm() and avc_has_perm_noaudit() are pretty
> high up there, together with selinux_inode_permission(). It just gets
> called a *lot*.
>
> Now, avc_has_perm_noaudit() should be inlined, but it turns out that
> it isn't because of some bad behavior of the unlikely path. That part
> looks fairly easy to massage away. I'm attaching a largely untested
> patch that makes the generated code look a bit better, in case anybody
> wants to look at it.

I'll take a look, although just a heads-up that I don't generally
merge patches into selinux/next at this point in the -rc cycle unless
they are bug fixes, or some other critical patch; it's likely this
will need to wait until after the upcoming merge window closes.

> But the real reason for this email is to query about 'selinux_state'.
>
> We pass that around as a pointer quite a bit, to the point where
> several function calls have to use stack space for arguments just
> because they have more than six arguments. And from what I can tell,
> it is *always* just a pointer to 'selinux_state', and never anything
> else.

We can, and should, look at those cases where the function parameters
start to spill into the stack space.

> This was all done five years ago by commit aa8e712cee93 ("selinux:
> wrap global selinux state") and I don't see the point. It never seems
> to have gone anywhere, there's no other state that I can find, and all
> it does is add overhead and complexity.
>
> So I'd like to undo that, and go back to the good old days when we
> didn't waste time and effort on passing a pointer that always has the
> same value as an argument.
>
> Comments? Is there some case I've missed?

You're correct in that selinux_state parameters currently always point
back to the single global instance, however there was, and still is, a
point to that patch ... although I will admit it is a long time
coming.  The purpose of this change was to help pave the way for
namespacing SELinux by easing development and helping to reduce
ongoing merge conflicts with the in-development patches.  For a
variety of reasons the namespacing work has languished a bit, but it
hasn't been forgotten and I expect it to gain importance once the LSM
stacking patches land (LSM stacking alone isn't going to solve all of
the problems that many are expecting, it will need to be combined with
LSM-specific namespacing).

-- 
paul-moore.com