Re: [PATCH v4 09/17] perf/core: Use static_call to optimize perf_guest_info_callbacks

[Carlos Llamas]

On Sun, Feb 06, 2022 at 06:55:56PM -0800, Kees Cook wrote:
> On Sun, Feb 06, 2022 at 09:28:52PM +0100, Peter Zijlstra wrote:
> > On Sun, Feb 06, 2022 at 10:45:15AM -0800, Kees Cook wrote:
> > 
> > > I'm digging through the macros to sort this out, but IIUC, an example of
> > > the problem is:
> > > 
> > 
> > > so the caller is expecting "unsigned int (*)(void)" but the prototype
> > > of __static_call_return0 is "long (*)(void)":
> > > 
> > > long __static_call_return0(void);
> > > 
> > > Could we simply declare a type-matched ret0 trampoline too?
> > 
> > That'll work for this case, but the next case the function will have
> > arguments we'll need even more nonsense...
> 
> Shouldn't the typeof() work there too, though? I.e. as long as the
> return value can hold a "0", it'd work.

I gave this a shot but then hit a wall with the arguments indeed:

typedef int (perf_snapshot_branch_stack_t)(struct perf_branch_entry *entries,
					   unsigned int cnt);
[...]
DEFINE_STATIC_CALL_RET0(perf_snapshot_branch_stack, perf_snapshot_branch_stack_t);

I can generate a stub with the matching return type using typeof() but
the arguments have to be fixed e.g. to (void):

#define DEFINE_STATIC_CALL_RET0(name, _func)				\
	static inline typeof(((typeof(_func)*)0)())			\
		__static_call_ret0_##name(void) { return 0; }		\
	__DEFINE_STATIC_CALL(name, _func, __static_call_ret0_##name)

I believe this would work for most perf callbacks cases except the one
above because the arguments would generate a different hash for CFI.

> 
> > And as stated in that other email, there's tb_stub_func() having the
> > exact same problem as well.
> 
> Yeah, I'd need to go look at that again.

Is this testing for "_func == __static_call_return0" in static_call()?
Ok, but I don't understand how to handle the arguments here either.
The call sites do "static_call(name)(...)", and I don't see a way to
handle this using macro magic.

> 
> > The x86_64 CFI patches had a work-around for this, that could trivially
> > be lifted I suppose.
> 
> Yeah, I think it'd be similar. I haven't had a chance to go look at that
> again...
> 

What is this work-around for x86?

Downstream I had to resolve this my providing individual stubs for each
DEFINE_STATIC_CALL_RET0() :( If you care to see my hack:
https://android-review.googlesource.com/c/kernel/common/+/3980171

I don't have a clue on how to fix this properly though. Any ideas?

--
Carlos Llamas