static call updates

RFC PATCH In review Ard Biesheuvel 2 participants 22 messages Nov 9, 2021

Subsystems (7)

x86 perf events Scheduler Generic include/asm header files +3 more subsystems

lore

Share & tools

lore

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #0 lore raw

cover static call updates

This series addresses a number of asymmetries and inconsistencies in both the API and the implementation of static calls. The aim is to ensure that the documented API is fully implemented and works correctly, regardless of which variety of static calls the arch decides to implement.

Another goal of this series is to ensure that the caller of the API does not need to choose a particular kind of macro depending on whether the static call exports its key and/or whether the static call in question may implement the NULL flavor.

Cc Peter Zijlstra <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Jason Baron <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Kees Cook <[email protected]>

Ard Biesheuvel (7):
static_call: get rid of static_call_cond()
static_call: deal with unexported keys without cluttering up the API
static_call: use helper to access non-exported key
static_call: fix broken static_call_query() for non-exported keys
static_call: use non-function types to refer to the trampolines
static_call: rename EXPORT_ macros to be more self-explanatory
static_call: add generic support for non-exported keys

 arch/x86/events/core.c                  |  20 ++--
 arch/x86/include/asm/kvm_host.h         |   4 +-
 arch/x86/include/asm/preempt.h          |   4 +-
 arch/x86/include/asm/static_call.h      |   6 -
 arch/x86/kvm/irq.c                      |   2 +-
 arch/x86/kvm/x86.c                      |  10 +-
 include/asm-generic/vmlinux.lds.h       |   5 +-
 include/linux/kernel.h                  |   2 +-
 include/linux/sched.h                   |   2 +-
 include/linux/static_call.h             | 117 +++++++++-----------
 include/linux/static_call_types.h       |  54 +++++----
 kernel/extable.c                        |   1 +
 kernel/sched/core.c                     |   8 +-
 kernel/static_call.c                    |  29 +----
 tools/include/linux/static_call_types.h |  54 +++++----
 tools/objtool/check.c                   |  43 ++++---
 16 files changed, 174 insertions(+), 187 deletions(-)

Signature · 1 line

2.30.2

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #1 lore raw

RFC PATCH 1/7 static_call: get rid of static_call_cond()

The main reason for the existence of static_call_cond() seems to be that in theory, when using the generic implementation of static calls, it may be possible for a compiler to elide the indirect call entirely if the target is NULL, while still guaranteeing that all side effects of argument evaluation occur as expected.

This is rather optimistic: as documented by an existing code comment, both GCC and Clang (version 10) get this wrong, and even if they ever get it right, this is far too subtle to rely on for a code path that is expected to be used only by the 'remaining' architectures once all the best supported ones implement either the out-of-line or inline optimized variety of static calls.

Given that having static_call_cond() clutters up the API, and puts the burden on the caller to go and check what kind of static call they are dealing with, let's just get rid of the distinction.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/events/core.c          | 20 ++++----
 arch/x86/include/asm/kvm_host.h |  4 +-
 arch/x86/kvm/irq.c              |  2 +-
 arch/x86/kvm/x86.c              |  4 +-
 include/linux/static_call.h     | 49 ++++----------------
 5 files changed, 23 insertions(+), 56 deletions(-)

arch/x86/events/core.c +10 ~~-10~~

index 38b2c779146f..ed1e5fcdb49d 100644
@@ -991,7 +991,7 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 	if (cpuc->txn_flags & PERF_PMU_TXN_ADD)
 		n0 -= cpuc->n_txn;
 
-	static_call_cond(x86_pmu_start_scheduling)(cpuc);
+	static_call(x86_pmu_start_scheduling)(cpuc);
 
 	for (i = 0, wmin = X86_PMC_IDX_MAX, wmax = 0; i < n; i++) {
 		c = cpuc->event_constraint[i];

@@ -1090,7 +1090,7 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 	 */
 	if (!unsched && assign) {
 		for (i = 0; i < n; i++)
-			static_call_cond(x86_pmu_commit_scheduling)(cpuc, i, assign[i]);
+			static_call(x86_pmu_commit_scheduling)(cpuc, i, assign[i]);
 	} else {
 		for (i = n0; i < n; i++) {
 			e = cpuc->event_list[i];

@@ -1098,13 +1098,13 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 			/*
 			 * release events that failed scheduling
 			 */
-			static_call_cond(x86_pmu_put_event_constraints)(cpuc, e);
+			static_call(x86_pmu_put_event_constraints)(cpuc, e);
 
 			cpuc->event_constraint[i] = NULL;
 		}
 	}
 
-	static_call_cond(x86_pmu_stop_scheduling)(cpuc);
+	static_call(x86_pmu_stop_scheduling)(cpuc);
 
 	return unsched ? -EINVAL : 0;
 }

@@ -1217,7 +1217,7 @@ static inline void x86_assign_hw_event(struct perf_event *event,
 	hwc->last_cpu = smp_processor_id();
 	hwc->last_tag = ++cpuc->tags[i];
 
-	static_call_cond(x86_pmu_assign)(event, idx);
+	static_call(x86_pmu_assign)(event, idx);
 
 	switch (hwc->idx) {
 	case INTEL_PMC_IDX_FIXED_BTS:

@@ -1494,7 +1494,7 @@ static int x86_pmu_add(struct perf_event *event, int flags)
 	 * This is before x86_pmu_enable() will call x86_pmu_start(),
 	 * so we enable LBRs before an event needs them etc..
 	 */
-	static_call_cond(x86_pmu_add)(event);
+	static_call(x86_pmu_add)(event);
 
 	ret = 0;
 out:

@@ -1647,7 +1647,7 @@ static void x86_pmu_del(struct perf_event *event, int flags)
 	if (i >= cpuc->n_events - cpuc->n_added)
 		--cpuc->n_added;
 
-	static_call_cond(x86_pmu_put_event_constraints)(cpuc, event);
+	static_call(x86_pmu_put_event_constraints)(cpuc, event);
 
 	/* Delete the array entry. */
 	while (++i < cpuc->n_events) {

@@ -1667,7 +1667,7 @@ static void x86_pmu_del(struct perf_event *event, int flags)
 	 * This is after x86_pmu_stop(); so we disable LBRs after any
 	 * event can need them etc..
 	 */
-	static_call_cond(x86_pmu_del)(event);
+	static_call(x86_pmu_del)(event);
 }
 
 int x86_pmu_handle_irq(struct pt_regs *regs)

@@ -2638,13 +2638,13 @@ static const struct attribute_group *x86_pmu_attr_groups[] = {
 
 static void x86_pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
 {
-	static_call_cond(x86_pmu_sched_task)(ctx, sched_in);
+	static_call(x86_pmu_sched_task)(ctx, sched_in);
 }
 
 static void x86_pmu_swap_task_ctx(struct perf_event_context *prev,
 				  struct perf_event_context *next)
 {
-	static_call_cond(x86_pmu_swap_task_ctx)(prev, next);
+	static_call(x86_pmu_swap_task_ctx)(prev, next);
 }
 
 void perf_check_microcode(void)

arch/x86/include/asm/kvm_host.h +2 -2

index 2acf37cc1991..295f6285b895 100644
@@ -1914,12 +1914,12 @@ static inline bool kvm_irq_is_postable(struct kvm_lapic_irq *irq)
 
 static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
 {
-	static_call_cond(kvm_x86_vcpu_blocking)(vcpu);
+	static_call(kvm_x86_vcpu_blocking)(vcpu);
 }
 
 static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
 {
-	static_call_cond(kvm_x86_vcpu_unblocking)(vcpu);
+	static_call(kvm_x86_vcpu_unblocking)(vcpu);
 }
 
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}

arch/x86/kvm/irq.c +1 -1

index 172b05343cfd..79e9354f076a 100644
@@ -150,7 +150,7 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
 {
 	__kvm_migrate_apic_timer(vcpu);
 	__kvm_migrate_pit_timer(vcpu);
-	static_call_cond(kvm_x86_migrate_timers)(vcpu);
+	static_call(kvm_x86_migrate_timers)(vcpu);
 }
 
 bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)

arch/x86/kvm/x86.c +2 -2

index c1c4e2b05a63..2202db26c4d7 100644
@@ -11418,7 +11418,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 		__x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, 0, 0);
 		mutex_unlock(&kvm->slots_lock);
 	}
-	static_call_cond(kvm_x86_vm_destroy)(kvm);
+	static_call(kvm_x86_vm_destroy)(kvm);
 	kvm_free_msr_filter(srcu_dereference_check(kvm->arch.msr_filter, &kvm->srcu, 1));
 	kvm_pic_destroy(kvm);
 	kvm_ioapic_destroy(kvm);

@@ -12038,7 +12038,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu)
 void kvm_arch_start_assignment(struct kvm *kvm)
 {
 	if (atomic_inc_return(&kvm->arch.assigned_device_count) == 1)
-		static_call_cond(kvm_x86_start_assignment)(kvm);
+		static_call(kvm_x86_start_assignment)(kvm);
 }
 EXPORT_SYMBOL_GPL(kvm_arch_start_assignment);

include/linux/static_call.h +8 ~~-41~~

90 lines not on this page · raw message

