On Wed, 18 Nov 2020 08:50:37 -0800
Nick Desaulniers [off-list ref] wrote:

On Wed, Nov 18, 2020 at 5:23 AM Peter Zijlstra [off-list ref] wrote:

quoted

On Tue, Nov 17, 2020 at 03:34:51PM -0500, Steven Rostedt wrote:
 

quoted

quoted

quoted

Since all tracepoints callbacks have at least one parameter (__data), we
could declare tp_stub_func as:

static void tp_stub_func(void *data, ...)
{
  return;
}

And now C knows that tp_stub_func() can be called with one or more
parameters, and had better be able to deal with it!  

AFAIU this won't work.

C99 6.5.2.2 Function calls

"If the function is defined with a type that is not compatible with the type (of the
expression) pointed to by the expression that denotes the called function, the behavior is
undefined."  

But is it really a problem in practice. I'm sure we could create an objtool
function to check to make sure we don't break anything at build time.  

I think that as long as the function is completely empty (it never
touches any of the arguments) this should work in practise.

That is:

  void tp_nop_func(void) { }  

or `void tp_nop_func()` if you plan to call it with different
parameter types that are all unused in the body.  If you do plan to
use them, maybe a pointer to a tagged union would be safer?

This stub function will never use the parameters passed to it.

You can see the patch I have for the tracepoint issue here:

 https://lore.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home (local)

I could change the stub from (void) to () if that would be better.

quoted

can be used as an argument to any function pointer that has a void
return. In fact, I already do that, grep for __static_call_nop().

I'm not sure what the LLVM-CFI crud makes of it, but that's their
problem.  

If you have instructions on how to exercise the code in question, we
can help test it with CFI.  Better to find any potential issues before
they get committed.

If you apply the patch to the Linux kernel, and then apply:

  https://lore.kernel.org/r/20201116181638.6b0de6f7@gandalf.local.home (local)

Which will force the failed case (to use the stubs). And build and boot the
kernel with those patches applied, you can test it with:


 # mount -t tracefs nodev /sys/kernel/tracing
 # cd /sys/kernel/tracing
 # echo 1 > events/sched/sched_switch/enable
 # mkdir instances/foo
 # echo 1 > instances/foo/events/sched/sched_switch/enable
 # echo 0 > events/sched/sched_switch/enable

Which add two callbacks to the function array for the sched_switch
tracepoint. The remove the first one, which would add the stub instead.

-- Steve