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 > enabled LSM callbacks at boot time and only the order can possibly > change at boot time with the lsm= kernel command line parameter. > > An array of static calls is defined per LSM hook and the static calls > are updated at boot time once the order has been determined. > > A static key guards whether an LSM static call is enabled or not, > without this static key, for LSM hooks that return an int, the presence > of the hook that returns a default value can create side-effects which > has resulted in bugs [1]. I think this patch has too many logic changes in it. There are basically two things going on here: - replace list with unrolled calls - replace calls with static calls I see why it was merged, since some of the logic that would be added for step 1 would be immediate replaced, but I think it might make things a bit more clear. There is likely a few intermediate steps here too, to rename things, etc. > There are some hooks that don't use the call_int_hook and > call_void_hook. These hooks are updated to use a new macro called > security_for_each_hook where the lsm_callback is directly invoked as an > indirect call. Currently, there are no performance sensitive hooks that > use the security_for_each_hook macro. However, if, some performance > sensitive hooks are discovered, these can be updated to use > static calls with loop unrolling as well using a custom macro. > > [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@xxxxxxxxxx/ > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx> > --- > include/linux/lsm_hooks.h | 83 +++++++++++++-- > security/security.c | 216 ++++++++++++++++++++++++-------------- > 2 files changed, 211 insertions(+), 88 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 0a5ba81f7367..c82d15a4ef50 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -28,6 +28,26 @@ > #include <linux/security.h> > #include <linux/init.h> > #include <linux/rculist.h> > +#include <linux/static_call.h> > +#include <linux/unroll.h> > +#include <linux/jump_label.h> > + > +/* Include the generated MAX_LSM_COUNT */ > +#include <generated/lsm_count.h> > + > +#define SECURITY_HOOK_ENABLED_KEY(HOOK, IDX) security_enabled_key_##HOOK##_##IDX > + > +/* > + * Identifier for the LSM static calls. > + * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h > + * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT > + */ > +#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX > + > +/* > + * Call the macro M for each LSM hook MAX_LSM_COUNT times. > + */ > +#define LSM_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) I think this should be: #define LSM_UNROLL(M, ...) do { \ UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__); \ } while (0) or maybe UNROLL needs the do/while. > > /** > * union security_list_options - Linux Security Module hook function list > @@ -1657,21 +1677,48 @@ union security_list_options { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > #include "lsm_hook_defs.h" > #undef LSM_HOOK > + void *lsm_callback; > }; > > -struct security_hook_heads { > - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME; > - #include "lsm_hook_defs.h" > +/* > + * @key: static call key as defined by STATIC_CALL_KEY > + * @trampoline: static call trampoline as defined by STATIC_CALL_TRAMP > + * @hl: The security_hook_list as initialized by the owning LSM. > + * @enabled_key: Enabled when the static call has an LSM hook associated. > + */ > +struct lsm_static_call { > + struct static_call_key *key; > + void *trampoline; > + struct security_hook_list *hl; > + struct static_key *enabled_key; > +}; > + > +/* > + * Table of the static calls for each LSM hook. > + * Once the LSMs are initialized, their callbacks will be copied to these > + * tables such that the calls are filled backwards (from last to first). > + * This way, we can jump directly to the first used static call, and execute > + * all of them after. This essentially makes the entry point > + * dynamic to adapt the number of static calls to the number of callbacks. > + */ > +struct lsm_static_calls_table { > + #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > + struct lsm_static_call NAME[MAX_LSM_COUNT]; > + #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > } __randomize_layout; > > /* > * Security module hook list structure. > * For use with generic list macros for common operations. > + * > + * struct security_hook_list - Contents of a cacheable, mappable object. > + * @scalls: The beginning of the array of static calls assigned to this hook. > + * @hook: The callback for the hook. > + * @lsm: The name of the lsm that owns this hook. > */ > struct security_hook_list { > - struct hlist_node list; > - struct hlist_head *head; > + struct lsm_static_call *scalls; > union security_list_options hook; > const char *lsm; > } __randomize_layout; > @@ -1701,10 +1748,12 @@ struct lsm_blob_sizes { > * care of the common case and reduces the amount of > * text involved. > */ > -#define LSM_HOOK_INIT(HEAD, HOOK) \ > - { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } } > +#define LSM_HOOK_INIT(NAME, CALLBACK) \ > + { \ > + .scalls = static_calls_table.NAME, \ > + .hook = { .NAME = CALLBACK } \ > + } > > -extern struct security_hook_heads security_hook_heads; > extern char *lsm_names; > > extern void security_add_hooks(struct security_hook_list *hooks, int count, > @@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; > static inline void security_delete_hooks(struct security_hook_list *hooks, > int count) Hey Paul, can we get rid of CONFIG_SECURITY_SELINUX_DISABLE yet? It's been deprecated for years.... > { > - int i; > + struct lsm_static_call *scalls; > + int i, j; > + > + for (i = 0; i < count; i++) { > + scalls = hooks[i].scalls; > + for (j = 0; j < MAX_LSM_COUNT; j++) { > + if (scalls[j].hl != &hooks[i]) > + continue; > > - for (i = 0; i < count; i++) > - hlist_del_rcu(&hooks[i].list); > + static_key_disable(scalls[j].enabled_key); > + __static_call_update(scalls[j].key, > + scalls[j].trampoline, NULL); > + scalls[j].hl = NULL; > + } > + } > } > #endif /* CONFIG_SECURITY_SELINUX_DISABLE */ > > @@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, > #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ > > extern int lsm_inode_alloc(struct inode *inode); > +extern struct lsm_static_calls_table static_calls_table __ro_after_init; > > #endif /* ! __LINUX_LSM_HOOKS_H */ > diff --git a/security/security.c b/security/security.c > index d1571900a8c7..e54d5ba187d1 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -29,6 +29,8 @@ > #include <linux/string.h> > #include <linux/msg.h> > #include <net/flow.h> > +#include <linux/static_call.h> > +#include <linux/jump_label.h> > > #define MAX_LSM_EVM_XATTR 2 > > @@ -74,7 +76,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { > [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality", > }; > > -struct security_hook_heads security_hook_heads __lsm_ro_after_init; > static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain); > > static struct kmem_cache *lsm_file_cache; > @@ -93,6 +94,43 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > static __initdata struct lsm_info **ordered_lsms; > static __initdata struct lsm_info *exclusive; > > +/* > + * 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.) > [...] > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + } Same here -- I would expect this to be static_branch_likely() or we'll get out-of-line branches. Also, IMO, this should be: do { if (...) static_call(...); } while (0) > +#define call_void_hook(FUNC, ...) \ > + do { \ > + LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \ > } while (0) With the do/while in LSM_UNROLL, this is more readable. > > -#define call_int_hook(FUNC, IRC, ...) ({ \ > - int RC = IRC; \ > - do { \ > - struct security_hook_list *P; \ > - \ > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > - RC = P->hook.FUNC(__VA_ARGS__); \ > - if (RC != 0) \ > - break; \ > - } \ > - } while (0); \ > - RC; \ > -}) > +#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \ > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + if (R != 0) \ > + goto out; \ > + } I would expect the label to be a passed argument, but maybe since it never changes, it's fine as-is? And again, I'd expect a do/while wrapper, and for it to be s_b_likely. > + > +#define call_int_hook(FUNC, IRC, ...) \ > + ({ \ > + __label__ out; \ > + int RC = IRC; \ > + do { \ > + LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \ > + \ > + } while (0); \ Then this becomes: ({ int RC = IRC; LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__); out: RC; }) > +#define security_for_each_hook(scall, NAME, expression) \ > + for (scall = static_calls_table.NAME; \ > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \ > + if (!static_key_enabled(scall->enabled_key)) \ > + continue; \ > + (expression); \ > + } Why isn't this using static_branch_enabled()? I would expect this to be: #define security_for_each_hook(scall, NAME) \ for (scall = static_calls_table.NAME; \ scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ if (static_branch_likely(scall->enabled_key)) > > /* Security operations */ > > @@ -859,7 +924,7 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz) > > int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > { > - struct security_hook_list *hp; > + struct lsm_static_call *scall; > int cap_sys_admin = 1; > int rc; > > @@ -870,13 +935,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > * agree that it should be set it will. If any module > * thinks it should not be set it won't. > */ > - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { > - rc = hp->hook.vm_enough_memory(mm, pages); > + security_for_each_hook(scall, vm_enough_memory, ({ > + rc = scall->hl->hook.vm_enough_memory(mm, pages); > if (rc <= 0) { > cap_sys_admin = 0; > break; > } > - } > + })); Then these replacements don't look weird. This would just be: security_for_each_hook(scall, vm_enough_memory) { rc = scall->hl->hook.vm_enough_memory(mm, pages); if (rc <= 0) { cap_sys_admin = 0; break; } } I'm excited to have this. The speed improvements are pretty nice. -- Kees Cook