Signature · 1 line

2.30.2

Peter Zijlstra

maintainer

2021-11-09 18:38 UTC #2 lore raw

Re: [RFC PATCH 1/7] static_call: get rid of static_call_cond()

Quoted 13 lines · Ard Biesheuvel

The main reason for the existence of static_call_cond() seems to be that in theory, when using the generic implementation of static calls, it may be possible for a compiler to elide the indirect call entirely if the target is NULL, while still guaranteeing that all side effects of argument evaluation occur as expected.
This is rather optimistic: as documented by an existing code comment, both GCC and Clang (version 10) get this wrong, and even if they ever get it right, this is far too subtle to rely on for a code path that is expected to be used only by the 'remaining' architectures once all the best supported ones implement either the out-of-line or inline optimized variety of static calls.

Quoted 3 lines

Given that having static_call_cond() clutters up the API, and puts the burden on the caller to go and check what kind of static call they are dealing with, let's just get rid of the distinction.

No, static_call_cond() signifies the function can be NULL. Both gcc and clang generate correct (but wildly ineffecient) code for this. Without static_call_cond() the generic implementation will do a NULL deref.

That is, static_call_cond() does properly encapuslate:

	func = READ_ONCE(key.func);
	if (func)
		func(ARGS);

You can't take that out.

Ard Biesheuvel

author reviewer

2021-11-09 18:41 UTC #3 lore raw

Re: [RFC PATCH 1/7] static_call: get rid of static_call_cond()

Quoted 23 lines · Peter Zijlstra

Quoted 13 lines · Ard Biesheuvel
The main reason for the existence of static_call_cond() seems to be that in theory, when using the generic implementation of static calls, it may be possible for a compiler to elide the indirect call entirely if the target is NULL, while still guaranteeing that all side effects of argument evaluation occur as expected.
This is rather optimistic: as documented by an existing code comment, both GCC and Clang (version 10) get this wrong, and even if they ever get it right, this is far too subtle to rely on for a code path that is expected to be used only by the 'remaining' architectures once all the best supported ones implement either the out-of-line or inline optimized variety of static calls.
Quoted 3 lines
Given that having static_call_cond() clutters up the API, and puts the burden on the caller to go and check what kind of static call they are dealing with, let's just get rid of the distinction.
No, static_call_cond() signifies the function can be NULL. Both gcc and clang generate correct (but wildly ineffecient) code for this. Without static_call_cond() the generic implementation will do a NULL deref.

Quoted 7 lines

That is, static_call_cond() does properly encapuslate:
        func = READ_ONCE(key.func);
        if (func)
                func(ARGS);
You can't take that out.

I actually address that in the patch.

AIUI, the compiler generates an indirect call to __static_call_nop(), right? So why not simply set .func to the address of __static_call_nop() when NULL is passed to update / the initializer?

Peter Zijlstra

maintainer

2021-11-09 19:22 UTC #4 lore raw

Re: [RFC PATCH 1/7] static_call: get rid of static_call_cond()

Quoted 30 lines · Ard Biesheuvel

Quoted 29 lines · Peter Zijlstra
Quoted 13 lines · Ard Biesheuvel
The main reason for the existence of static_call_cond() seems to be that in theory, when using the generic implementation of static calls, it may be possible for a compiler to elide the indirect call entirely if the target is NULL, while still guaranteeing that all side effects of argument evaluation occur as expected.
This is rather optimistic: as documented by an existing code comment, both GCC and Clang (version 10) get this wrong, and even if they ever get it right, this is far too subtle to rely on for a code path that is expected to be used only by the 'remaining' architectures once all the best supported ones implement either the out-of-line or inline optimized variety of static calls.
Quoted 3 lines
Given that having static_call_cond() clutters up the API, and puts the burden on the caller to go and check what kind of static call they are dealing with, let's just get rid of the distinction.
No, static_call_cond() signifies the function can be NULL. Both gcc and clang generate correct (but wildly ineffecient) code for this. Without static_call_cond() the generic implementation will do a NULL deref.
That is, static_call_cond() does properly encapuslate:
        func = READ_ONCE(key.func);
        if (func)
                func(ARGS);

Quoted 7 lines

Quoted 1 line
You can't take that out.
I actually address that in the patch.
AIUI, the compiler generates an indirect call to __static_call_nop(), right? So why not simply set .func to the address of __static_call_nop() when NULL is passed to update / the initializer?

Ooh, lemme go have a proper look then.

Peter Zijlstra

maintainer

2021-11-09 19:33 UTC #5 lore raw

Re: [RFC PATCH 1/7] static_call: get rid of static_call_cond()

In reply to Ard Biesheuvel

include/linux/static_call.h static inline long __static_call_return0(void) · line 285

Quoted 5 lines · Ard Biesheuvel

static inline
void __static_call_update(struct static_call_key *key, void *tramp, void *func) {
include/linux/static_call.h +1 -0
+	WRITE_ONCE(key->func, func ?: (void *)&__static_call_nop);
 }

Ha, yes, I suppose that ought to work.

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #6 lore raw

RFC PATCH 2/7 static_call: deal with unexported keys without cluttering up the API

In reply to Ard Biesheuvel

In principle, invoking a static call does not require access to the static call key that is used to manipulate the function that the static call targets. This means that exporting only the static call but not its key to modules should prevent such modules from manipulating the target inadvertently.

However, for inline static calls, we do need access to the key at module load time, because the code patching routines rely on it. For this reason, there is special handling in objtool that recovers the key pointer from the address of the trampoline (which does get exported unconditionally), using a lookup table in the core kernel.

Unfortunately, this clutters up the user-visible API: a static call is normally invoked using the static_call() macro, but in cases where a) the call originates from a module and b) the associated key was not exported, it is left up to the user to pick the static_call_mod() macro instead. Since that macro relies on the lookup table in the core kernel, it can only be used on static calls that are exported from the core kernel.

It would be much better if the call sites can simply use static_call() and not have to figure out whether or not the static call exports its key and/or whether it as exported from the core kernel.

So let's address this, by using a weak reference for the key. This means we can force the symbol reference in the module to be emitted unconditionally, allowing the caller to use static_call() without knowing the difference. This does imply that the logic in objtool that tests whether a symbol exists in the object no longer works, as the symbol is guaranteed to exist. So instead, add another field to struct static_call_site to cover the trampoline reference, and fix up the module notifier that consumes this data accordingly.

This gets rid of the following macros:

__STATIC_CALL_MOD_ADDRESSABLE()
static_call_mod()
__static_call()
__raw_static_call()

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/include/asm/preempt.h          |  4 +--
 include/linux/kernel.h                  |  2 +-
 include/linux/sched.h                   |  2 +-
 include/linux/static_call_types.h       | 26 +++++-----------
 kernel/static_call.c                    |  8 ++---
 tools/include/linux/static_call_types.h | 26 +++++-----------
 tools/objtool/check.c                   | 31 ++++++++++----------
 7 files changed, 38 insertions(+), 61 deletions(-)

arch/x86/include/asm/preempt.h +2 -2

index fe5efbcba824..341745a03726 100644
@@ -121,7 +121,7 @@ DECLARE_STATIC_CALL(preempt_schedule, __preempt_schedule_func);
 
 #define __preempt_schedule() \
 do { \
-	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule); \
+	__STATIC_CALL_ADDRESSABLE(preempt_schedule); \
 	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule) : ASM_CALL_CONSTRAINT); \
 } while (0)

@@ -129,7 +129,7 @@ DECLARE_STATIC_CALL(preempt_schedule_notrace, __preempt_schedule_notrace_func);
 
 #define __preempt_schedule_notrace() \
 do { \
-	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule_notrace); \
+	__STATIC_CALL_ADDRESSABLE(preempt_schedule_notrace); \
 	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule_notrace) : ASM_CALL_CONSTRAINT); \
 } while (0)

include/linux/kernel.h +1 -1

index e5359b09de1d..1e1159878e40 100644
@@ -101,7 +101,7 @@ DECLARE_STATIC_CALL(might_resched, __cond_resched);
 
 static __always_inline void might_resched(void)
 {
-	static_call_mod(might_resched)();
+	static_call(might_resched)();
 }
 
 #else

include/linux/sched.h +1 -1

index 78c351e35fec..3cdf55c1074c 100644
@@ -2014,7 +2014,7 @@ DECLARE_STATIC_CALL(cond_resched, __cond_resched);
 
 static __always_inline int _cond_resched(void)
 {
-	return static_call_mod(cond_resched)();
+	return static_call(cond_resched)();
 }
 
 #else

include/linux/static_call_types.h +7 ~~-19~~

index 5a00b8b2cf9f..0bb36294cce7 100644
@@ -32,15 +32,20 @@
 struct static_call_site {
 	s32 addr;
 	s32 key;
+	s32 tramp;
 };
 
 #define DECLARE_STATIC_CALL(name, func)					\
-	extern struct static_call_key STATIC_CALL_KEY(name);		\
+	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern typeof(func) STATIC_CALL_TRAMP(name);
 
 #ifdef CONFIG_HAVE_STATIC_CALL
 
