Messages in this thread | | | From | KP Singh <> | Date | Mon, 24 Feb 2020 18:23:09 +0100 | Subject | Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs |
| |
Hi Kees,
Thanks for the feedback!
On 21-Feb 20:22, Kees Cook wrote: > On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote: > > On 2/20/2020 9:52 AM, KP Singh wrote: > > > From: KP Singh <kpsingh@google.com> > > > > Sorry about the heavy list pruning - the original set > > blows thunderbird up. > > (I've added some people back; I had to dig this thread back out of lkml > since I didn't get a direct copy...) > > > > The BPF LSM programs are implemented as fexit trampolines to avoid the > > > overhead of retpolines. These programs cannot be attached to security_* > > > wrappers as there are quite a few security_* functions that do more than > > > just calling the LSM callbacks. > > > > > > This was discussed on the lists in: > > > > > > https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266 > > > > > > Adding a NOP callback after all the static LSM callbacks are called has > > > the following benefits: > > > > > > - The BPF programs run at the right stage of the security_* wrappers. > > > - They run after all the static LSM hooks allowed the operation, > > > therefore cannot allow an action that was already denied. > > > > I still say that the special call-out to BPF is unnecessary. > > I remain unconvinced by the arguments. You aren't doing anything > > so special that the general mechanism won't work. > > If I'm understanding this correctly, there are two issues: > > 1- BPF needs to be run last due to fexit trampolines (?) > > 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. > > "1" can be solved a lot of ways, and doesn't seem to be a debated part > of this series. > > "2" is interesting -- it creates a performance problem for EVERYONE that > builds in this kernel feature, regardless of them using it. Excepting > SELinux, "traditional" LSMs tends to be relatively sparse in their hooking: > > $ grep '^ struct hlist_head' include/linux/lsm_hooks.h | wc -l > 230 > $ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \ > do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done > apparmor 68 > loadpin 3 > lockdown 1 > safesetid 2 > selinux 202 > smack 108 > tomoyo 28 > yama 4 > > 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. > > To me, some questions present themselves: > > a) What, exactly, are the performance characteristics of: > "before" > "with indirect calls" > "with static keys optimization"
Good suggestion!
I will do some analysis and come back with the numbers.
> > b) Would there actually be a global benefit to using the static keys > optimization for other LSMs? (Especially given that they're already > sparsely populated and policy likely determines utility -- all the > LSMs would just turn ON all their static keys or turn off ALL their > static keys depending on having policy loaded.)
As Alexei mentioned, we can use the patches for static calls after they are merged:
https://lore.kernel.org/lkml/8bc857824f82462a296a8a3c4913a11a7f801e74.1547073843.git.jpoimboe@redhat.com/
to make the framework better (as a separate series) especially given that we are unsure how they work with BPF.
- KP
> > If static keys are justified for KRSI (by "a") then it seems the approach > here should stand. If "b" is also true, then we need an additional > series to apply this optimization for the other LSMs (but that seems > distinctly separate from THIS series). > > -- > Kees Cook
| |