[PATCH] static_call: fix function type mismatch

[PATCH] static_call: fix function type mismatch

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The __static_call_return0() function is declared to return a 'long',
while it aliases a couple of functions that all return 'int'. When
building with 'make W=1', gcc warns about this:

kernel/sched/core.c:5420:37: error: cast between incompatible function types from 'long int (*)(void)' to 'int (*)(void)' [-Werror=cast-function-type]
 5420 |   static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);

Change the function to return 'int' as well, but remove the cast to
ensure we get a warning if any of the types ever change.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/static_call.h | 6 +++---
 kernel/sched/core.c         | 6 +++---
 kernel/static_call.c        | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/static_call.h b/include/linux/static_call.h
index 85ecc789f4ff..3fc2975906ad 100644
--- a/include/linux/static_call.h
+++ b/include/linux/static_call.h
@@ -148,7 +148,7 @@ extern void __static_call_update(struct static_call_key *key, void *tramp, void
 extern int static_call_mod_init(struct module *mod);
 extern int static_call_text_reserved(void *start, void *end);
 
-extern long __static_call_return0(void);
+extern int __static_call_return0(void);
 
 #define __DEFINE_STATIC_CALL(name, _func, _func_init)			\
 	DECLARE_STATIC_CALL(name, _func);				\
@@ -221,7 +221,7 @@ static inline int static_call_text_reserved(void *start, void *end)
 	return 0;
 }
 
-static inline long __static_call_return0(void)
+static inline int __static_call_return0(void)
 {
 	return 0;
 }
@@ -247,7 +247,7 @@ struct static_call_key {
 	void *func;
 };
 
-static inline long __static_call_return0(void)
+static inline int __static_call_return0(void)
 {
 	return 0;
 }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3a36f0b0742e..d22c609b9484 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5399,7 +5399,7 @@ static void sched_dynamic_update(int mode)
 	switch (mode) {
 	case preempt_dynamic_none:
 		static_call_update(cond_resched, __cond_resched);
-		static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);
+		static_call_update(might_resched, __static_call_return0);
 		static_call_update(preempt_schedule, (typeof(&preempt_schedule)) NULL);
 		static_call_update(preempt_schedule_notrace, (typeof(&preempt_schedule_notrace)) NULL);
 		static_call_update(irqentry_exit_cond_resched, (typeof(&irqentry_exit_cond_resched)) NULL);
@@ -5416,8 +5416,8 @@ static void sched_dynamic_update(int mode)
 		break;
 
 	case preempt_dynamic_full:
-		static_call_update(cond_resched, (typeof(&__cond_resched)) __static_call_return0);
-		static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);
+		static_call_update(cond_resched, __static_call_return0);
+		static_call_update(might_resched, __static_call_return0);
 		static_call_update(preempt_schedule, __preempt_schedule_func);
 		static_call_update(preempt_schedule_notrace, __preempt_schedule_notrace_func);
 		static_call_update(irqentry_exit_cond_resched, irqentry_exit_cond_resched);
diff --git a/kernel/static_call.c b/kernel/static_call.c
index 6906c6ec4c97..11aa4bcee315 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -489,7 +489,7 @@ int __init static_call_init(void)
 }
 early_initcall(static_call_init);
 
-long __static_call_return0(void)
+int __static_call_return0(void)
 {
 	return 0;
 }
-- 
2.29.2

Re: [PATCH] static_call: fix function type mismatch

[Steven Rostedt]

On Mon, 22 Mar 2021 18:06:37 +0100
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> The __static_call_return0() function is declared to return a 'long',
> while it aliases a couple of functions that all return 'int'. When
> building with 'make W=1', gcc warns about this:
> 
> kernel/sched/core.c:5420:37: error: cast between incompatible function types from 'long int (*)(void)' to 'int (*)(void)' [-Werror=cast-function-type]
>  5420 |   static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);
> 
> Change the function to return 'int' as well, but remove the cast to
> ensure we get a warning if any of the types ever change.

I think the answer is the other way around. That is, to make the functions
it references return long instead. __static_call_return0 is part of the
dynamic call infrastructure. Perhaps it is currently only used by functions
that return int, but what happens when it is used for a function that
returns a pointer?

-- Steve


> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