-#define __raw_static_call(name)	(&STATIC_CALL_TRAMP(name))
+#define static_call(name)						\
+({									\
+	__STATIC_CALL_ADDRESSABLE(name);				\
+	(&STATIC_CALL_TRAMP(name));					\
+})
 
 #ifdef CONFIG_HAVE_STATIC_CALL_INLINE

@@ -52,12 +57,6 @@ struct static_call_site {
 #define __STATIC_CALL_ADDRESSABLE(name) \
 	__ADDRESSABLE(STATIC_CALL_KEY(name))
 
-#define __static_call(name)						\
-({									\
-	__STATIC_CALL_ADDRESSABLE(name);				\
-	__raw_static_call(name);					\
-})
-
 struct static_call_key {
 	void *func;
 	union {

@@ -71,7 +70,6 @@ struct static_call_key {
 #else /* !CONFIG_HAVE_STATIC_CALL_INLINE */
 
 #define __STATIC_CALL_ADDRESSABLE(name)
-#define __static_call(name)	__raw_static_call(name)
 
 struct static_call_key {
 	void *func;

@@ -79,16 +77,6 @@ struct static_call_key {
 
 #endif /* CONFIG_HAVE_STATIC_CALL_INLINE */
 
-#ifdef MODULE
-#define __STATIC_CALL_MOD_ADDRESSABLE(name)
-#define static_call_mod(name)	__raw_static_call(name)
-#else
-#define __STATIC_CALL_MOD_ADDRESSABLE(name) __STATIC_CALL_ADDRESSABLE(name)
-#define static_call_mod(name)	__static_call(name)
-#endif
-
-#define static_call(name)	__static_call(name)
-
 #else
 
 struct static_call_key {

kernel/static_call.c +4 -4

index 43ba0b1e0edb..360cc3cd0fbf 100644
@@ -366,18 +366,18 @@ static int static_call_add_module(struct module *mod)
 		 * means modules are allowed to call static_call_update() on
 		 * it.
 		 *
-		 * Otherwise, the key isn't exported, and 'addr' points to the
+		 * Otherwise, the key isn't exported, and 'tramp' points to the
 		 * trampoline so we need to lookup the key.
 		 *
 		 * We go through this dance to prevent crazy modules from
 		 * abusing sensitive static calls.
 		 */
-		if (!kernel_text_address(addr))
+		if (addr)
 			continue;
 
-		key = tramp_key_lookup(addr);
+		key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
 		if (!key) {
-			pr_warn("Failed to fixup __raw_static_call() usage at: %ps\n",
+			pr_warn("Failed to fixup static_call() usage at: %ps\n",
 				static_call_addr(site));
 			return -EINVAL;
 		}

tools/include/linux/static_call_types.h +7 ~~-19~~

index 5a00b8b2cf9f..0bb36294cce7 100644
@@ -32,15 +32,20 @@
 struct static_call_site {
 	s32 addr;
 	s32 key;
+	s32 tramp;
 };
 
 #define DECLARE_STATIC_CALL(name, func)					\
-	extern struct static_call_key STATIC_CALL_KEY(name);		\
+	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern typeof(func) STATIC_CALL_TRAMP(name);
 
 #ifdef CONFIG_HAVE_STATIC_CALL
 
-#define __raw_static_call(name)	(&STATIC_CALL_TRAMP(name))
+#define static_call(name)						\
+({									\
+	__STATIC_CALL_ADDRESSABLE(name);				\
+	(&STATIC_CALL_TRAMP(name));					\
+})
 
 #ifdef CONFIG_HAVE_STATIC_CALL_INLINE

@@ -52,12 +57,6 @@ struct static_call_site {
 #define __STATIC_CALL_ADDRESSABLE(name) \
 	__ADDRESSABLE(STATIC_CALL_KEY(name))
 
-#define __static_call(name)						\
-({									\
-	__STATIC_CALL_ADDRESSABLE(name);				\
-	__raw_static_call(name);					\
-})
-
 struct static_call_key {
 	void *func;
 	union {

@@ -71,7 +70,6 @@ struct static_call_key {
 #else /* !CONFIG_HAVE_STATIC_CALL_INLINE */
 
 #define __STATIC_CALL_ADDRESSABLE(name)
-#define __static_call(name)	__raw_static_call(name)
 
 struct static_call_key {
 	void *func;

@@ -79,16 +77,6 @@ struct static_call_key {
 
 #endif /* CONFIG_HAVE_STATIC_CALL_INLINE */
 
-#ifdef MODULE
-#define __STATIC_CALL_MOD_ADDRESSABLE(name)
-#define static_call_mod(name)	__raw_static_call(name)
-#else
-#define __STATIC_CALL_MOD_ADDRESSABLE(name) __STATIC_CALL_ADDRESSABLE(name)
-#define static_call_mod(name)	__static_call(name)
-#endif
-
-#define static_call(name)	__static_call(name)
-
 #else
 
 struct static_call_key {

tools/objtool/check.c +16 ~~-15~~

index add39902166d..ec6ddaaf6cf6 100644
@@ -652,21 +652,8 @@ static int create_static_call_sections(struct objtool_file *file)
 
 		key_sym = find_symbol_by_name(file->elf, tmp);
 		if (!key_sym) {
-			if (!module) {
-				WARN("static_call: can't find static_call_key symbol: %s", tmp);
-				return -1;
-			}
-
-			/*
-			 * For modules(), the key might not be exported, which
-			 * means the module can make static calls but isn't
-			 * allowed to change them.
-			 *
-			 * In that case we temporarily set the key to be the
-			 * trampoline address.  This is fixed up in
-			 * static_call_add_module().
-			 */
-			key_sym = insn->call_dest;
+			WARN("static_call: can't find static_call_key symbol: %s", tmp);
+			return -1;
 		}
 		free(key_name);

@@ -677,6 +664,20 @@ static int create_static_call_sections(struct objtool_file *file)
 				  is_sibling_call(insn) * STATIC_CALL_SITE_TAIL))
 			return -1;
 
+		/*
+		 * For modules(), the key might not be exported, which means
+		 * the module can make static calls but isn't allowed to change
+		 * them.
+		 *
+		 * For this case, we pass the trampoline in .static_call_sites
+		 * as well. This is used to lookup the key in
+		 * static_call_add_module().
+		 */
+		if (elf_add_reloc(file->elf, sec,
+				  idx * sizeof(struct static_call_site) + 8,
+				  R_X86_64_PC32, insn->call_dest, 0))
+			return -1;
+
 		idx++;
 	}

Signature · 1 line

2.30.2

Peter Zijlstra

maintainer

2021-11-09 18:49 UTC #7 lore raw

Re: [RFC PATCH 2/7] static_call: deal with unexported keys without cluttering up the API

include/linux/static_call_types.h line 36

Quoted 10 lines · Ard Biesheuvel

include/linux/static_call_types.h +1 -0

index 5a00b8b2cf9f..0bb36294cce7 100644
@@ -32,15 +32,20 @@
 struct static_call_site {
 	s32 addr;
 	s32 key;
+	s32 tramp;
 };

I can't say I'm thrilled at growing this thing, but the cleanup is nice. Perhaps we can increase alignment on struct static_call_key and instead frob it in .key still?

include/linux/static_call_types.h line 40

Quoted 5 lines

include/linux/static_call_types.h +1 -1

 #define DECLARE_STATIC_CALL(name, func)					\
-	extern struct static_call_key STATIC_CALL_KEY(name);		\
+	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern typeof(func) STATIC_CALL_TRAMP(name);

I'm a little bit confused on how this actually works. What does a __weak extern data symbol do?

A __weak function definition would create a module local instance of the function barring a strong override.

But what does a __weak extern do?

Quoted 8 lines

kernel/static_call.c +0 -0

index 43ba0b1e0edb..360cc3cd0fbf 100644
@@ -366,18 +366,18 @@ static int static_call_add_module(struct module *mod)
 		 * means modules are allowed to call static_call_update() on
 		 * it.
 		 *

kernel/static_call.c static int static_call_add_module(struct module *mod) · line 376

Quoted 10 lines

kernel/static_call.c +2 -2

-		 * Otherwise, the key isn't exported, and 'addr' points to the
+		 * Otherwise, the key isn't exported, and 'tramp' points to the
 		 * trampoline so we need to lookup the key.
 		 *
 		 * We go through this dance to prevent crazy modules from
 		 * abusing sensitive static calls.
 		 */
-		if (!kernel_text_address(addr))
+		if (addr)
 			continue;

This seems to imply that the __weak extern symbol gets resolved to 0 at module link time.

Quoted 10 lines

kernel/static_call.c +2 -2

-		key = tramp_key_lookup(addr);
+		key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
 		if (!key) {
-			pr_warn("Failed to fixup __raw_static_call() usage at: %ps\n",
+			pr_warn("Failed to fixup static_call() usage at: %ps\n",
 				static_call_addr(site));
 			return -EINVAL;
 
 		}

Ard Biesheuvel

author reviewer

2021-11-09 18:53 UTC #8 lore raw

Re: [RFC PATCH 2/7] static_call: deal with unexported keys without cluttering up the API

Quoted 18 lines · Peter Zijlstra

include/linux/static_call_types.h line 36
Quoted 10 lines · Ard Biesheuvel
include/linux/static_call_types.h +1 -0
index 5a00b8b2cf9f..0bb36294cce7 100644
@@ -32,15 +32,20 @@
 struct static_call_site {
      s32 addr;
      s32 key;
+     s32 tramp;
 };
I can't say I'm thrilled at growing this thing, but the cleanup is nice. Perhaps we can increase alignment on struct static_call_key and instead frob it in .key still?

This is already a place-relative field, and one points into the data section and the other into text. So I don't see how we can squeeze enough bits out of it to make this fit.

Quoted 6 lines

include/linux/static_call_types.h line 40

Quoted 5 lines

include/linux/static_call_types.h +1 -1

 #define DECLARE_STATIC_CALL(name, func)                                      \
-     extern struct static_call_key STATIC_CALL_KEY(name);            \
+     extern __weak struct static_call_key STATIC_CALL_KEY(name);     \
      extern typeof(func) STATIC_CALL_TRAMP(name);

Quoted 8 lines

I'm a little bit confused on how this actually works. What does a __weak extern data symbol do?
A __weak function definition would create a module local instance of the function barring a strong override.
But what does a __weak extern do?

It is simply a reference that is permitted to remain undefined.

That is why (in another patch) I do sth like

extern __weak foo;

if (&foo)
... use foo
else
... use sth else

Quoted 13 lines

kernel/static_call.c +1 -1

index 43ba0b1e0edb..360cc3cd0fbf 100644
@@ -366,18 +366,18 @@ static int static_call_add_module(struct module *mod)
               * means modules are allowed to call static_call_update() on
               * it.
               *
-              * Otherwise, the key isn't exported, and 'addr' points to the
+              * Otherwise, the key isn't exported, and 'tramp' points to the
               * trampoline so we need to lookup the key.
               *
               * We go through this dance to prevent crazy modules from

Quoted 9 lines

kernel/static_call.c static int static_call_add_module(struct module *mod) · line 376
Quoted 5 lines
kernel/static_call.c +1 -1
               * abusing sensitive static calls.
               */
-             if (!kernel_text_address(addr))
+             if (addr)
                      continue;
This seems to imply that the __weak extern symbol gets resolved to 0 at module link time.

Yes, if the referenced symbol is not exported, it just uses 0x0 for its value.

Quoted 10 lines

kernel/static_call.c +2 -2

-             key = tramp_key_lookup(addr);
+             key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
              if (!key) {
-                     pr_warn("Failed to fixup __raw_static_call() usage at: %ps\n",
+                     pr_warn("Failed to fixup static_call() usage at: %ps\n",
                              static_call_addr(site));
                      return -EINVAL;
 
              }

Peter Zijlstra

maintainer

2021-11-09 19:41 UTC #9 lore raw

Re: [RFC PATCH 2/7] static_call: deal with unexported keys without cluttering up the API

Quoted 20 lines · Ard Biesheuvel

Quoted 18 lines · Peter Zijlstra
include/linux/static_call_types.h line 36
Quoted 10 lines · Ard Biesheuvel
include/linux/static_call_types.h +1 -0
index 5a00b8b2cf9f..0bb36294cce7 100644
@@ -32,15 +32,20 @@
 struct static_call_site {
      s32 addr;
      s32 key;
+     s32 tramp;
 };
I can't say I'm thrilled at growing this thing, but the cleanup is nice. Perhaps we can increase alignment on struct static_call_key and instead frob it in .key still?

Quoted 3 lines

This is already a place-relative field, and one points into the data section and the other into text. So I don't see how we can squeeze enough bits out of it to make this fit.

Well, the actual address will be:

	((unsigned long)&site->key + site->key)

either way around, right? Now, if we align struct static_call_key to 8, that means the low 3 bits of that address will be 0 and free for us to muck about with.

That is, we already use the low 2 bits of that (because natural alignment etc.).

If we got 3 bits, we could shift the existing two bits one up and free up bit0, then say that if bit0 is set, it's a trampoline address. The only additional requirement would be that trampolines are (at least) aligned 2 (they're at 4 currently).

Except.... it would be the linker having to create that field, and there's no way that thing is smart like that :-( Ooh well.

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #10 lore raw

RFC PATCH 3/7 static_call: use helper to access non-exported key

In reply to Ard Biesheuvel

We support static calls from modules without the keys being exported, but since the inline patching code needs access to the key regardless, we have special handling in the module notifier that looks up the key based on the trampoline address in a list that is kept especially for this purpose.

This list is part of the core kernel, and only contains static calls exported from the core kernel. This means that exporting static calls without the associated key is only possible from the core kernel, and any attempts to do so from another module currently result in module load failures.

So let's replace this list with a special per-static call helper that returns the static call key address, provided that the call originates from the core kernel. This prevents access to the key for modular callers, while removing the need to keep a list of key/trampoline pairs.

Signed-off-by Ard Biesheuvel <[email protected]>

---
Alternatively, we might fix the module loader to look for .static_call_tramp_key sections in all modules loaded prior, but I'm not sure whether that would be cleaner.

 arch/x86/include/asm/static_call.h      |  6 -----
 include/asm-generic/vmlinux.lds.h       |  5 +----
 include/linux/static_call.h             | 18 ++++++++-------
 include/linux/static_call_types.h       |  9 +++++++-
 kernel/extable.c                        |  1 +
 kernel/static_call.c                    | 23 +++-----------------
 tools/include/linux/static_call_types.h |  9 +++++++-
 tools/objtool/check.c                   | 20 ++++++++++++-----
 8 files changed, 45 insertions(+), 46 deletions(-)

arch/x86/include/asm/static_call.h +0 -6

index cbb67b6030f9..beb2bbaae8b1 100644
@@ -38,10 +38,4 @@
 	__ARCH_DEFINE_STATIC_CALL_TRAMP(name, "ret; nop; nop; nop; nop")
 
 
-#define ARCH_ADD_TRAMP_KEY(name)					\
-	asm(".pushsection .static_call_tramp_key, \"a\"		\n"	\
-	    ".long " STATIC_CALL_TRAMP_STR(name) " - .		\n"	\
-	    ".long " STATIC_CALL_KEY_STR(name) " - .		\n"	\
-	    ".popsection					\n")
-
 #endif /* _ASM_STATIC_CALL_H */

include/asm-generic/vmlinux.lds.h +1 -4

index 42f3866bca69..5fd802ccb62f 100644
@@ -408,10 +408,7 @@
 	. = ALIGN(8);							\
 	__start_static_call_sites = .;					\
 	KEEP(*(.static_call_sites))					\
-	__stop_static_call_sites = .;					\
-	__start_static_call_tramp_key = .;				\
-	KEEP(*(.static_call_tramp_key))					\
-	__stop_static_call_tramp_key = .;
+	__stop_static_call_sites = .;
 
 /*
  * Allow architectures to handle ro_after_init data on their

include/linux/static_call.h +10 -8

index 19c98cab8643..3bba0bcba844 100644
@@ -161,12 +161,6 @@ struct static_call_mod {
 	struct static_call_site *sites;
 };
 
-/* For finding the key associated with a trampoline */
-struct static_call_tramp_key {
-	s32 tramp;
-	s32 key;
-};
-
 extern void __static_call_update(struct static_call_key *key, void *tramp, void *func);
 extern int static_call_mod_init(struct module *mod);
 extern int static_call_text_reserved(void *start, void *end);

@@ -196,13 +190,21 @@ extern long __static_call_return0(void);
 	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));			\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
+#define EXPORT_STATIC_CALL_GETKEY_HELPER(name)				\
+	struct static_call_key *STATIC_CALL_GETKEY(name)(void) {	\
+		BUG_ON(!core_kernel_text(				\
+			(unsigned long)__builtin_return_address(0)));	\
+		return &STATIC_CALL_KEY(name);				\
+	}								\
+	EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))
+
 /* Leave the key unexported, so modules can't change static call targets: */
 #define EXPORT_STATIC_CALL_TRAMP(name)					\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name));				\
-	ARCH_ADD_TRAMP_KEY(name)
+	EXPORT_STATIC_CALL_GETKEY_HELPER(name)
 #define EXPORT_STATIC_CALL_TRAMP_GPL(name)				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name));			\
-	ARCH_ADD_TRAMP_KEY(name)
+	EXPORT_STATIC_CALL_GETKEY_HELPER(name)
 
 #elif defined(CONFIG_HAVE_STATIC_CALL)

include/linux/static_call_types.h +8 -1

index 0bb36294cce7..a31782909e43 100644
@@ -18,6 +18,11 @@
 #define STATIC_CALL_TRAMP(name)		__PASTE(STATIC_CALL_TRAMP_PREFIX, name)
 #define STATIC_CALL_TRAMP_STR(name)	__stringify(STATIC_CALL_TRAMP(name))
 
+#define STATIC_CALL_GETKEY_PREFIX	__SCG__
+#define STATIC_CALL_GETKEY_PREFIX_STR	__stringify(STATIC_CALL_GETKEY_PREFIX)
+#define STATIC_CALL_GETKEY_PREFIX_LEN	(sizeof(STATIC_CALL_GETKEY_PREFIX_STR) - 1)
+#define STATIC_CALL_GETKEY(name)	__PASTE(STATIC_CALL_GETKEY_PREFIX, name)
+
 /*
  * Flags in the low bits of static_call_site::key.
  */

@@ -32,11 +37,12 @@
 struct static_call_site {
 	s32 addr;
 	s32 key;
-	s32 tramp;
+	s32 helper;
 };
 
 #define DECLARE_STATIC_CALL(name, func)					\
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
+	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
 	extern typeof(func) STATIC_CALL_TRAMP(name);
 
 #ifdef CONFIG_HAVE_STATIC_CALL

@@ -55,6 +61,7 @@ struct static_call_site {
  * .static_call_sites section.
  */
 #define __STATIC_CALL_ADDRESSABLE(name) \
+	__ADDRESSABLE(STATIC_CALL_GETKEY(name)) \
 	__ADDRESSABLE(STATIC_CALL_KEY(name))
 
 struct static_call_key {

kernel/extable.c +1 -0

index 290661f68e6b..e52579e46180 100644
@@ -81,6 +81,7 @@ int notrace core_kernel_text(unsigned long addr)
 		return 1;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(core_kernel_text);
 
 /**
  * core_kernel_data - tell if addr points to kernel data

kernel/static_call.c +3 ~~-20~~

index 360cc3cd0fbf..29ae772dc86e 100644
@@ -12,8 +12,6 @@
 
 extern struct static_call_site __start_static_call_sites[],
 			       __stop_static_call_sites[];
-extern struct static_call_tramp_key __start_static_call_tramp_key[],
-				    __stop_static_call_tramp_key[];
 
 static bool static_call_initialized;

@@ -333,23 +331,6 @@ static int __static_call_mod_text_reserved(void *start, void *end)
 	return ret;
 }
 
-static unsigned long tramp_key_lookup(unsigned long addr)
-{
-	struct static_call_tramp_key *start = __start_static_call_tramp_key;
-	struct static_call_tramp_key *stop = __stop_static_call_tramp_key;
-	struct static_call_tramp_key *tramp_key;
-
-	for (tramp_key = start; tramp_key != stop; tramp_key++) {
-		unsigned long tramp;
-
-		tramp = (long)tramp_key->tramp + (long)&tramp_key->tramp;
-		if (tramp == addr)
-			return (long)tramp_key->key + (long)&tramp_key->key;
-	}
-
-	return 0;
-}
-
 static int static_call_add_module(struct module *mod)
 {
 	struct static_call_site *start = mod->static_call_sites;

@@ -359,6 +340,7 @@ static int static_call_add_module(struct module *mod)
 	for (site = start; site != stop; site++) {
 		unsigned long s_key = __static_call_key(site);
 		unsigned long addr = s_key & ~STATIC_CALL_SITE_FLAGS;
+		unsigned long (*key_helper)(void);
 		unsigned long key;
 
 		/*

@@ -375,7 +357,8 @@ static int static_call_add_module(struct module *mod)
 		if (addr)
 			continue;
 
-		key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
+		key_helper = offset_to_ptr(&site->helper);
+		key = key_helper();
 		if (!key) {
 			pr_warn("Failed to fixup static_call() usage at: %ps\n",
 				static_call_addr(site));

tools/include/linux/static_call_types.h +8 -1

index 0bb36294cce7..a31782909e43 100644
@@ -18,6 +18,11 @@
 #define STATIC_CALL_TRAMP(name)		__PASTE(STATIC_CALL_TRAMP_PREFIX, name)
 #define STATIC_CALL_TRAMP_STR(name)	__stringify(STATIC_CALL_TRAMP(name))
 
+#define STATIC_CALL_GETKEY_PREFIX	__SCG__
+#define STATIC_CALL_GETKEY_PREFIX_STR	__stringify(STATIC_CALL_GETKEY_PREFIX)
+#define STATIC_CALL_GETKEY_PREFIX_LEN	(sizeof(STATIC_CALL_GETKEY_PREFIX_STR) - 1)
+#define STATIC_CALL_GETKEY(name)	__PASTE(STATIC_CALL_GETKEY_PREFIX, name)
+
 /*
  * Flags in the low bits of static_call_site::key.
  */

@@ -32,11 +37,12 @@
 struct static_call_site {
 	s32 addr;
 	s32 key;
-	s32 tramp;
+	s32 helper;
 };
 
 #define DECLARE_STATIC_CALL(name, func)					\
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
+	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
 	extern typeof(func) STATIC_CALL_TRAMP(name);
 
 #ifdef CONFIG_HAVE_STATIC_CALL

@@ -55,6 +61,7 @@ struct static_call_site {
  * .static_call_sites section.
  */
 #define __STATIC_CALL_ADDRESSABLE(name) \
+	__ADDRESSABLE(STATIC_CALL_GETKEY(name)) \
 	__ADDRESSABLE(STATIC_CALL_KEY(name))
 
 struct static_call_key {

tools/objtool/check.c +14 -6

index ec6ddaaf6cf6..3698ace884a5 100644
@@ -600,7 +600,7 @@ static int create_static_call_sections(struct objtool_file *file)
 	struct section *sec;
 	struct static_call_site *site;
 	struct instruction *insn;
-	struct symbol *key_sym;
+	struct symbol *key_sym, *helper_sym;
 	char *key_name, *tmp;
 	int idx;

@@ -655,7 +655,6 @@ static int create_static_call_sections(struct objtool_file *file)
 			WARN("static_call: can't find static_call_key symbol: %s", tmp);
 			return -1;
 		}
-		free(key_name);
 
 		/* populate reloc for 'key' */
 		if (elf_add_reloc(file->elf, sec,

@@ -664,18 +663,27 @@ static int create_static_call_sections(struct objtool_file *file)
 				  is_sibling_call(insn) * STATIC_CALL_SITE_TAIL))
 			return -1;
 
+		memcpy(tmp, STATIC_CALL_GETKEY_PREFIX_STR, STATIC_CALL_GETKEY_PREFIX_LEN);
+
+		helper_sym = find_symbol_by_name(file->elf, tmp);
+		if (!helper_sym) {
+			WARN("static_call: can't find static_call_key symbol: %s", tmp);
+			return -1;
+		}
+		free(key_name);
+
 		/*
 		 * For modules(), the key might not be exported, which means
 		 * the module can make static calls but isn't allowed to change
 		 * them.
 		 *
-		 * For this case, we pass the trampoline in .static_call_sites
-		 * as well. This is used to lookup the key in
-		 * static_call_add_module().
+		 * For this case, we pass the special key helper routine in
+		 * .static_call_sites as well. This is used to lookup the key
+		 * in static_call_add_module().
 		 */
 		if (elf_add_reloc(file->elf, sec,
 				  idx * sizeof(struct static_call_site) + 8,
-				  R_X86_64_PC32, insn->call_dest, 0))
+				  R_X86_64_PC32, helper_sym, 0))
 			return -1;
 
 		idx++;

Signature · 1 line

2.30.2

Peter Zijlstra

maintainer

2021-11-09 18:53 UTC #11 lore raw

Re: [RFC PATCH 3/7] static_call: use helper to access non-exported key

include/linux/static_call.h extern long __static_call_return0(void); · line 199

Quoted 11 lines · Ard Biesheuvel

include/linux/static_call.h +7 -0

@@ -196,13 +190,21 @@ extern long __static_call_return0(void);
 	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));			\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
+#define EXPORT_STATIC_CALL_GETKEY_HELPER(name)				\
+	struct static_call_key *STATIC_CALL_GETKEY(name)(void) {	\
+		BUG_ON(!core_kernel_text(				\
+			(unsigned long)__builtin_return_address(0)));	\
+		return &STATIC_CALL_KEY(name);				\
+	}								\
+	EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))

So if I were a nevarious module, I would look up the above symbol from kallsyms (it is exported and easily obtainable) and then simply read the text to discover the key address and we're in business.

Ard Biesheuvel

author reviewer

2021-11-09 18:54 UTC #12 lore raw

Re: [RFC PATCH 3/7] static_call: use helper to access non-exported key

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Ard Biesheuvel

include/linux/static_call.h +0 -0

@@ -196,13 +190,21 @@ extern long __static_call_return0(void);
      EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));                       \
      EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))

Quoted 12 lines

include/linux/static_call.h extern long __static_call_return0(void); · line 199
Quoted 7 lines
include/linux/static_call.h +7 -0
+#define EXPORT_STATIC_CALL_GETKEY_HELPER(name)                               \
+     struct static_call_key *STATIC_CALL_GETKEY(name)(void) {        \
+             BUG_ON(!core_kernel_text(                               \
+                     (unsigned long)__builtin_return_address(0)));   \
+             return &STATIC_CALL_KEY(name);                          \
+     }                                                               \
+     EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))
So if I were a nevarious module, I would look up the above symbol from kallsyms (it is exported and easily obtainable) and then simply read the text to discover the key address and we're in business.

