Re: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls

From: Casey Schaufler <casey@schaufler-ca.com>
To: KP Singh <kpsingh@kernel.org>, Kees Cook <keescook@chromium.org>
Cc: linux-security-module@vger.kernel.org, bpf@vger.kernel.org,
	ast@kernel.org, daniel@iogearbox.net, jackmanb@google.com,
	renauld@google.com, paul@paul-moore.com, song@kernel.org,
	revest@chromium.org, casey@schaufler-ca.com
Subject: Re: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
Date: Mon, 6 Feb 2023 08:29:15 -0800	[thread overview]
Message-ID: <db1fed31-0283-5401-cf55-d18a98ca33ae@schaufler-ca.com> (raw)
In-Reply-To: <CACYkzJ75nYnunhcAaE-20p9YHLzVynUEAA+uK1tmGeOWA83MjA@mail.gmail.com>

On 2/6/2023 5:04 AM, KP Singh wrote:
> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote:
>>> The indirect calls are not really needed as one knows the addresses of
> [...]
>
>>> +/*
>>> + * Define static calls and static keys for each LSM hook.
>>> + */
>>> +
>>> +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...)                  \
>>> +     DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM),             \
>>> +                             *((RET(*)(__VA_ARGS__))NULL));          \
>>> +     DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ENABLED_KEY(NAME, NUM));
>> Hm, another place where we would benefit from having separated logic for
>> "is it built?" and "is it enabled by default?" and we could use
>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use
>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be
>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're
>> trying to optimize for having LSMs, I think we should default to inline
>> calls. (The machine code in the commit log seems to indicate that they
>> are out of line -- it uses jumps.)
>>
> I should have added it in the commit description, actually we are
> optimizing for "hot paths are less likely to have LSM hooks enabled"
> (eg. socket_sendmsg).

How did you come to that conclusion? Where is there a correlation between
"hot path" and "less likely to be enabled"? 

>  But I do see that there are LSMs that have these
> enabled. Maybe we can put this behind a config option, possibly
> depending on CONFIG_EXPERT?

Help me, as the maintainer of one of those LSMs, understand why that would
be a good idea.