On Mon, Mar 22, 2021 at 03:32:14PM -0400, Steven Rostedt wrote:
> On Mon, 22 Mar 2021 18:06:37 +0100
> Arnd Bergmann <arnd@kernel.org> wrote:
> 
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The __static_call_return0() function is declared to return a 'long',
> > while it aliases a couple of functions that all return 'int'. When
> > building with 'make W=1', gcc warns about this:
> > 
> > kernel/sched/core.c:5420:37: error: cast between incompatible function types from 'long int (*)(void)' to 'int (*)(void)' [-Werror=cast-function-type]
> >  5420 |   static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);
> > 
> > Change the function to return 'int' as well, but remove the cast to
> > ensure we get a warning if any of the types ever change.
> 
> I think the answer is the other way around. That is, to make the functions
> it references return long instead. __static_call_return0 is part of the
> dynamic call infrastructure. Perhaps it is currently only used by functions
> that return int, but what happens when it is used for a function that
> returns a pointer?

Steve is correct. Also, why is that warning correct? On x86 we return in
RAX, and using int will simply not inspect the upper 32 bits there.

And I'm fairly sure I had a pointer user somewhere recently.

Re: [PATCH] static_call: fix function type mismatch

[Arnd Bergmann]

On Mon, Mar 22, 2021 at 9:47 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Mar 22, 2021 at 03:32:14PM -0400, Steven Rostedt wrote:
> > On Mon, 22 Mar 2021 18:06:37 +0100
> > Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The __static_call_return0() function is declared to return a 'long',
> > > while it aliases a couple of functions that all return 'int'. When
> > > building with 'make W=1', gcc warns about this:
> > >
> > > kernel/sched/core.c:5420:37: error: cast between incompatible function types from 'long int (*)(void)' to 'int (*)(void)' [-Werror=cast-function-type]
> > >  5420 |   static_call_update(might_resched, (typeof(&__cond_resched)) __static_call_return0);
> > >
> > > Change the function to return 'int' as well, but remove the cast to
> > > ensure we get a warning if any of the types ever change.
> >
> > I think the answer is the other way around. That is, to make the functions
> > it references return long instead. __static_call_return0 is part of the
> > dynamic call infrastructure. Perhaps it is currently only used by functions
> > that return int, but what happens when it is used for a function that
> > returns a pointer?

I've done a little testing on the replacement patch now, will send in a bit.

> Steve is correct. Also, why is that warning correct? On x86 we return in
> RAX, and using int will simply not inspect the upper 32 bits there.

I think the code works correctly on all architectures we support because
both 'int' and 'long' are returned in a register with any unused bits cleared.
It is however undefined behavior in C because 'int' and 'long' are not
compatible types, and the calling conventions don't have to allow this.

> And I'm fairly sure I had a pointer user somewhere recently.

I've only tested my series with 5.12-rc so far, but don't get any other
such warnings. Maybe it's in linux-next?

          Arnd

Re: [PATCH] static_call: fix function type mismatch

[Steven Rostedt]

On Mon, 22 Mar 2021 22:18:17 +0100
Arnd Bergmann <arnd@kernel.org> wrote:

> I think the code works correctly on all architectures we support because
> both 'int' and 'long' are returned in a register with any unused bits cleared.
> It is however undefined behavior in C because 'int' and 'long' are not
> compatible types, and the calling conventions don't have to allow this.

Static calls (and so do tracepoints) currently rely on these kind of
"undefined behavior" in C. This isn't the only UB that it relies on.

<shameless plug>
Perhaps this might make a good topic to talk about at the tool chain
microconference at Plumbers!
</shameless plug>

-- Steve

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

On Mon, Mar 22, 2021 at 10:18:17PM +0100, Arnd Bergmann wrote:

> > Steve is correct. Also, why is that warning correct? On x86 we return in
> > RAX, and using int will simply not inspect the upper 32 bits there.
> 
> I think the code works correctly on all architectures we support because
> both 'int' and 'long' are returned in a register with any unused bits cleared.
> It is however undefined behavior in C because 'int' and 'long' are not
> compatible types, and the calling conventions don't have to allow this.

Then please kill the warning, it's bullshit.

> > And I'm fairly sure I had a pointer user somewhere recently.
> 
> I've only tested my series with 5.12-rc so far, but don't get any other
> such warnings. Maybe it's in linux-next?

No, it's in Linus' tree, see commit:

  c8e2fe13d1d1 ("x86/perf: Use RET0 as default for guest_get_msrs to handle "no PMU" case")

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

