On Tue, Jun 13, 2023 at 11:43 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Wed, Jun 7, 2023 at 10:48 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > On Mon, Feb 6, 2023 at 7:51 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > > On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote: > > > > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > On 2/6/2023 9:48 AM, Song Liu wrote: > > > > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > > > > > >> On 2/6/2023 5:04 AM, KP Singh wrote: > > > > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > > > > >>>> 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 > > > > > >>> [...] > > > > > >>> > > > > > >>>>> +/* > > > > > >>>>> + * 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.) > > > > > >>>> > > > > > >>> I should have added it in the commit description, actually we are > > > > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > > > > > >>> (eg. socket_sendmsg). > > > > > >> How did you come to that conclusion? Where is there a correlation between > > > > > >> "hot path" and "less likely to be enabled"? > > > > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > > > > > hot path will give more performance overhead. In our use cases (Meta), > > > > > > we are very careful with "small" performance hits. 0.25% is significant > > > > > > overhead; 1% overhead will not fly without very good reasons (Do we > > > > > > have to do this? Are there any other alternatives?). If it is possible to > > > > > > achieve similar security on a different hook, we will not enable the hook on > > > > > > the hot path. For example, we may not enable socket_sendmsg, but try > > > > > > to disallow opening such sockets instead. > > > > > > > > > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > > > > > If you're talking strictly about BPF you need to say that. I'm all for > > > > > performance improvement. But as I've said before, it should be for all > > > > > the security modules, not just BPF. > > > > > > > > It's a trade off that will work differently for different LSMs and > > > > distros (based on the LSM they chose) and this the config option. I > > > > even suggested this be behind CONFIG_EXPERT (which is basically says > > > > this: > > > > > > > > "This option allows certain base kernel options and settings > > > > to be disabled or tweaked. This is for specialized > > > > environments which can tolerate a "non-standard" kernel. > > > > Only use this if you really know what you are doing." > > > > > > Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros > > > tied to a new CONFIG seems like it can give us a reasonable knob for > > > in-line vs out-of-line calls. > > > > Coming back to this after a while as I finally got time to work on > > this. (work/personal downtime). > > > > I am changing it to DEFINE_STATIC_KEY_TRUE in this patch and > > DEFINE_STATIC_KEY_MAYBE in a subsequent one and guarded by a config > > called CONFIG_SECURITY_HOOK_LIKELY. I am letting it default to yes, > > but distros can change it depending on their choice of LSM and > > performance characteristics. > > I'm still more curious about the correctness/isolation aspect that I > mused about back in Jan/Feb on your original posting. Thanks, I put some clarifications there. I will post a v2 soon (TM). Although beware I have upcoming downtime due to a surgery next week. > > -- > paul-moore.com
