Messages in this thread | | | Subject | Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs | From | Casey Schaufler <> | Date | Mon, 24 Feb 2020 08:32:10 -0800 |
| |
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.
| |