On Mon, Mar 22, 2021 at 05:29:21PM -0400, Steven Rostedt wrote:
> On Mon, 22 Mar 2021 22:18:17 +0100
> Arnd Bergmann <arnd@kernel.org> wrote:
> 
> > I think the code works correctly on all architectures we support because
> > both 'int' and 'long' are returned in a register with any unused bits cleared.
> > It is however undefined behavior in C because 'int' and 'long' are not
> > compatible types, and the calling conventions don't have to allow this.
> 
> Static calls (and so do tracepoints) currently rely on these kind of
> "undefined behavior" in C. This isn't the only UB that it relies on.

Right, most of the kernel lives in UB. That's what we have -fwrapv
-fno-strict-aliassing and lots of other bits for, to 'fix' the stupid C
standard.

This is one more of them, so just ignore the warning and make it go
away:

 -Wno-cast-function-type

seems to be the magic knob.

Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

On 23/03/2021 08.47, Peter Zijlstra wrote:
> On Mon, Mar 22, 2021 at 05:29:21PM -0400, Steven Rostedt wrote:
>> On Mon, 22 Mar 2021 22:18:17 +0100
>> Arnd Bergmann <arnd@kernel.org> wrote:
>>
>>> I think the code works correctly on all architectures we support because
>>> both 'int' and 'long' are returned in a register with any unused bits cleared.
>>> It is however undefined behavior in C because 'int' and 'long' are not
>>> compatible types, and the calling conventions don't have to allow this.
>>
>> Static calls (and so do tracepoints) currently rely on these kind of
>> "undefined behavior" in C. This isn't the only UB that it relies on.
> 
> Right, most of the kernel lives in UB. That's what we have -fwrapv
> -fno-strict-aliassing and lots of other bits for, to 'fix' the stupid C
> standard.
> 
> This is one more of them, so just ignore the warning and make it go
> away:
> 
>  -Wno-cast-function-type
> 
> seems to be the magic knob.
> 

That can be done for now, but I think something has to be done if CFI is
to become a thing.

Sami, what happens if you try to boot a
CONFIG_CFI_CLANG+CONFIG_PREEMPT_DYNAMIC kernel?

Rasmus

Re: [PATCH] static_call: fix function type mismatch

[Sami Tolvanen]

On Wed, Mar 24, 2021 at 5:46 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 23/03/2021 08.47, Peter Zijlstra wrote:
> > On Mon, Mar 22, 2021 at 05:29:21PM -0400, Steven Rostedt wrote:
> >> On Mon, 22 Mar 2021 22:18:17 +0100
> >> Arnd Bergmann <arnd@kernel.org> wrote:
> >>
> >>> I think the code works correctly on all architectures we support because
> >>> both 'int' and 'long' are returned in a register with any unused bits cleared.
> >>> It is however undefined behavior in C because 'int' and 'long' are not
> >>> compatible types, and the calling conventions don't have to allow this.
> >>
> >> Static calls (and so do tracepoints) currently rely on these kind of
> >> "undefined behavior" in C. This isn't the only UB that it relies on.
> >
> > Right, most of the kernel lives in UB. That's what we have -fwrapv
> > -fno-strict-aliassing and lots of other bits for, to 'fix' the stupid C
> > standard.
> >
> > This is one more of them, so just ignore the warning and make it go
> > away:
> >
> >  -Wno-cast-function-type
> >
> > seems to be the magic knob.
> >
>
> That can be done for now, but I think something has to be done if CFI is
> to become a thing.
>
> Sami, what happens if you try to boot a
> CONFIG_CFI_CLANG+CONFIG_PREEMPT_DYNAMIC kernel?

Seems to boot just fine. CFI instrumentation is only for
compiler-generated indirect calls. Casting functions to different
types is fine as long as you don't end up calling them using an
incorrect pointer type.

Sami

Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