Yeah I realised that. So would you prefer to have a .static_call_tramp_key section in each module and look up the keys in the module loader?

Peter Zijlstra

maintainer

2021-11-09 19:42 UTC #13 lore raw

Re: [RFC PATCH 3/7] static_call: use helper to access non-exported key

Quoted 20 lines · Ard Biesheuvel

Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Ard Biesheuvel
include/linux/static_call.h +0 -0
@@ -196,13 +190,21 @@ extern long __static_call_return0(void);
      EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));                       \
      EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
Quoted 12 lines
include/linux/static_call.h extern long __static_call_return0(void); · line 199
Quoted 7 lines
include/linux/static_call.h +7 -0
+#define EXPORT_STATIC_CALL_GETKEY_HELPER(name)                               \
+     struct static_call_key *STATIC_CALL_GETKEY(name)(void) {        \
+             BUG_ON(!core_kernel_text(                               \
+                     (unsigned long)__builtin_return_address(0)));   \
+             return &STATIC_CALL_KEY(name);                          \
+     }                                                               \
+     EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))
So if I were a nevarious module, I would look up the above symbol from kallsyms (it is exported and easily obtainable) and then simply read the text to discover the key address and we're in business.

Quoted 3 lines

Yeah I realised that. So would you prefer to have a .static_call_tramp_key section in each module and look up the keys in the module loader?

