On 2/21/2020 3:09 PM, KP Singh wrote: > Thanks Casey, > > I appreciate your quick responses! > > On 21-Feb 14:31, Casey Schaufler wrote: >> On 2/21/2020 11:41 AM, KP Singh wrote: >>> On 21-Feb 11:19, Casey Schaufler wrote: >>>> On 2/20/2020 9:52 AM, KP Singh wrote: >>>>> From: KP Singh <kpsingh@xxxxxxxxxx> >>>> Again, apologies for the CC list trimming. >>>> >>>>> # v3 -> v4 >>>>> >>>>> https://lkml.org/lkml/2020/1/23/515 >>>>> >>>>> * Moved away from allocating a separate security_hook_heads and adding a >>>>> new special case for arch_prepare_bpf_trampoline to using BPF fexit >>>>> trampolines called from the right place in the LSM hook and toggled by >>>>> static keys based on the discussion in: >>>>> >>>>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@xxxxxxxxxxxxxx/ >>>>> >>>>> * Since the code does not deal with security_hook_heads anymore, it goes >>>>> from "being a BPF LSM" to "BPF program attachment to LSM hooks". > [...] > >>>> likely harmful. >>> We will be happy to document each of the macros in detail. Do note a >>> few things here: >>> >>> * There is really nothing magical about them though, >> >> +#define LSM_HOOK_void(NAME, ...) \ >> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {} >> + >> +#include <linux/lsm_hook_names.h> >> +#undef LSM_HOOK >> >> I haven't seen anything this ... novel ... in a very long time. > This is not "novel", it's a fairly common pattern followed in tracing: > > For example, the TRACE_INCLUDE macro which is used for tracepoints: > > include/trace/define_trace.h > > and used in: > > * include/trace/bpf_probe.h > > https://github.com/torvalds/linux/blob/master/include/trace/bpf_probe.h#L110 > > * include/trace/perf.h > > https://github.com/torvalds/linux/blob/master/include/trace/perf.h#L90 > > * include/trace/trace_events.h > > https://github.com/torvalds/linux/blob/master/include/trace/trace_events.h#L402 I can't say I care for that, either, and it's a simpler case. >> I see why you want to do this, but you're tying the two sets >> of code together unnaturally. When (not if) the two sets diverge >> you're going to be introducing another clever way to deal with > I don't fully understand what "two sets diverge means" here. All the > BPF headers need is the name, return type and the args. This is the > same information which is needed by the call_{int, void}_hooks and the > LSM declarataions (i.e. security_hook_heads and > security_list_options). As you've noticed, not all the interfaces can use call_{int,void}_hooks. If you've been following the stacking efforts, you'll see that increasing. At some point I anticipate a BPF hook that needs different information than the LSM hook. That's been discussed, too. Asserting that it will never happen does not make me comfortable. >> the special case. >> >> It's not that I don't understand what you're doing. It's that >> I don't like what you're doing. Explanation doesn't make me like >> it better. > As I have previously said, we will be happy to (and have already) > updated our approach based on the consensus we arrive at here. Not to put too fine a point on it, but I have raised the same objection - that you should use the infrastructure as it is - each time. I do not see consensus, I see you plowing ahead with the direction you've chosen in spite of the significant objection. > The > best outcome would be to not sacrifice performance as the LSM hooks > are called from various performance critical code-paths. Then help me tune the infrastructure to be better in those cases. > It would be great to know the maintainers' (BPF and Security) > perspective on this as well. Many eyes and all that, but the BPF maintainers haven't been working with the LSM infrastructure and won't be familiar with its quirks.
