Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

On 25/03/2021 00.40, Sami Tolvanen wrote:
> On Wed, Mar 24, 2021 at 3:53 PM Rasmus Villemoes
> <linux@xxxxxxxxxxxxxxxxxx> wrote:
>>
>> On 24/03/2021 23.34, Sami Tolvanen wrote:
>>> On Wed, Mar 24, 2021 at 2:51 PM Rasmus Villemoes
>>> <linux@xxxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> On 24/03/2021 18.33, Peter Zijlstra wrote:
>>>>> On Wed, Mar 24, 2021 at 05:45:52PM +0100, Rasmus Villemoes wrote:
>>>>>> Sorry, I think I misread the code. The static calls are indeed
>>>>>> initialized with a function with the right prototype. Try adding
>>>>>> "preempt=full" on the command line so that we exercise these lines
>>>>>>
>>>>>>                static_call_update(cond_resched,
>>>>>> (typeof(&__cond_resched)) __static_call_return0);
>>>>>>                 static_call_update(might_resched,
>>>>>> (typeof(&__cond_resched)) __static_call_return0);
>>>>>>
>>>>>> I would expect that to blow up, since we end up calling a long (*)(void)
>>>>>> function using a function pointer of type int (*)(void).
>>>>>
>>>>> Note that on x86 there won't actually be any calling of function
>>>>> pointers. See what arch/x86/kernel/static_call.c does :-)
>>>>
>>>> I know, but so far x86 is the only one with HAVE_STATIC_CALL, so for
>>>> arm64 which is where CFI seems to be targeted initially, static_calls
>>>> are function pointers. And unless CFI ignores the return type, I'd
>>>> really expect the above to fail.
>>>
>>> I think you're correct, this would trip CFI without HAVE_STATIC_CALL.
>>> However, arm64 also doesn't support PREEMPT_DYNAMIC at the moment, so
>>> this isn't currently a problem there.
>>
>> Well, there's PREEMPT_DYNAMIC and HAVE_PREEMPT_DYNAMIC. The former
>> doesn't depend on the latter (and the latter does depend on
>> HAVE_STATIC_CALL, so effectively not for anything but x86). You should
>> be able to select both PREEMPT_DYNAMIC and CFI_CLANG, and test if
>> booting with preempt=full does give the fireworks one expects.
> 
> Actually, it looks like I can't select PREEMPT_DYNAMIC> and tweaking Kconfig

Ah, there's no prompt on the "bool" line, so it doesn't show up. That
seems to be a mistake, since there's an elaborate help text which says

          The runtime overhead is negligible with
HAVE_STATIC_CALL_INLINE enabled
          but if runtime patching is not available for the specific
architecture
          then the potential overhead should be considered.

So it seems that it was meant to be "you can enable this if you really
want".

to force enable it on arm64 results in a build error
> ("implicit declaration of function 'static_call_mod'").

Seems to be an omission in the last !HAVE_STATIC_CALL branch in
static_call_types.h, and there's also no
EXPORT_STATIC_CALL_TRAMP{,_GPL} in static_call.h for that case.

Rasmus