On Mon, Aug 24, 2020 at 04:33:44PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 24, 2020 at 04:09:09PM +0200, Brendan Jackman wrote:
>
> > > > Why this trick with a switch statement? The table of static call is defined
> > > > at compile time. The number of hook callbacks that will be defined is
> > > > unknown at that time, and the table cannot be resized at runtime. Static
> > > > calls do not define a conditional execution for a non-void function, so the
> > > > executed slots must be non-empty. With this use of the table and the
> > > > switch, it is possible to jump directly to the first used slot and execute
> > > > all of the slots after. This essentially makes the entry point of the table
> > > > dynamic. Instead, it would also be possible to start from 0 and break after
> > > > the final populated slot, but that would require an additional conditional
> > > > after each slot.
> > >
> > > Instead of just "NOP", having the static branches perform a jump would
> > > solve this pretty cleanly, yes? Something like:
> > >
> > > ret = DEFAULT_RET;
> > >
> > > ret = A(args); <--- direct call, no retpoline
> > > if ret != 0:
> > > goto out;
> > >
> > > ret = B(args); <--- direct call, no retpoline
> > > if ret != 0:
> > > goto out;
> > >
> > > goto out;
> > > if ret != 0:
> > > goto out;
> > >
> > > out:
> > > return ret;
> >
> > Hmm yeah that's a cool idea. This would either need to be implemented
> > with custom code-modification logic for the LSM hooks, or we'd need to
> > think of a way to express it in a sensible addition to the static_call
> > API. I do wonder if the latter could take the form of a generic system
> > for arrays of static calls.
>
> So you basically want something like:
>
> if (A[0] && (ret = static_call(A[0])(...)))
> return ret;
>
> if (A[1] && (ret = static_call(A[1])(...)))
> return ret;
>
> ....
>
> return ret;
>
> Right? The problem with static_call_cond() is that we don't know what to
> do with the return value when !func, which is why it's limited to void
> return type.
>
> You can however construct something like the above with a combination of
> static_branch() and static_call() though. It'll not be pretty, but it
> ought to work:
>
> if (static_branch_likely(A[0].key)) {
> ret = static_call(A[0].call)(...);
> if (ret)
> return ret;
> }
>
> ...
>
> return ret;
>
Right. That's actually exactly what Paul's first implementation
looked like for call_int_hook. But we thought the switch thing was
easier to understand.
>
> > It would also need to handle the fact that IIUC at the moment the last
> > static_call may be a tail call, so we'd be patching an existing jump
> > into a jump to a different target, I don't know if we can do that
> > atomically.
>
> Of course we can, the static_call() series supports tail-calls just
> fine. In fact, patching jumps is far easier, it was patching call that
> was the real problem because it mucks about with the stack.
>
OK great. I had a vague apprehension that we could only patch to or from
a NOP.
