On Mon, Feb 6, 2023 at 6:49 PM Song Liu [off-list ref] wrote:

On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler [off-list ref] wrote:

quoted

On 2/6/2023 5:04 AM, KP Singh wrote:

quoted

On Fri, Jan 20, 2023 at 5:36 AM Kees Cook [off-list ref] wrote:

quoted

On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote:

quoted

The indirect calls are not really needed as one knows the addresses of

[...]

quoted

quoted

+/*
+ * 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"?

I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on
hot path will give more performance overhead. In our use cases (Meta),
we are very careful with "small" performance hits. 0.25% is significant

+1 to everything Song said, I am not saying that one direction is
better than the other and for distros that have LSMs (like SELinux and
AppArmor enabled) it's okay to have this default to
static_branch_likely. On systems that will have just the BPF LSM
enabled, it's the opposite that is true, i.e. one would never add a
hook on a hotpath as the overheads are unacceptable, and when one does
add a hook, they are willing to add the extra overhead (this is
already much less compared to the indirect calls). I am okay with the
default being static_branch_likely if that's what the other LSM
maintainers prefer.

overhead; 1% overhead will not fly without very good reasons (Do we
have to do this? Are there any other alternatives?). If it is possible to
achieve similar security on a different hook, we will not enable the hook on
the hot path. For example, we may not enable socket_sendmsg, but try
to disallow opening such sockets instead.

quoted

quoted

 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.

IIUC, this is also from performance concerns. We would like to manage
the complexity at compile time for performance benefits.

Thanks,
Song