Think so... do you actually have a need for this?

Ard Biesheuvel

author reviewer

2021-11-09 19:45 UTC #14 lore raw

Re: [RFC PATCH 3/7] static_call: use helper to access non-exported key

Quoted 22 lines · Peter Zijlstra

Quoted 20 lines · Ard Biesheuvel
Quoted 18 lines · Peter Zijlstra
include/linux/static_call.h extern long __static_call_return0(void); · line 199
Quoted 11 lines · Ard Biesheuvel
include/linux/static_call.h +7 -0
@@ -196,13 +190,21 @@ extern long __static_call_return0(void);
      EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));                       \
      EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
+#define EXPORT_STATIC_CALL_GETKEY_HELPER(name)                               \
+     struct static_call_key *STATIC_CALL_GETKEY(name)(void) {        \
+             BUG_ON(!core_kernel_text(                               \
+                     (unsigned long)__builtin_return_address(0)));   \
+             return &STATIC_CALL_KEY(name);                          \
+     }                                                               \
+     EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))
So if I were a nevarious module, I would look up the above symbol from kallsyms (it is exported and easily obtainable) and then simply read the text to discover the key address and we're in business.

Quoted 5 lines

Quoted 3 lines
Yeah I realised that. So would you prefer to have a .static_call_tramp_key section in each module and look up the keys in the module loader?
Think so... do you actually have a need for this?

