On Thu, Apr 11, 2024 at 3:12 AM KP Singh <kpsingh@xxxxxxxxxx> wrote:
> > On 11 Apr 2024, at 02:38, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Feb 7, 2024 KP Singh <kpsingh@xxxxxxxxxx> wrote:
> >>
> >> LSM hooks are currently invoked from a linked list as indirect calls
> >> which are invoked using retpolines as a mitigation for speculative
> >> attacks (Branch History / Target injection) and add extra overhead which
> >> is especially bad in kernel hot paths:
...
> > Beyond that, let's find a way to use static calls in the LSM hooks
> > which don't use the call_{int,void}_hook() macros. If we're going to do
> > this to help close some attack vectors, let's make sure we do the
> > conversion everywhere.
>
> This is surely doable, We can unroll the loop individually in these separate hooks. It would need separate
>
> LSM_LOOP_UNROLL(__CALL_STATIC_xfrm_state_pol_flow_match, x, xp file)
>
> Would you be okay if we do it in a follow up series? These are special hooks and I don't want to introduce any subtle logical bugs when fixing potential speculative side channels (Which could be fixed with retpolines, proper flushing at privilege changes etc).
I'm okay if you want to do it in a separate patch, but I would like to
see it included in the same patchset. The good news is that recent
commits have significantly reduced the number of cases where we aren't
using the macros.
> >> @@ -846,29 +906,41 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> >> * call_int_hook:
> >> * This is a hook that returns a value.
> >> */
> >> +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
> >> +do { \
> >> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> >
> > I'm not a fan of the likely()/unlikely() style markings/macros in cases
> > like this as it can vary tremendously. Drop the likely()/unlikely()
> > checks and just do a static_call().
> >
>
> These are actually not the the classical likely, unlikely macros which are just hints to the compiler:
>
> #define likely(x) __builtin_expect(!!(x), 1)
> #define unlikely(x) __builtin_expect(!!(x), 0
>
>
> but a part of the static keys API which generates jump tables and the code generated depends on the (default state, likelyhood). It could have been named better, all we need is to have a jump table so that we can optimize this extra branch in hotpaths, in one direction.
>
> https://www.kernel.org/doc/Documentation/static-keys.txt
>
>
> If you want I can put this behind a macro:
>
>
> #define LSM_HOOK_ACTIVE(HOOK, NUM) static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM)
>
> the static_branch_likely / static_branch_unlikey actually does not matter much here, because without this we have a conditional branch and an extra load.
Fair enough, leave it as-is. Thanks for the explanation.
--
paul-moore.com