On 24/03/2021 17.01, Sami Tolvanen wrote:
> On Wed, Mar 24, 2021 at 5:46 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> On 23/03/2021 08.47, Peter Zijlstra wrote:
>>> On Mon, Mar 22, 2021 at 05:29:21PM -0400, Steven Rostedt wrote:
>>>> On Mon, 22 Mar 2021 22:18:17 +0100
>>>> Arnd Bergmann <arnd@kernel.org> wrote:
>>>>
>>>>> I think the code works correctly on all architectures we support because
>>>>> both 'int' and 'long' are returned in a register with any unused bits cleared.
>>>>> It is however undefined behavior in C because 'int' and 'long' are not
>>>>> compatible types, and the calling conventions don't have to allow this.
>>>>
>>>> Static calls (and so do tracepoints) currently rely on these kind of
>>>> "undefined behavior" in C. This isn't the only UB that it relies on.
>>>
>>> Right, most of the kernel lives in UB. That's what we have -fwrapv
>>> -fno-strict-aliassing and lots of other bits for, to 'fix' the stupid C
>>> standard.
>>>
>>> This is one more of them, so just ignore the warning and make it go
>>> away:
>>>
>>>  -Wno-cast-function-type
>>>
>>> seems to be the magic knob.
>>>
>>
>> That can be done for now, but I think something has to be done if CFI is
>> to become a thing.
>>
>> Sami, what happens if you try to boot a
>> CONFIG_CFI_CLANG+CONFIG_PREEMPT_DYNAMIC kernel?
> 
> Seems to boot just fine. CFI instrumentation is only for
> compiler-generated indirect calls. Casting functions to different
> types is fine as long as you don't end up calling them using an
> incorrect pointer type.

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).

Rasmus

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

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 :-)

But I think some of this code might need some __va_function() love when
combined with CFI.

But yes, this is why I think something like -fcdecl might be a good
idea, that ought to tell the compiler about the calling convention,
which ought to be enough for the compiler to figure out that this magic
really is ok.

Notable things we rely on:

 - caller cleanup of stack; the function caller sets up any stack
   arguments and is also responsible for cleanin up the stack once the
   function returns.

 - the return value is in a register.

Per the first we can call a function that has a partial (empty per
extremum) argument list. Per the second we can call a function with a
different return type as long as they all fit in the same register.

The calling of a 'long (*)()' function for a 'int (*)()' type then
becomes idential to something like: 'int x = (long)y', and that is
something C is perfectly fine with.

We then slightly push things with the other __static_call_return0()
usage in the kernel, where we basically end up with: 'void *x =
(long)y', which is something C really rather would have a cast on.

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

On Wed, Mar 24, 2021 at 06:33:39PM +0100, 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 :-)
> 
> But I think some of this code might need some __va_function() love when
> combined with CFI.
> 
> But yes, this is why I think something like -fcdecl might be a good
> idea, that ought to tell the compiler about the calling convention,
> which ought to be enough for the compiler to figure out that this magic
> really is ok.
> 
> Notable things we rely on:
> 
>  - caller cleanup of stack; the function caller sets up any stack
>    arguments and is also responsible for cleanin up the stack once the
>    function returns.

  - the arguments are pushed on stack right to left;

>  - the return value is in a register.
> 
> Per the first we can call a function that has a partial (empty per
> extremum) argument list. 

That extra constraint is required to make partial args work; as it
happens we only use empty args, and as such don't really care about this
atm.

> Per the second we can call a function with a
> different return type as long as they all fit in the same register.
> 
> The calling of a 'long (*)()' function for a 'int (*)()' type then
> becomes idential to something like: 'int x = (long)y', and that is
> something C is perfectly fine with.
> 
> We then slightly push things with the other __static_call_return0()
> usage in the kernel, where we basically end up with: 'void *x =
> (long)y', which is something C really rather would have a cast on.

Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

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.

> But I think some of this code might need some __va_function() love when
> combined with CFI.

Well, that was also my first thought when reading through the CFI
patches, I hoped that might salvage my
reduce-boilerplate-and-get-better-type-safety proposal for the
devm_*_action APIs [1]. But I don't think that would help at all;
storing __va_function(__static_call_return0) instead of
&__static_call_return0  (i.e., __static_call_return0 instead of
__static_call_return0.cfi_jt) doesn't help the call sites of that
static_call at all, neither address belongs to the range of jump table
entries corresponding to the prototype "int (*)(void)". So I think it
would be the static_call macro that would somehow need to grow a way to
suppress the cfi checking.

Rasmus

[1]
https://lore.kernel.org/lkml/20210309235917.2134565-1-linux@rasmusvillemoes.dk/

Re: [PATCH] static_call: fix function type mismatch

[Sami Tolvanen]

On Wed, Mar 24, 2021 at 2:51 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> 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.

Sami

Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

On 24/03/2021 23.34, Sami Tolvanen wrote:
> On Wed, Mar 24, 2021 at 2:51 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> 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.

