On 2/23/2020 2:08 PM, Alexei Starovoitov wrote: > On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote: >> If I'm understanding this correctly, there are two issues: >> >> 1- BPF needs to be run last due to fexit trampolines (?) > no. > The placement of nop call can be anywhere. > BPF trampoline is automagically converting nop call into a sequence > of directly invoked BPF programs. > No link list traversals and no indirect calls in run-time. Then why the insistence that it be last? >> 2- BPF hooks don't know what may be attached at any given time, so >> ALL LSM hooks need to be universally hooked. THIS turns out to create >> a measurable performance problem in that the cost of the indirect call >> on the (mostly/usually) empty BPF policy is too high. > also no. Um, then why not use the infrastructure as is? >> So, trying to avoid the indirect calls is, as you say, an optimization, >> but it might be a needed one due to the other limitations. > I'm convinced that avoiding the cost of retpoline in critical path is a > requirement for any new infrastructure in the kernel. Sorry, I haven't gotten that memo. > Not only for security, but for any new infra. The LSM infrastructure ain't new. > Networking stack converted all such places to conditional calls. > In BPF land we converted indirect calls to direct jumps and direct calls. > It took two years to do so. Adding new indirect calls is not an option. > I'm eagerly waiting for Peter's static_call patches to land to convert > a lot more indirect calls. May be existing LSMs will take advantage > of static_call patches too, but static_call is not an option for BPF. > That's why we introduced BPF trampoline in the last kernel release. Sorry, but I don't see how BPF is so overwhelmingly special. >> b) Would there actually be a global benefit to using the static keys >> optimization for other LSMs? > Yes. Just compiling with CONFIG_SECURITY adds "if (hlist_empty)" check > for every hook. Err, no, it doesn't. It does an hlish_for_each_entry(), which may be the equivalent on an empty list, but let's not go around spreading misinformation. > Some of those hooks are in critical path. This load+cmp > can be avoided with static_key optimization. I think it's worth doing. I admit to being unfamiliar with the static_key implementation, but if it would work for a list of hooks rather than a singe hook, I'm all ears. >> If static keys are justified for KRSI > I really like that KRSI costs absolutely zero when it's not enabled. And I dislike that there's security module specific code in security.c, security.h and/or lsm_hooks.h. KRSI *is not that special*. > Attaching BPF prog to one hook preserves zero cost for all other hooks. > And when one hook is BPF powered it's using direct call instead of > super expensive retpoline. I'm not objecting to the good it does for KRSI. I am *strongly* objecting to special casing KRSI. > Overall this patch set looks good to me. There was a minor issue with prog > accounting. I expect only that bit to be fixed in v5.
