Re: [PATCH v2 09/11] static_call: Make NULL static calls consistent

Christophe Leroy
2023-03-22T15:05:03Z

Le 22/03/2023 à 05:00, Josh Poimboeuf a écrit :
> NULL static calls have inconsistent behavior. With HAVE_STATIC_CALL=y
> they're a NOP, but with HAVE_STATIC_CALL=n they go boom.
>
> That's guaranteed to cause subtle bugs. Make the behavior consistent by
> making NULL static calls a NOP with HAVE_STATIC_CALL=n.
>
> This is probably easier than doing the reverse (making NULL static calls
> panic with HAVE_STATIC_CALL=y). And it seems to match the current use
> cases better: there are several call sites which rely on the NOP
> behavior, whereas no call sites rely on the crashing behavior.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---

> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..27c095c7fc96 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -89,7 +89,7 @@ static void __ref __static_call_transform(void *insn, enum insn_type type,
>
> case JCC:
> if (!func) {
> - func = __static_call_return;
> + func = __static_call_return; //FIXME use __static_call_nop()?
> if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
> func = x86_return_thunk;
> }
> @@ -139,33 +139,35 @@ static void __static_call_validate(u8 *insn, bool tail, bool tramp)
> BUG();
> }
>
> -static inline enum insn_type __sc_insn(bool null, bool tail)
> +static inline enum insn_type __sc_insn(bool nop, bool tail)
> {
> /*
> * Encode the following table without branches:
> *
> - * tail null insn
> + * tail nop insn
> * -----+-------+------
> * 0 | 0 | CALL
> * 0 | 1 | NOP
> * 1 | 0 | JMP
> * 1 | 1 | RET
> */
> - return 2*tail + null;
> + return 2*tail + nop;
> }
>
> void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
> {
> + bool nop = (func == __static_call_nop);
> +

Would be clearer to call it 'is_nop', wouldn't it ?

> mutex_lock(&text_mutex);
>
> if (tramp) {
> __static_call_validate(tramp, true, true);
> - __static_call_transform(tramp, __sc_insn(!func, true), func, false);
> + __static_call_transform(tramp, __sc_insn(nop, true), func, false);
> }
>
> if (IS_ENABLED(CONFIG_HAVE_STATIC_CALL_INLINE) && site) {
> __static_call_validate(site, tail, false);
> - __static_call_transform(site, __sc_insn(!func, tail), func, false);
> + __static_call_transform(site, __sc_insn(nop, tail), func, false);
> }
>
> mutex_unlock(&text_mutex);

Christophe