Rasmus

Re: [PATCH] static_call: fix function type mismatch

[Sami Tolvanen]

On Wed, Mar 24, 2021 at 3:53 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 24/03/2021 23.34, Sami Tolvanen wrote:
> > On Wed, Mar 24, 2021 at 2:51 PM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> 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 to force enable it on arm64 results in a build error
("implicit declaration of function 'static_call_mod'").

Sami

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@rasmusvillemoes.dk> wrote:
>>
>> On 24/03/2021 23.34, Sami Tolvanen wrote:
>>> On Wed, Mar 24, 2021 at 2:51 PM Rasmus Villemoes
>>> <linux@rasmusvillemoes.dk> 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

Re: [PATCH] static_call: fix function type mismatch

[Peter Zijlstra]

On Thu, Mar 25, 2021 at 01:42:41AM +0100, Rasmus Villemoes wrote:
> > 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

Right, PREEMPT_DYNAMIC really hard relies on HAVE_STATIC_CALL

There's an implicit dependency in the select:

config PREEMPT
	...
	select PREEMPT_DYNAMIC if HAVE_PREEMPT_DYNAMIC

> > ("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.

That interface doesn't make sense for !HAVE_STATIC_CALL. It's impossible
to not export the function pointer itself but still call it for
!HAVE_STATIC_CALL.

Re: [PATCH] static_call: fix function type mismatch

[Ard Biesheuvel]

On Thu, 25 Mar 2021 at 08:43, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Mar 25, 2021 at 01:42:41AM +0100, Rasmus Villemoes wrote:
> > > 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
>
> Right, PREEMPT_DYNAMIC really hard relies on HAVE_STATIC_CALL
>
> There's an implicit dependency in the select:
>
> config PREEMPT
>         ...
>         select PREEMPT_DYNAMIC if HAVE_PREEMPT_DYNAMIC
>
> > > ("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.
>
> That interface doesn't make sense for !HAVE_STATIC_CALL. It's impossible
> to not export the function pointer itself but still call it for
> !HAVE_STATIC_CALL.

I proposed an implementation for the indirect static call variety for
arm64 here [0] but we haven't yet decided whether it is needed, given
that indirect calls are mostly fine on arm64 (modulo CFI of course)

Maybe this helps?


[0] https://lore.kernel.org/linux-arm-kernel/20201120082103.4840-1-ardb@kernel.org/

Re: [PATCH] static_call: fix function type mismatch

[Rasmus Villemoes]

On 25/03/2021 08.42, Peter Zijlstra wrote:
> On Thu, Mar 25, 2021 at 01:42:41AM +0100, Rasmus Villemoes wrote:
>>> 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
> 
> Right, PREEMPT_DYNAMIC really hard relies on HAVE_STATIC_CALL
> 
> There's an implicit dependency in the select:
> 
> config PREEMPT
> 	...
> 	select PREEMPT_DYNAMIC if HAVE_PREEMPT_DYNAMIC

That's not a dependency, that's a "force PREEMPT_DYNAMIC on", and users
on x86 can't deselect PREEMPT_DYNAMIC even if they wanted to.

Having a help text but not providing a prompt string is rather unusual.
What's the point of that paragraph I quoted above if PREEMPT_DYNAMIC is
not supposed to be settable by the developer?

>>> ("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.
> 
> That interface doesn't make sense for !HAVE_STATIC_CALL.

Perhaps, perhaps not. But I think it's silly to have code with such a
random hidden dependency, especially when it's only a defense against
crazy oot modules and not some fundamental requirement.

> It's impossible
> to not export the function pointer itself but still call it for
> !HAVE_STATIC_CALL.

Well, I think there's a way. At this point, the audience is asked to
wear sun glasses.

// foo.h
extern const int foo;
extern int __foo_just_for_vmlinux;

// foo.c
int __foo_just_for_vmlinux;
extern const int foo __attribute__((__alias__("__foo_just_for_vmlinux")));
EXPORT_SYMBOL(foo);

Modules can read foo, but can't do foo = 5. (Yeah, they can take the
address and cast away the const...). Basically, this is a kind of
top-level anonymous union trick a la i_nlink/__i_nlink. And it's more or
less explicitly said in the gcc docs that this is supposed to work:
"Except for top-level qualifiers the alias target must have the same
type as the alias."

Rasmus