Well, not exactly. But the XOR patch I sent earlier today does cover this use case, and would be broken if we implemented inline static calls on arm64.

So the easy fix is to allow EXPORT_STATIC_CALL_RO() only from the core kernel, and require EXPORT_STATIC_CALL_RW() otherwise. That way, this can never end up being used in a way that breaks at runtime. Would you prefer doing that instead?

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #15 lore raw

RFC PATCH 4/7 static_call: fix broken static_call_query() for non-exported keys

In reply to Ard Biesheuvel

static_call_query() accesses the func member of the static call key directly, which means that it is broken for cases where it is used from a module and the key resides elsewhere and is not exported.

Let's add a helper that returns this value, and export it from the same module that the key resides in if the key itself is not exported. This way, we can always get the value regardless of whether the key is exported or not.

Note that the non-NULL check of &STATIC_CALL_KEY(...) does not typically result in a load: for ISAs that support relocatable immediates, the address is patched into the instruction stream.

For example, on ARM/Thumb2, we get

14a: f240 0300 movw r3, #0

                        14a: R_ARM_THM_MOVW_ABS_NC      __SCK__pv_steal_clock
 14e:   f2c0 0300       movt    r3, #0
                        14e: R_ARM_THM_MOVT_ABS         __SCK__pv_steal_clock
 152:   b10b            cbz     r3, 158 <foo+0x14>
 154:   6818            ldr     r0, [r3, #0]
 156:   4770            bx      lr
 158:   f7ff bffe       b.w     0 <__SCQ__pv_steal_clock>
                        158: R_ARM_THM_JUMP24           __SCQ__pv_steal_clock

I.e., it performs a conditional branch if zero (CBZ) on a quantity that was loaded using an MOVW/MOVT move-immediate pair, and either loads the func member directly, or tail calls __SCQ__pv_steal_clock if they key is not exported.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 include/linux/static_call.h             |  9 ++++++++-
 include/linux/static_call_types.h       | 18 +++++++++++++++++-
 tools/include/linux/static_call_types.h | 18 +++++++++++++++++-
 3 files changed, 42 insertions(+), 3 deletions(-)

include/linux/static_call.h +8 -1

index 3bba0bcba844..391f737496eb 100644
@@ -149,7 +149,10 @@ extern void arch_static_call_transform(void *site, void *tramp, void *func, bool
 			     STATIC_CALL_TRAMP_ADDR(name), __F);	\
 })
 
-#define static_call_query(name) (READ_ONCE(STATIC_CALL_KEY(name).func))
+#define EXPORT_STATIC_CALL_QUERY(name, sfx)				\
+	typeof(STATIC_CALL_QUERY(name)()) STATIC_CALL_QUERY(name)(void)	\
+		{ return STATIC_CALL_KEY(name).func; }			\
+	EXPORT_SYMBOL ## sfx (STATIC_CALL_QUERY(name))
 
 #ifdef CONFIG_HAVE_STATIC_CALL_INLINE

@@ -200,9 +203,11 @@ extern long __static_call_return0(void);
 
 /* Leave the key unexported, so modules can't change static call targets: */
 #define EXPORT_STATIC_CALL_TRAMP(name)					\
+	EXPORT_STATIC_CALL_QUERY(name,);				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name));				\
 	EXPORT_STATIC_CALL_GETKEY_HELPER(name)
 #define EXPORT_STATIC_CALL_TRAMP_GPL(name)				\
+	EXPORT_STATIC_CALL_QUERY(name, _GPL);				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name));			\
 	EXPORT_STATIC_CALL_GETKEY_HELPER(name)

@@ -253,8 +258,10 @@ static inline long __static_call_return0(void)
 
 /* Leave the key unexported, so modules can't change static call targets: */
 #define EXPORT_STATIC_CALL_TRAMP(name)					\
+	EXPORT_STATIC_CALL_QUERY(name,);				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name))
 #define EXPORT_STATIC_CALL_TRAMP_GPL(name)				\
+	EXPORT_STATIC_CALL_QUERY(name, _GPL);				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
 #else /* Generic implementation */

include/linux/static_call_types.h +17 -1

index a31782909e43..2fce9aa8a995 100644
@@ -23,6 +23,9 @@
 #define STATIC_CALL_GETKEY_PREFIX_LEN	(sizeof(STATIC_CALL_GETKEY_PREFIX_STR) - 1)
 #define STATIC_CALL_GETKEY(name)	__PASTE(STATIC_CALL_GETKEY_PREFIX, name)
 
+#define STATIC_CALL_QUERY_PREFIX	__SCQ__
+#define STATIC_CALL_QUERY(name)		__PASTE(STATIC_CALL_QUERY_PREFIX, name)
+
 /*
  * Flags in the low bits of static_call_site::key.
  */

