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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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