@@ -43,7 +46,20 @@ struct static_call_site {
 #define DECLARE_STATIC_CALL(name, func)					\
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
-	extern typeof(func) STATIC_CALL_TRAMP(name);
+	extern __weak typeof(func) *STATIC_CALL_QUERY(name)(void);	\
+	extern typeof(func) STATIC_CALL_TRAMP(name)
+
+#define __static_call_query(name)					\
+	((typeof(STATIC_CALL_QUERY(name)()))READ_ONCE(STATIC_CALL_KEY(name).func))
+
+#ifdef MODULE
+/* the key might not be exported */
+#define static_call_query(name)						\
+	(&STATIC_CALL_KEY(name) ? __static_call_query(name)		\
+				: STATIC_CALL_QUERY(name)())
+#else
+#define static_call_query(name)	__static_call_query(name)
+#endif
 
 #ifdef CONFIG_HAVE_STATIC_CALL

tools/include/linux/static_call_types.h +17 -1

index a31782909e43..2fce9aa8a995 100644
@@ -23,6 +23,9 @@
 #define STATIC_CALL_GETKEY_PREFIX_LEN	(sizeof(STATIC_CALL_GETKEY_PREFIX_STR) - 1)
 #define STATIC_CALL_GETKEY(name)	__PASTE(STATIC_CALL_GETKEY_PREFIX, name)
 
+#define STATIC_CALL_QUERY_PREFIX	__SCQ__
+#define STATIC_CALL_QUERY(name)		__PASTE(STATIC_CALL_QUERY_PREFIX, name)
+
 /*
  * Flags in the low bits of static_call_site::key.
  */

@@ -43,7 +46,20 @@ struct static_call_site {
 #define DECLARE_STATIC_CALL(name, func)					\
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
-	extern typeof(func) STATIC_CALL_TRAMP(name);
+	extern __weak typeof(func) *STATIC_CALL_QUERY(name)(void);	\
+	extern typeof(func) STATIC_CALL_TRAMP(name)
+
+#define __static_call_query(name)					\
+	((typeof(STATIC_CALL_QUERY(name)()))READ_ONCE(STATIC_CALL_KEY(name).func))
+
+#ifdef MODULE
+/* the key might not be exported */
+#define static_call_query(name)						\
+	(&STATIC_CALL_KEY(name) ? __static_call_query(name)		\
+				: STATIC_CALL_QUERY(name)())
+#else
+#define static_call_query(name)	__static_call_query(name)
+#endif
 
 #ifdef CONFIG_HAVE_STATIC_CALL

Signature · 1 line

2.30.2

Peter Zijlstra

maintainer

2021-11-09 18:56 UTC #16 lore raw

Re: [RFC PATCH 4/7] static_call: fix broken static_call_query() for non-exported keys

Quoted 4 lines · Ard Biesheuvel

static_call_query() accesses the func member of the static call key directly, which means that it is broken for cases where it is used from a module and the key resides elsewhere and is not exported.

Arguably when the module can't change the value, it shouldn't be having any interest in obtaining said value either.

I really think you're exporting too much. Modules aren't good, they deserve to suffer.

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #17 lore raw

RFC PATCH 5/7 static_call: use non-function types to refer to the trampolines

In reply to Ard Biesheuvel

In order to prevent CFI enabled code from grabbing a jump table entry that jumps to the trampoline, rather than the trampoline itself, use an incomplete non-function type for the trampoline, and cast it to the right type only when invoking it.

Acked-by Peter Zijlstra <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 include/linux/static_call.h             | 2 +-
 include/linux/static_call_types.h       | 6 +++---
 tools/include/linux/static_call_types.h | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

include/linux/static_call.h +1 -1

index 391f737496eb..6b261fe96ba3 100644
@@ -144,7 +144,7 @@ extern void arch_static_call_transform(void *site, void *tramp, void *func, bool
 
 #define static_call_update(name, func)					\
 ({									\
-	typeof(&STATIC_CALL_TRAMP(name)) __F = (func);			\
+	typeof(STATIC_CALL_QUERY(name)()) __F = (func);			\
 	__static_call_update(&STATIC_CALL_KEY(name),			\
 			     STATIC_CALL_TRAMP_ADDR(name), __F);	\
 })

include/linux/static_call_types.h +3 -3

index 2fce9aa8a995..eae11c5b015d 100644
@@ -47,7 +47,7 @@ struct static_call_site {
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
 	extern __weak typeof(func) *STATIC_CALL_QUERY(name)(void);	\
-	extern typeof(func) STATIC_CALL_TRAMP(name)
+	extern struct static_call_tramp STATIC_CALL_TRAMP(name)
 
 #define __static_call_query(name)					\
 	((typeof(STATIC_CALL_QUERY(name)()))READ_ONCE(STATIC_CALL_KEY(name).func))

@@ -66,7 +66,7 @@ struct static_call_site {
 #define static_call(name)						\
 ({									\
 	__STATIC_CALL_ADDRESSABLE(name);				\
-	(&STATIC_CALL_TRAMP(name));					\
+	((typeof(STATIC_CALL_QUERY(name)()))&STATIC_CALL_TRAMP(name));	\
 })
 
 #ifdef CONFIG_HAVE_STATIC_CALL_INLINE

@@ -107,7 +107,7 @@ struct static_call_key {
 };
 
 #define static_call(name)						\
-	((typeof(STATIC_CALL_TRAMP(name))*)(STATIC_CALL_KEY(name).func))
+	((typeof(STATIC_CALL_QUERY(name)()))(STATIC_CALL_KEY(name).func))
 
 #endif /* CONFIG_HAVE_STATIC_CALL */

tools/include/linux/static_call_types.h +3 -3

index 2fce9aa8a995..eae11c5b015d 100644
@@ -47,7 +47,7 @@ struct static_call_site {
 	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
 	extern __weak struct static_call_key *STATIC_CALL_GETKEY(name)(void);\
 	extern __weak typeof(func) *STATIC_CALL_QUERY(name)(void);	\
-	extern typeof(func) STATIC_CALL_TRAMP(name)
+	extern struct static_call_tramp STATIC_CALL_TRAMP(name)
 
 #define __static_call_query(name)					\
 	((typeof(STATIC_CALL_QUERY(name)()))READ_ONCE(STATIC_CALL_KEY(name).func))

@@ -66,7 +66,7 @@ struct static_call_site {
 #define static_call(name)						\
 ({									\
 	__STATIC_CALL_ADDRESSABLE(name);				\
-	(&STATIC_CALL_TRAMP(name));					\
+	((typeof(STATIC_CALL_QUERY(name)()))&STATIC_CALL_TRAMP(name));	\
 })
 
 #ifdef CONFIG_HAVE_STATIC_CALL_INLINE

@@ -107,7 +107,7 @@ struct static_call_key {
 };
 
 #define static_call(name)						\
-	((typeof(STATIC_CALL_TRAMP(name))*)(STATIC_CALL_KEY(name).func))
+	((typeof(STATIC_CALL_QUERY(name)()))(STATIC_CALL_KEY(name).func))
 
 #endif /* CONFIG_HAVE_STATIC_CALL */

Signature · 1 line

2.30.2

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #18 lore raw

RFC PATCH 6/7 static_call: rename EXPORT_ macros to be more self-explanatory

In reply to Ard Biesheuvel

There are currently two ways to export static calls to modules:
- export only the current static call target, so that modules can invoke it but not make it point to another target;
- export the static call key so that modules may call static_call_update() and patch all invocations tied to that key.

Depending on the flavor of static call implementation, different things need to be exported: for the generic case, we need to export the key regardless of which export was chosen, but for other cases, we only export the trampoline but not the key.

As the latter is an implementation detail, EXPORT_STATIC_CALL_TRAMP() is not the best choice for the name of the export macro that only exports the target but not the key.

So let's rename these macros to

EXPORT_STATIC_CALL -> EXPORT_STATIC_CALL_FOR_UPDATE
EXPORT_STATIC_CALL_TRAMP -> EXPORT_STATIC_CALL

(and the _GPL suffixed ones in a similar fashion). That way, the default does not expose the key, and users that need to expose control over the static call key to modules can do so explicitly by choosing the _FOR_UPDATE version instead.

While at it, add the missing ones for the generic static call implementation.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/kvm/x86.c          |  6 ++--
 include/linux/static_call.h | 35 ++++++++++++--------
 kernel/sched/core.c         |  8 ++---
 3 files changed, 28 insertions(+), 21 deletions(-)

arch/x86/kvm/x86.c +3 -3

index 2202db26c4d7..c3749911e966 100644
@@ -130,9 +130,9 @@ EXPORT_SYMBOL_GPL(kvm_x86_ops);
 				*(((struct kvm_x86_ops *)0)->func));
 #define KVM_X86_OP_NULL KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
-EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
-EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
-EXPORT_STATIC_CALL_GPL(kvm_x86_tlb_flush_current);
+EXPORT_STATIC_CALL_FOR_UPDATE_GPL(kvm_x86_get_cs_db_l_bits);
+EXPORT_STATIC_CALL_FOR_UPDATE_GPL(kvm_x86_cache_reg);
+EXPORT_STATIC_CALL_FOR_UPDATE_GPL(kvm_x86_tlb_flush_current);
 
 static bool __read_mostly ignore_msrs = 0;
 module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR);

include/linux/static_call.h +21 ~~-14~~

index 6b261fe96ba3..21929147879a 100644
@@ -26,7 +26,7 @@
  *   static_call_update(name, func);
  *   static_call_query(name);
  *
- *   EXPORT_STATIC_CALL{,_TRAMP}{,_GPL}()
+ *   EXPORT_STATIC_CALL{,_FOR_UPDATE}{,_GPL}()
  *
  * Usage example:
  *

@@ -90,7 +90,7 @@
  *     if (my_func_ptr)
  *         my_func_ptr(arg1)
  *
- *   where the argument evaludation also depends on the pointer value.
+ *   where the argument evaluation also depends on the pointer value.
  *
  *   To query which function is currently set to be called, use:
  *

@@ -116,11 +116,15 @@
  *   Notably argument setup is unconditional.
  *
  *
- * EXPORT_STATIC_CALL() vs EXPORT_STATIC_CALL_TRAMP():
+ * EXPORT_STATIC_CALL() vs EXPORT_STATIC_CALL_FOR_UPDATE():
  *
- *   The difference is that the _TRAMP variant tries to only export the
- *   trampoline with the result that a module can use static_call{,_cond}() but
- *   not static_call_update().
+ *   EXPORT_STATIC_CALL() exports the minimal set of symbols that are needed
+ *   for a module to be able invoke the static call, with the result that it
+ *   can use static_call{,_cond}() but not static_call_update(). If the intent
+ *   is to permit modules to manipulate the targets of static calls associated
+ *   with a certain static call key occurring anywhere in the core kernel or
+ *   other modules than the one importing the static call, use
+ *   EXPORT_STATIC_CALL_FOR_UPDATE() instead.
  *
  */

@@ -186,10 +190,10 @@ extern long __static_call_return0(void);
 	};								\
 	ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name)
 
-#define EXPORT_STATIC_CALL(name)					\
+#define EXPORT_STATIC_CALL_FOR_UPDATE(name)				\
 	EXPORT_SYMBOL(STATIC_CALL_KEY(name));				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name))
-#define EXPORT_STATIC_CALL_GPL(name)					\
+#define EXPORT_STATIC_CALL_FOR_UPDATE_GPL(name)				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));			\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))

@@ -202,11 +206,11 @@ extern long __static_call_return0(void);
 	EXPORT_SYMBOL_GPL(STATIC_CALL_GETKEY(name))
 
 /* Leave the key unexported, so modules can't change static call targets: */
-#define EXPORT_STATIC_CALL_TRAMP(name)					\
+#define EXPORT_STATIC_CALL(name)					\
 	EXPORT_STATIC_CALL_QUERY(name,);				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name));				\
 	EXPORT_STATIC_CALL_GETKEY_HELPER(name)
-#define EXPORT_STATIC_CALL_TRAMP_GPL(name)				\
+#define EXPORT_STATIC_CALL_GPL(name)					\
 	EXPORT_STATIC_CALL_QUERY(name, _GPL);				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name));			\
 	EXPORT_STATIC_CALL_GETKEY_HELPER(name)

@@ -249,18 +253,18 @@ static inline long __static_call_return0(void)
 	return 0;
 }
 
-#define EXPORT_STATIC_CALL(name)					\
+#define EXPORT_STATIC_CALL_FOR_UPDATE(name)				\
 	EXPORT_SYMBOL(STATIC_CALL_KEY(name));				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name))
-#define EXPORT_STATIC_CALL_GPL(name)					\
+#define EXPORT_STATIC_CALL_FOR_UPDATE_GPL(name)				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name));			\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))
 
 /* Leave the key unexported, so modules can't change static call targets: */
-#define EXPORT_STATIC_CALL_TRAMP(name)					\
+#define EXPORT_STATIC_CALL(name)					\
 	EXPORT_STATIC_CALL_QUERY(name,);				\
 	EXPORT_SYMBOL(STATIC_CALL_TRAMP(name))
-#define EXPORT_STATIC_CALL_TRAMP_GPL(name)				\
+#define EXPORT_STATIC_CALL_GPL(name)					\
 	EXPORT_STATIC_CALL_QUERY(name, _GPL);				\
 	EXPORT_SYMBOL_GPL(STATIC_CALL_TRAMP(name))

@@ -301,6 +305,9 @@ static inline int static_call_text_reserved(void *start, void *end)
 #define EXPORT_STATIC_CALL(name)	EXPORT_SYMBOL(STATIC_CALL_KEY(name))
 #define EXPORT_STATIC_CALL_GPL(name)	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name))
 
+#define EXPORT_STATIC_CALL_FOR_UPDATE(name)	EXPORT_SYMBOL(STATIC_CALL_KEY(name))
+#define EXPORT_STATIC_CALL_FOR_UPDATE_GPL(name)	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name))
+
 #endif /* CONFIG_HAVE_STATIC_CALL */
 
 #define DEFINE_STATIC_CALL(name, _func)					\

kernel/sched/core.c +4 -4

index 523fd602ea90..0e963d77176f 100644
@@ -6445,7 +6445,7 @@ EXPORT_SYMBOL(preempt_schedule);
 
 #ifdef CONFIG_PREEMPT_DYNAMIC
 DEFINE_STATIC_CALL(preempt_schedule, __preempt_schedule_func);
-EXPORT_STATIC_CALL_TRAMP(preempt_schedule);
+EXPORT_STATIC_CALL(preempt_schedule);
 #endif

@@ -6503,7 +6503,7 @@ EXPORT_SYMBOL_GPL(preempt_schedule_notrace);
 
 #ifdef CONFIG_PREEMPT_DYNAMIC
 DEFINE_STATIC_CALL(preempt_schedule_notrace, __preempt_schedule_notrace_func);
-EXPORT_STATIC_CALL_TRAMP(preempt_schedule_notrace);
+EXPORT_STATIC_CALL(preempt_schedule_notrace);
 #endif
 
 #endif /* CONFIG_PREEMPTION */

@@ -8150,10 +8150,10 @@ EXPORT_SYMBOL(__cond_resched);
 
 #ifdef CONFIG_PREEMPT_DYNAMIC
 DEFINE_STATIC_CALL_RET0(cond_resched, __cond_resched);
-EXPORT_STATIC_CALL_TRAMP(cond_resched);
+EXPORT_STATIC_CALL(cond_resched);
 
 DEFINE_STATIC_CALL_RET0(might_resched, __cond_resched);
-EXPORT_STATIC_CALL_TRAMP(might_resched);
+EXPORT_STATIC_CALL(might_resched);
 #endif
 
 /*

Signature · 1 line

2.30.2

Peter Zijlstra

maintainer

2021-11-09 19:00 UTC #19 lore raw

Re: [RFC PATCH 6/7] static_call: rename EXPORT_ macros to be more self-explanatory

Quoted 5 lines · Ard Biesheuvel

So let's rename these macros to
EXPORT_STATIC_CALL -> EXPORT_STATIC_CALL_FOR_UPDATE
EXPORT_STATIC_CALL_TRAMP -> EXPORT_STATIC_CALL

Ok, let's pain this shed a bit.

How about:

EXPORT_STATIC_CALL_RW
EXPORT_STATIC_CALL_RO

respectively. OR.. alternatively, have both:

EXPORT_STATIC_CALL_KEY
EXPORT_STATIC_CALL_TRAMP

and those that want to export both get to use both.

Ard Biesheuvel

author reviewer

2021-11-09 19:04 UTC #20 lore raw

Re: [RFC PATCH 6/7] static_call: rename EXPORT_ macros to be more self-explanatory

Quoted 8 lines · Peter Zijlstra

Quoted 5 lines · Ard Biesheuvel
So let's rename these macros to
EXPORT_STATIC_CALL -> EXPORT_STATIC_CALL_FOR_UPDATE
EXPORT_STATIC_CALL_TRAMP -> EXPORT_STATIC_CALL

Quoted 7 lines

Ok, let's pain this shed a bit.
How about:
EXPORT_STATIC_CALL_RW
EXPORT_STATIC_CALL_RO

Works for me

Quoted 6 lines

respectively. OR.. alternatively, have both:
EXPORT_STATIC_CALL_KEY
EXPORT_STATIC_CALL_TRAMP
and those that want to export both get to use both.

Yeah but whether a trampoline even exists is an implementation detail, which I would like to omit from the API.

Ard Biesheuvel

author reviewer

2021-11-09 16:46 UTC #21 lore raw

RFC PATCH 7/7 static_call: add generic support for non-exported keys

In reply to Ard Biesheuvel

The static_call feature where only the static call target is exposed to modules while access to the key is withheld is only implemented for the inline and out-of-line varieties, but not for the generic indirect call based variety.

Let's use static_call_query() to obtain the call target address if the key is not exported. Note that this means we can do the converse as well, i.e., implement static_call_query() in terms of the key struct if the key is exported, so we only need to export one of the two.

Note that this adds an additional load and potentially a direct call to the instantiations of static_call residing in module code.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 include/linux/static_call.h             | 4 ++--
 include/linux/static_call_types.h       | 3 +--
 tools/include/linux/static_call_types.h | 3 +--
 3 files changed, 4 insertions(+), 6 deletions(-)

include/linux/static_call.h +2 -2

index 21929147879a..d88fd613f06d 100644
@@ -302,8 +302,8 @@ static inline int static_call_text_reserved(void *start, void *end)
 	return 0;
 }
 
-#define EXPORT_STATIC_CALL(name)	EXPORT_SYMBOL(STATIC_CALL_KEY(name))
-#define EXPORT_STATIC_CALL_GPL(name)	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name))
+#define EXPORT_STATIC_CALL(name)	EXPORT_STATIC_CALL_QUERY(name,)
+#define EXPORT_STATIC_CALL_GPL(name)	EXPORT_STATIC_CALL_QUERY(name, _GPL)
 
 #define EXPORT_STATIC_CALL_FOR_UPDATE(name)	EXPORT_SYMBOL(STATIC_CALL_KEY(name))
 #define EXPORT_STATIC_CALL_FOR_UPDATE_GPL(name)	EXPORT_SYMBOL_GPL(STATIC_CALL_KEY(name))

include/linux/static_call_types.h +1 -2

index eae11c5b015d..920828ad1d69 100644
@@ -106,8 +106,7 @@ struct static_call_key {
 	void *func;
 };
 
-#define static_call(name)						\
-	((typeof(STATIC_CALL_QUERY(name)()))(STATIC_CALL_KEY(name).func))
+#define static_call(name)	static_call_query(name)
 
 #endif /* CONFIG_HAVE_STATIC_CALL */

tools/include/linux/static_call_types.h +1 -2

index eae11c5b015d..920828ad1d69 100644
@@ -106,8 +106,7 @@ struct static_call_key {
 	void *func;
 };
 
-#define static_call(name)						\
-	((typeof(STATIC_CALL_QUERY(name)()))(STATIC_CALL_KEY(name).func))
+#define static_call(name)	static_call_query(name)
 
 #endif /* CONFIG_HAVE_STATIC_CALL */

Signature · 1 line

2.30.2