call_dest_name() does not get passed the file pointer of validate_call(), which means its invocation of insn_reloc() will always return NULL. Make it take a file pointer.

While at it, make sure call_dest_name() uses arch_dest_reloc_offset(), otherwise it gets the pv_ops[] offset wrong.

Fabricating an intentional warning shows the change; previously:

vmlinux.o: warning: objtool: __flush_tlb_all_noinstr+0x4: call to {dynamic}() leaves .noinstr.text section

now:

vmlinux.o: warning: objtool: __flush_tlb_all_noinstr+0x4: call to pv_ops[1]() leaves .noinstr.text section

Signed-off-by: Valentin Schneider vschneid@redhat.com Acked-by: Josh Poimboeuf jpoimboe@kernel.org --- tools/objtool/check.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 76060da755b5c..b35763f05a548 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -3448,7 +3448,7 @@ static inline bool func_uaccess_safe(struct symbol *func) return false; }

-static inline const char *call_dest_name(struct instruction *insn) +static inline const char *call_dest_name(struct objtool_file *file, struct instruction *insn) { static char pvname[19]; struct reloc *reloc; @@ -3457,9 +3457,9 @@ static inline const char *call_dest_name(struct instruction *insn) if (insn_call_dest(insn)) return insn_call_dest(insn)->name;

- reloc = insn_reloc(NULL, insn); + reloc = insn_reloc(file, insn); if (reloc && !strcmp(reloc->sym->name, "pv_ops")) { - idx = (reloc_addend(reloc) / sizeof(void *)); + idx = (arch_dest_reloc_offset(reloc_addend(reloc)) / sizeof(void *)); snprintf(pvname, sizeof(pvname), "pv_ops[%d]", idx); return pvname; } @@ -3538,17 +3538,19 @@ static int validate_call(struct objtool_file *file, { if (state->noinstr && state->instr <= 0 && !noinstr_call_dest(file, insn, insn_call_dest(insn))) { - WARN_INSN(insn, "call to %s() leaves .noinstr.text section", call_dest_name(insn)); + WARN_INSN(insn, "call to %s() leaves .noinstr.text section", call_dest_name(file, insn)); return 1; }

if (state->uaccess && !func_uaccess_safe(insn_call_dest(insn))) { - WARN_INSN(insn, "call to %s() with UACCESS enabled", call_dest_name(insn)); + WARN_INSN(insn, "call to %s() with UACCESS enabled", + call_dest_name(file, insn)); return 1; }

if (state->df) { - WARN_INSN(insn, "call to %s() with DF set", call_dest_name(insn)); + WARN_INSN(insn, "call to %s() with DF set", + call_dest_name(file, insn)); return 1; }

A later commit will reduce the size of the RCU watching counter to free up some bits for another purpose. Paul suggested adding a config option to test the extreme case where the counter is reduced to its minimum usable width for rcutorture to poke at, so do that.

Make it only configurable under RCU_EXPERT. While at it, add a comment to explain the layout of context_tracking->state.

Link: http://lore.kernel.org/r/4c2cb573-168f-4806-b1d9-164e8276e66a@paulmck-laptop Suggested-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Valentin Schneider vschneid@redhat.com Reviewed-by: Paul E. McKenney paulmck@kernel.org --- include/linux/context_tracking_state.h | 44 ++++++++++++++++++++++---- kernel/rcu/Kconfig.debug | 15 +++++++++ 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/include/linux/context_tracking_state.h b/include/linux/context_tracking_state.h index 7b8433d5a8efe..0b81248aa03e2 100644 --- a/include/linux/context_tracking_state.h +++ b/include/linux/context_tracking_state.h @@ -18,12 +18,6 @@ enum ctx_state { CT_STATE_MAX = 4, };

-/* Odd value for watching, else even. */ -#define CT_RCU_WATCHING CT_STATE_MAX - -#define CT_STATE_MASK (CT_STATE_MAX - 1) -#define CT_RCU_WATCHING_MASK (~CT_STATE_MASK) - struct context_tracking { #ifdef CONFIG_CONTEXT_TRACKING_USER /* @@ -44,9 +38,45 @@ struct context_tracking { #endif };

+/* + * We cram two different things within the same atomic variable: + * + * CT_RCU_WATCHING_START CT_STATE_START + * | | + * v v + * MSB [ RCU watching counter ][ context_state ] LSB + * ^ ^ + * | | + * CT_RCU_WATCHING_END CT_STATE_END + * + * Bits are used from the LSB upwards, so unused bits (if any) will always be in + * upper bits of the variable. + */ #ifdef CONFIG_CONTEXT_TRACKING +#define CT_SIZE (sizeof(((struct context_tracking *)0)->state) * BITS_PER_BYTE) + +#define CT_STATE_WIDTH bits_per(CT_STATE_MAX - 1) +#define CT_STATE_START 0 +#define CT_STATE_END (CT_STATE_START + CT_STATE_WIDTH - 1) + +#define CT_RCU_WATCHING_MAX_WIDTH (CT_SIZE - CT_STATE_WIDTH) +#define CT_RCU_WATCHING_WIDTH (IS_ENABLED(CONFIG_RCU_DYNTICKS_TORTURE) ? 2 : CT_RCU_WATCHING_MAX_WIDTH) +#define CT_RCU_WATCHING_START (CT_STATE_END + 1) +#define CT_RCU_WATCHING_END (CT_RCU_WATCHING_START + CT_RCU_WATCHING_WIDTH - 1) +#define CT_RCU_WATCHING BIT(CT_RCU_WATCHING_START) + +#define CT_STATE_MASK GENMASK(CT_STATE_END, CT_STATE_START) +#define CT_RCU_WATCHING_MASK GENMASK(CT_RCU_WATCHING_END, CT_RCU_WATCHING_START) + +#define CT_UNUSED_WIDTH (CT_RCU_WATCHING_MAX_WIDTH - CT_RCU_WATCHING_WIDTH) + +static_assert(CT_STATE_WIDTH + + CT_RCU_WATCHING_WIDTH + + CT_UNUSED_WIDTH == + CT_SIZE); + DECLARE_PER_CPU(struct context_tracking, context_tracking); -#endif +#endif /* CONFIG_CONTEXT_TRACKING */

#ifdef CONFIG_CONTEXT_TRACKING_USER static __always_inline int __ct_state(void) diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug index 9b0b52e1836fa..ea36953803a1e 100644 --- a/kernel/rcu/Kconfig.debug +++ b/kernel/rcu/Kconfig.debug @@ -168,4 +168,19 @@ config RCU_STRICT_GRACE_PERIOD when looking for certain types of RCU usage bugs, for example, too-short RCU read-side critical sections.

+ +config RCU_DYNTICKS_TORTURE + bool "Minimize RCU dynticks counter size" + depends on RCU_EXPERT && !COMPILE_TEST + default n + help + This option sets the width of the dynticks counter to its + minimum usable value. This minimum width greatly increases + the probability of flushing out bugs involving counter wrap, + but it also increases the probability of extending grace period + durations. This Kconfig option should therefore be avoided in + production due to the consequent increased probability of OOMs. + + This has no value for production and is only for testing. + endmenu # "RCU Debugging"

We now have an RCU_EXPERT config for testing small-sized RCU dynticks counter: CONFIG_RCU_DYNTICKS_TORTURE.

Modify scenario TREE04 to exercise to use this config in order to test a ridiculously small counter (2 bits).

Link: http://lore.kernel.org/r/4c2cb573-168f-4806-b1d9-164e8276e66a@paulmck-laptop Suggested-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Valentin Schneider vschneid@redhat.com Reviewed-by: Paul E. McKenney paulmck@kernel.org --- tools/testing/selftests/rcutorture/configs/rcu/TREE04 | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TREE04 b/tools/testing/selftests/rcutorture/configs/rcu/TREE04 index dc4985064b3ad..67caf4276bb01 100644 --- a/tools/testing/selftests/rcutorture/configs/rcu/TREE04 +++ b/tools/testing/selftests/rcutorture/configs/rcu/TREE04 @@ -16,3 +16,4 @@ CONFIG_DEBUG_OBJECTS_RCU_HEAD=n CONFIG_RCU_EXPERT=y CONFIG_RCU_EQS_DEBUG=y CONFIG_RCU_LAZY=y +CONFIG_RCU_DYNTICKS_TORTURE=y

From: Josh Poimboeuf jpoimboe@kernel.org

Deferring a code patching IPI is unsafe if the patched code is in a noinstr region. In that case the text poke code must trigger an immediate IPI to all CPUs, which can rudely interrupt an isolated NO_HZ CPU running in userspace.

Some noinstr static branches may really need to be patched at runtime, despite the resulting disruption. Add DEFINE_STATIC_KEY_*_NOINSTR() variants for those. They don't do anything special yet; that will come later.

Signed-off-by: Josh Poimboeuf jpoimboe@kernel.org --- include/linux/jump_label.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index f5a2727ca4a9a..88bb6e32fdcbc 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -385,6 +385,23 @@ struct static_key_false { #define DEFINE_STATIC_KEY_FALSE_RO(name) \ struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT

+/* + * The _NOINSTR variants are used to tell objtool the static key is allowed to + * be used in noinstr code. + * + * They should almost never be used, as they prevent code patching IPIs from + * being deferred, which can be problematic for isolated NOHZ_FULL CPUs running + * in pure userspace. + * + * If using one of these _NOINSTR variants, please add a comment above the + * definition with the rationale. + */ +#define DEFINE_STATIC_KEY_TRUE_NOINSTR(name) \ + DEFINE_STATIC_KEY_TRUE(name) + +#define DEFINE_STATIC_KEY_FALSE_NOINSTR(name) \ + DEFINE_STATIC_KEY_FALSE(name) + #define DECLARE_STATIC_KEY_FALSE(name) \ extern struct static_key_false name

Later commits will cause objtool to warn about static calls being used in .noinstr sections in order to safely defer instruction patching IPIs targeted at NOHZ_FULL CPUs.

pv_sched_clock is updated in: o __init vmware_paravirt_ops_setup() o __init xen_init_time_common() o kvm_sched_clock_init() <- __init kvmclock_init() o hv_setup_sched_clock() <- __init hv_init_tsc_clocksource()

IOW purely init context, and can thus be marked as __ro_after_init.

Reported-by: Josh Poimboeuf jpoimboe@kernel.org Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/kernel/paravirt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index fec3815335558..ae6675167877b 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -73,7 +73,7 @@ static u64 native_steal_clock(int cpu) }

DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock); -DEFINE_STATIC_CALL(pv_sched_clock, native_sched_clock); +DEFINE_STATIC_CALL_RO(pv_sched_clock, native_sched_clock);

void paravirt_set_sched_clock(u64 (*func)(void)) {

The static call is only ever updated in

__init pv_time_init() __init xen_init_time_common() __init vmware_paravirt_ops_setup() __init xen_time_setup_guest(

so mark it appropriately as __ro_after_init.

Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/kernel/paravirt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index ae6675167877b..a19b0002b18d8 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -72,7 +72,7 @@ static u64 native_steal_clock(int cpu) return 0; }

-DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock); +DEFINE_STATIC_CALL_RO(pv_steal_clock, native_steal_clock); DEFINE_STATIC_CALL_RO(pv_sched_clock, native_sched_clock);

void paravirt_set_sched_clock(u64 (*func)(void))

The static call is only ever updated in

__init pv_time_init() __init xen_time_setup_guest()

so mark it appropriately as __ro_after_init.

Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/arm64/kernel/paravirt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c index aa718d6a9274a..ad28fa23c9228 100644 --- a/arch/arm64/kernel/paravirt.c +++ b/arch/arm64/kernel/paravirt.c @@ -32,7 +32,7 @@ static u64 native_steal_clock(int cpu) return 0; }

-DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock); +DEFINE_STATIC_CALL_RO(pv_steal_clock, native_steal_clock);

struct pv_time_stolen_time_region { struct pvclock_vcpu_stolen_time __rcu *kaddr;

Later commits will cause objtool to warn about static calls being used in .noinstr sections in order to safely defer instruction patching IPIs targeted at NOHZ_FULL CPUs.

perf_lopwr_cb is used in .noinstr code, but is only ever updated in __init amd_brs_lopwr_init(), and can thus be marked as __ro_after_init.

Reported-by: Josh Poimboeuf jpoimboe@kernel.org Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/events/amd/brs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/brs.c b/arch/x86/events/amd/brs.c index 780acd3dff22a..e2ff03af15d82 100644 --- a/arch/x86/events/amd/brs.c +++ b/arch/x86/events/amd/brs.c @@ -422,7 +422,7 @@ void noinstr perf_amd_brs_lopwr_cb(bool lopwr_in) } }

-DEFINE_STATIC_CALL_NULL(perf_lopwr_cb, perf_amd_brs_lopwr_cb); +DEFINE_STATIC_CALL_NULL_RO(perf_lopwr_cb, perf_amd_brs_lopwr_cb); EXPORT_STATIC_CALL_TRAMP_GPL(perf_lopwr_cb);

void __init amd_brs_lopwr_init(void)

Later commits will cause objtool to warn about static keys being used in .noinstr sections in order to safely defer instruction patching IPIs targeted at NOHZ_FULL CPUs.

mds_idle_clear is used in .noinstr code, and can be modified at runtime (SMT hotplug). Suppressing the text_poke_sync() IPI has little benefits for this key, as hotplug implies eventually going through takedown_cpu() -> stop_machine_cpuslocked() which is going to cause interference on all online CPUs anyway.

Mark it to let objtool know not to warn about it.

Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/kernel/cpu/bugs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 47a01d4028f60..acad84dcfc3cd 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -113,8 +113,13 @@ DEFINE_STATIC_KEY_FALSE(switch_mm_cond_ibpb); /* Control unconditional IBPB in switch_mm() */ DEFINE_STATIC_KEY_FALSE(switch_mm_always_ibpb);

-/* Control MDS CPU buffer clear before idling (halt, mwait) */ -DEFINE_STATIC_KEY_FALSE(mds_idle_clear); +/* + * Control MDS CPU buffer clear before idling (halt, mwait) + * + * Allowed in .noinstr as this key is updated during hotplug which comes with + * more interference than just the text patching IPI. + */ +DEFINE_STATIC_KEY_FALSE_NOINSTR(mds_idle_clear); EXPORT_SYMBOL_GPL(mds_idle_clear);

/*

Later commits will cause objtool to warn about static keys being used in .noinstr sections in order to safely defer instruction patching IPIs targeted at NOHZ_FULL CPUs.

These keys are used in .noinstr code, and can be modified at runtime (/proc/kernel/vmx* write). However it is not expected that they will be flipped during latency-sensitive operations, and thus shouldn't be a source of interference wrt the text patching IPI.

Mark it to let objtool know not to warn about it.

Reported-by: Josh Poimboeuf jpoimboe@kernel.org Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/kvm/vmx/vmx.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 893366e537322..a028c38f44e02 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -225,8 +225,15 @@ module_param(pt_mode, int, S_IRUGO);

struct x86_pmu_lbr __ro_after_init vmx_lbr_caps;

-static DEFINE_STATIC_KEY_FALSE(vmx_l1d_should_flush); -static DEFINE_STATIC_KEY_FALSE(vmx_l1d_flush_cond); +/* + * Both of these static keys end up being used in .noinstr sections, however + * they are only modified: + * - at init + * - from a /proc/kernel/vmx* write + * thus during latency-sensitive operations they should remain stable. + */ +static DEFINE_STATIC_KEY_FALSE_NOINSTR(vmx_l1d_should_flush); +static DEFINE_STATIC_KEY_FALSE_NOINSTR(vmx_l1d_flush_cond); static DEFINE_MUTEX(vmx_l1d_flush_mutex);

/* Storage for pre module init parameter parsing */

From: Josh Poimboeuf jpoimboe@kernel.org

Warn about static branches/calls in noinstr regions, unless the corresponding key is RO-after-init or has been manually whitelisted with DEFINE_STATIC_KEY_*_NOINSTR(().

Signed-off-by: Josh Poimboeuf jpoimboe@kernel.org [Added NULL check for insn_call_dest() return value] Signed-off-by: Valentin Schneider vschneid@redhat.com --- include/linux/jump_label.h | 17 +++-- include/linux/objtool.h | 7 ++ include/linux/static_call.h | 3 + tools/objtool/Documentation/objtool.txt | 34 +++++++++ tools/objtool/check.c | 92 ++++++++++++++++++++++--- tools/objtool/include/objtool/check.h | 1 + tools/objtool/include/objtool/elf.h | 1 + tools/objtool/include/objtool/special.h | 1 + tools/objtool/special.c | 18 ++++- 9 files changed, 156 insertions(+), 18 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index 88bb6e32fdcbc..dc8a82a62c109 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -75,6 +75,7 @@

#include <linux/types.h> #include <linux/compiler.h> +#include <linux/objtool.h>

extern bool static_key_initialized;

@@ -373,8 +374,9 @@ struct static_key_false { #define DEFINE_STATIC_KEY_TRUE(name) \ struct static_key_true name = STATIC_KEY_TRUE_INIT

-#define DEFINE_STATIC_KEY_TRUE_RO(name) \ - struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT +#define DEFINE_STATIC_KEY_TRUE_RO(name) \ + struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT; \ + ANNOTATE_NOINSTR_ALLOWED(name)

#define DECLARE_STATIC_KEY_TRUE(name) \ extern struct static_key_true name @@ -382,8 +384,9 @@ struct static_key_false { #define DEFINE_STATIC_KEY_FALSE(name) \ struct static_key_false name = STATIC_KEY_FALSE_INIT

-#define DEFINE_STATIC_KEY_FALSE_RO(name) \ - struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT +#define DEFINE_STATIC_KEY_FALSE_RO(name) \ + struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT; \ + ANNOTATE_NOINSTR_ALLOWED(name)

/* * The _NOINSTR variants are used to tell objtool the static key is allowed to @@ -397,10 +400,12 @@ struct static_key_false { * definition with the rationale. */ #define DEFINE_STATIC_KEY_TRUE_NOINSTR(name) \ - DEFINE_STATIC_KEY_TRUE(name) + DEFINE_STATIC_KEY_TRUE(name); \ + ANNOTATE_NOINSTR_ALLOWED(name)

#define DEFINE_STATIC_KEY_FALSE_NOINSTR(name) \ - DEFINE_STATIC_KEY_FALSE(name) + DEFINE_STATIC_KEY_FALSE(name); \ + ANNOTATE_NOINSTR_ALLOWED(name)

#define DECLARE_STATIC_KEY_FALSE(name) \ extern struct static_key_false name diff --git a/include/linux/objtool.h b/include/linux/objtool.h index b3b8d3dab52d5..1a7389f273063 100644 --- a/include/linux/objtool.h +++ b/include/linux/objtool.h @@ -34,6 +34,12 @@ static void __used __section(".discard.func_stack_frame_non_standard") \ *__func_stack_frame_non_standard_##func = func

+#define __ANNOTATE_NOINSTR_ALLOWED(key) \ + static void __used __section(".discard.noinstr_allowed") \ + *__annotate_noinstr_allowed_##key = &key + +#define ANNOTATE_NOINSTR_ALLOWED(key) __ANNOTATE_NOINSTR_ALLOWED(key) + /* * STACK_FRAME_NON_STANDARD_FP() is a frame-pointer-specific function ignore * for the case where a function is intentionally missing frame pointer setup, @@ -157,6 +163,7 @@ #define STACK_FRAME_NON_STANDARD_FP(func) #define ANNOTATE_NOENDBR #define ASM_REACHABLE +#define ANNOTATE_NOINSTR_ALLOWED(key) #else #define ANNOTATE_INTRA_FUNCTION_CALL .macro UNWIND_HINT type:req sp_reg=0 sp_offset=0 signal=0 diff --git a/include/linux/static_call.h b/include/linux/static_call.h index ea6ca57e2a829..0d4b16d348501 100644 --- a/include/linux/static_call.h +++ b/include/linux/static_call.h @@ -133,6 +133,7 @@

#include <linux/types.h> #include <linux/cpu.h> +#include <linux/objtool.h> #include <linux/static_call_types.h>

#ifdef CONFIG_HAVE_STATIC_CALL @@ -198,6 +199,7 @@ extern long __static_call_return0(void); .func = _func, \ .type = 1, \ }; \ + ANNOTATE_NOINSTR_ALLOWED(STATIC_CALL_TRAMP(name)); \ ARCH_DEFINE_STATIC_CALL_TRAMP(name, _func)

#define DEFINE_STATIC_CALL_NULL(name, _func) \ @@ -214,6 +216,7 @@ extern long __static_call_return0(void); .func = NULL, \ .type = 1, \ }; \ + ANNOTATE_NOINSTR_ALLOWED(STATIC_CALL_TRAMP(name)); \ ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name)

#define DEFINE_STATIC_CALL_RET0(name, _func) \ diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt index 7c3ee959b63c7..922d3b41541d0 100644 --- a/tools/objtool/Documentation/objtool.txt +++ b/tools/objtool/Documentation/objtool.txt @@ -447,6 +447,40 @@ the objtool maintainers. names and does not use module_init() / module_exit() macros to create them.

+13. file.o: warning: func()+0x2a: key: non-RO static key usage in noinstr code + file.o: warning: func()+0x2a: key: non-RO static call usage in noinstr code + + This means that noinstr function func() uses a static key or + static call named 'key' which can be modified at runtime. This is + discouraged because it prevents code patching IPIs from being + deferred. + + You have the following options: + + 1) Check whether the static key/call in question is only modified + during init. If so, define it as read-only-after-init with + DEFINE_STATIC_KEY_*_RO() or DEFINE_STATIC_CALL_RO(). + + 2) Avoid the runtime patching. For static keys this can be done by + using static_key_enabled() or by getting rid of the static key + altogether if performance is not a concern. + + For static calls, something like the following could be done: + + target = static_call_query(foo); + if (target == func1) + func1(); + else if (target == func2) + func2(); + ... + + 3) Silence the warning by defining the static key/call with + DEFINE_STATIC_*_NOINSTR(). This decision should not + be taken lightly as it may result in code patching IPIs getting + sent to isolated NOHZ_FULL CPUs running in pure userspace. A + comment should be added above the definition explaining the + rationale for the decision. +

If the error doesn't seem to make sense, it could be a bug in objtool. Feel free to ask the objtool maintainer for help. diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 6aa9259fc9940..24219538c1587 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1068,6 +1068,45 @@ static int create_direct_call_sections(struct objtool_file *file) return 0; }

+static int read_noinstr_allowed(struct objtool_file *file) +{ + struct section *rsec; + struct symbol *sym; + struct reloc *reloc; + + rsec = find_section_by_name(file->elf, ".rela.discard.noinstr_allowed"); + if (!rsec) + return 0; + + for_each_reloc(rsec, reloc) { + switch (reloc->sym->type) { + case STT_OBJECT: + case STT_FUNC: + sym = reloc->sym; + break; + + case STT_SECTION: + sym = find_symbol_by_offset(reloc->sym->sec, + reloc_addend(reloc)); + if (!sym) { + WARN_FUNC("can't find static key/call symbol", + reloc->sym->sec, reloc_addend(reloc)); + return -1; + } + break; + + default: + WARN("unexpected relocation symbol type in %s: %d", + rsec->name, reloc->sym->type); + return -1; + } + + sym->noinstr_allowed = 1; + } + + return 0; +} + /* * Warnings shouldn't be reported for ignored functions. */ @@ -1955,6 +1994,8 @@ static int handle_jump_alt(struct objtool_file *file, return -1; }

+ orig_insn->key = special_alt->key; + if (opts.hack_jump_label && special_alt->key_addend & 2) { struct reloc *reloc = insn_reloc(file, orig_insn);

@@ -2731,6 +2772,10 @@ static int decode_sections(struct objtool_file *file) if (ret) return ret;

+ ret = read_noinstr_allowed(file); + if (ret) + return ret; + return 0; }

@@ -3494,9 +3539,9 @@ static bool pv_call_dest(struct objtool_file *file, struct instruction *insn) return file->pv_ops[idx].clean; }

-static inline bool noinstr_call_dest(struct objtool_file *file, - struct instruction *insn, - struct symbol *func) +static inline bool noinstr_call_allowed(struct objtool_file *file, + struct instruction *insn, + struct symbol *func) { /* * We can't deal with indirect function calls at present; @@ -3516,10 +3561,10 @@ static inline bool noinstr_call_dest(struct objtool_file *file, return true;

/* - * If the symbol is a static_call trampoline, we can't tell. + * Only DEFINE_STATIC_CALL_*_RO allowed. */ if (func->static_call_tramp) - return true; + return func->noinstr_allowed;

/* * The __ubsan_handle_*() calls are like WARN(), they only happen when @@ -3532,14 +3577,29 @@ static inline bool noinstr_call_dest(struct objtool_file *file, return false; }

+static char *static_call_name(struct symbol *func) +{ + return func->name + strlen("__SCT__"); +} + static int validate_call(struct objtool_file *file, struct instruction *insn, struct insn_state *state) { - if (state->noinstr && state->instr <= 0 && - !noinstr_call_dest(file, insn, insn_call_dest(insn))) { - WARN_INSN(insn, "call to %s() leaves .noinstr.text section", call_dest_name(file, insn)); - return 1; + if (state->noinstr && state->instr <= 0) { + struct symbol *dest = insn_call_dest(insn); + + if (dest && dest->static_call_tramp) { + if (!dest->noinstr_allowed) { + WARN_INSN(insn, "%s: non-RO static call usage in noinstr", + static_call_name(dest)); + } + + } else if (dest && !noinstr_call_allowed(file, insn, dest)) { + WARN_INSN(insn, "call to %s() leaves .noinstr.text section", + call_dest_name(file, insn)); + return 1; + } }

if (state->uaccess && !func_uaccess_safe(insn_call_dest(insn))) { @@ -3604,6 +3664,17 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct return 0; }

+static int validate_static_key(struct instruction *insn, struct insn_state *state) +{ + if (state->noinstr && state->instr <= 0 && !insn->key->noinstr_allowed) { + WARN_INSN(insn, "%s: non-RO static key usage in noinstr", + insn->key->name); + return 1; + } + + return 0; +} + static struct instruction *next_insn_to_validate(struct objtool_file *file, struct instruction *insn) { @@ -3765,6 +3836,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, if (handle_insn_ops(insn, next_insn, &state)) return 1;

+ if (insn->key) + validate_static_key(insn, &state); + switch (insn->type) {

case INSN_RETURN: diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h index daa46f1f0965a..c0da7246eac7b 100644 --- a/tools/objtool/include/objtool/check.h +++ b/tools/objtool/include/objtool/check.h @@ -77,6 +77,7 @@ struct instruction { struct symbol *sym; struct stack_op *stack_ops; struct cfi_state *cfi; + struct symbol *key; };

static inline struct symbol *insn_func(struct instruction *insn) diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h index d7e815c2fd156..0cb79931262bb 100644 --- a/tools/objtool/include/objtool/elf.h +++ b/tools/objtool/include/objtool/elf.h @@ -69,6 +69,7 @@ struct symbol { u8 embedded_insn : 1; u8 local_label : 1; u8 frame_pointer : 1; + u8 noinstr_allowed : 1; struct list_head pv_target; struct reloc *relocs; }; diff --git a/tools/objtool/include/objtool/special.h b/tools/objtool/include/objtool/special.h index 86d4af9c5aa9d..ce4759358ec48 100644 --- a/tools/objtool/include/objtool/special.h +++ b/tools/objtool/include/objtool/special.h @@ -20,6 +20,7 @@ struct special_alt { bool skip_alt; bool jump_or_nop; u8 key_addend; + struct symbol *key;

struct section *orig_sec; unsigned long orig_off; diff --git a/tools/objtool/special.c b/tools/objtool/special.c index 097a69db82a0e..982d5cb55e1bb 100644 --- a/tools/objtool/special.c +++ b/tools/objtool/special.c @@ -119,14 +119,26 @@ static int get_alt_entry(struct elf *elf, const struct special_entry *entry,

if (entry->key) { struct reloc *key_reloc; + struct symbol *key; + s64 key_addend;

key_reloc = find_reloc_by_dest(elf, sec, offset + entry->key); if (!key_reloc) { - WARN_FUNC("can't find key reloc", - sec, offset + entry->key); + WARN_FUNC("can't find key reloc", sec, offset + entry->key); return -1; } - alt->key_addend = reloc_addend(key_reloc); + + key = key_reloc->sym; + key_addend = reloc_addend(key_reloc); + + if (key->type == STT_SECTION) + key = find_symbol_by_offset(key->sec, key_addend & ~3); + + /* embedded keys not supported */ + if (key) { + alt->key = key; + alt->key_addend = key_addend; + } }

return 0;

ct_nmi_{enter, exit}() only touches the RCU watching counter and doesn't modify the actual CT state part context_tracking.state. This means that upon receiving an IRQ when idle, the CT_STATE_IDLE->CT_STATE_KERNEL transition only happens in ct_idle_exit().

One can note that ct_nmi_enter() can only ever be entered with the CT state as either CT_STATE_KERNEL or CT_STATE_IDLE, as an IRQ/NMI happenning in the CT_STATE_USER or CT_STATE_GUEST states will be routed down to ct_user_exit().

Add/remove CT_STATE_IDLE from the context tracking state as needed in ct_nmi_{enter, exit}().

Note that this leaves the following window where the CPU is executing code in kernelspace, but the context tracking state is CT_STATE_IDLE:

~> IRQ ct_nmi_enter() state = state + CT_STATE_KERNEL - CT_STATE_IDLE

[...]

ct_nmi_exit() state = state - CT_STATE_KERNEL + CT_STATE_IDLE

[...] /!\ CT_STATE_IDLE here while we're really in kernelspace! /!\

ct_cpuidle_exit() state = state + CT_STATE_KERNEL - CT_STATE_IDLE

Signed-off-by: Valentin Schneider vschneid@redhat.com --- kernel/context_tracking.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c index a61498a8425e2..15f10ddec8cbe 100644 --- a/kernel/context_tracking.c +++ b/kernel/context_tracking.c @@ -236,7 +236,9 @@ void noinstr ct_nmi_exit(void) instrumentation_end();

// RCU is watching here ... - ct_kernel_exit_state(CT_RCU_WATCHING); + ct_kernel_exit_state(CT_RCU_WATCHING - + CT_STATE_KERNEL + + CT_STATE_IDLE); // ... but is no longer watching here.

if (!in_nmi()) @@ -259,6 +261,7 @@ void noinstr ct_nmi_enter(void) { long incby = 2; struct context_tracking *ct = this_cpu_ptr(&context_tracking); + int curr_state;

/* Complain about underflow. */ WARN_ON_ONCE(ct_nmi_nesting() < 0); @@ -271,13 +274,26 @@ void noinstr ct_nmi_enter(void) * to be in the outermost NMI handler that interrupted an RCU-idle * period (observation due to Andy Lutomirski). */ - if (!rcu_is_watching_curr_cpu()) { + curr_state = raw_atomic_read(this_cpu_ptr(&context_tracking.state)); + if (!(curr_state & CT_RCU_WATCHING)) {

if (!in_nmi()) rcu_task_enter();

+ /* + * RCU isn't watching, so we're one of + * CT_STATE_IDLE + * CT_STATE_USER + * CT_STATE_GUEST + * guest/user entry is handled by ct_user_enter(), so this has + * to be idle entry. + */ + WARN_ON_ONCE((curr_state & CT_STATE_MASK) != CT_STATE_IDLE); + // RCU is not watching here ... - ct_kernel_enter_state(CT_RCU_WATCHING); + ct_kernel_enter_state(CT_RCU_WATCHING + + CT_STATE_KERNEL - + CT_STATE_IDLE); // ... but is watching here.

instrumentation_begin();

smp_call_function() & friends have the unfortunate habit of sending IPIs to isolated, NOHZ_FULL, in-userspace CPUs, as they blindly target all online CPUs.

Some callsites can be bent into doing the right, such as done by commit:

cc9e303c91f5 ("x86/cpu: Disable frequency requests via aperfmperf IPI for nohz_full CPUs")

Unfortunately, not all SMP callbacks can be omitted in this fashion. However, some of them only affect execution in kernelspace, which means they don't have to be executed *immediately* if the target CPU is in userspace: stashing the callback and executing it upon the next kernel entry would suffice. x86 kernel instruction patching or kernel TLB invalidation are prime examples of it.

Reduce the RCU dynticks counter width to free up some bits to be used as a deferred callback bitmask. Add some build-time checks to validate that setup.

Presence of CT_STATE_KERNEL in the ct_state prevents queuing deferred work.

Later commits introduce the bit:callback mappings.

Link: https://lore.kernel.org/all/20210929151723.162004989@infradead.org/ Signed-off-by: Nicolas Saenz Julienne nsaenzju@redhat.com Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/Kconfig | 9 +++ arch/x86/Kconfig | 1 + arch/x86/include/asm/context_tracking_work.h | 16 +++++ include/linux/context_tracking.h | 21 +++++++ include/linux/context_tracking_state.h | 30 ++++++--- include/linux/context_tracking_work.h | 26 ++++++++ kernel/context_tracking.c | 66 +++++++++++++++++++- kernel/time/Kconfig | 5 ++ 8 files changed, 162 insertions(+), 12 deletions(-) create mode 100644 arch/x86/include/asm/context_tracking_work.h create mode 100644 include/linux/context_tracking_work.h

diff --git a/arch/Kconfig b/arch/Kconfig index 6682b2a53e342..b637f20f0fc68 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -952,6 +952,15 @@ config HAVE_CONTEXT_TRACKING_USER_OFFSTACK - No use of instrumentation, unless instrumentation_begin() got called.

+config HAVE_CONTEXT_TRACKING_WORK + bool + help + Architecture supports deferring work while not in kernel context. + This is especially useful on setups with isolated CPUs that might + want to avoid being interrupted to perform housekeeping tasks (for + ex. TLB invalidation or icache invalidation). The housekeeping + operations are performed upon re-entering the kernel. + config HAVE_TIF_NOHZ bool help diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 9d7bd0ae48c42..ca5dd4cfc354b 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -216,6 +216,7 @@ config X86 select HAVE_CMPXCHG_LOCAL select HAVE_CONTEXT_TRACKING_USER if X86_64 select HAVE_CONTEXT_TRACKING_USER_OFFSTACK if HAVE_CONTEXT_TRACKING_USER + select HAVE_CONTEXT_TRACKING_WORK if X86_64 select HAVE_C_RECORDMCOUNT select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL select HAVE_OBJTOOL_NOP_MCOUNT if HAVE_OBJTOOL_MCOUNT diff --git a/arch/x86/include/asm/context_tracking_work.h b/arch/x86/include/asm/context_tracking_work.h new file mode 100644 index 0000000000000..5f3b2d0977235 --- /dev/null +++ b/arch/x86/include/asm/context_tracking_work.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_CONTEXT_TRACKING_WORK_H +#define _ASM_X86_CONTEXT_TRACKING_WORK_H + +static __always_inline void arch_context_tracking_work(enum ct_work work) +{ + switch (work) { + case CT_WORK_n: + // Do work... + break; + case CT_WORK_MAX: + WARN_ON_ONCE(true); + } +} + +#endif diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h index af9fe87a09225..0b0faa040e9b5 100644 --- a/include/linux/context_tracking.h +++ b/include/linux/context_tracking.h @@ -5,6 +5,7 @@ #include <linux/sched.h> #include <linux/vtime.h> #include <linux/context_tracking_state.h> +#include <linux/context_tracking_work.h> #include <linux/instrumentation.h>

#include <asm/ptrace.h> @@ -137,6 +138,26 @@ static __always_inline unsigned long ct_state_inc(int incby) return raw_atomic_add_return(incby, this_cpu_ptr(&context_tracking.state)); }

+#ifdef CONFIG_CONTEXT_TRACKING_WORK +static __always_inline unsigned long ct_state_inc_clear_work(int incby) +{ + struct context_tracking *ct = this_cpu_ptr(&context_tracking); + unsigned long new, old, state; + + state = arch_atomic_read(&ct->state); + do { + old = state; + new = old & ~CT_WORK_MASK; + new += incby; + state = arch_atomic_cmpxchg(&ct->state, old, new); + } while (old != state); + + return new; +} +#else +#define ct_state_inc_clear_work(x) ct_state_inc(x) +#endif + static __always_inline bool warn_rcu_enter(void) { bool ret = false; diff --git a/include/linux/context_tracking_state.h b/include/linux/context_tracking_state.h index eb2149b20baef..b6ddfccba9714 100644 --- a/include/linux/context_tracking_state.h +++ b/include/linux/context_tracking_state.h @@ -5,6 +5,7 @@ #include <linux/percpu.h> #include <linux/static_key.h> #include <linux/context_tracking_irq.h> +#include <linux/context_tracking_work.h>

/* Offset to allow distinguishing irq vs. task-based idle entry/exit. */ #define CT_NESTING_IRQ_NONIDLE ((LONG_MAX / 2) + 1) @@ -39,16 +40,19 @@ struct context_tracking { };

/* - * We cram two different things within the same atomic variable: + * We cram up to three different things within the same atomic variable: * - * CT_RCU_WATCHING_START CT_STATE_START - * | | - * v v - * MSB [ RCU watching counter ][ context_state ] LSB - * ^ ^ - * | | - * CT_RCU_WATCHING_END CT_STATE_END + * CT_RCU_WATCHING_START CT_STATE_START + * | CT_WORK_START | + * | | | + * v v v + * MSB [ RCU watching counter ][ context work ][ context_state ] LSB + * ^ ^ ^ + * | | | + * | CT_WORK_END | + * CT_RCU_WATCHING_END CT_STATE_END * + * The [ context work ] region spans 0 bits if CONFIG_CONTEXT_WORK=n * Bits are used from the LSB upwards, so unused bits (if any) will always be in * upper bits of the variable. */ @@ -59,18 +63,24 @@ struct context_tracking { #define CT_STATE_START 0 #define CT_STATE_END (CT_STATE_START + CT_STATE_WIDTH - 1)

-#define CT_RCU_WATCHING_MAX_WIDTH (CT_SIZE - CT_STATE_WIDTH) +#define CT_WORK_WIDTH (IS_ENABLED(CONFIG_CONTEXT_TRACKING_WORK) ? CT_WORK_MAX_OFFSET : 0) +#define CT_WORK_START (CT_STATE_END + 1) +#define CT_WORK_END (CT_WORK_START + CT_WORK_WIDTH - 1) + +#define CT_RCU_WATCHING_MAX_WIDTH (CT_SIZE - CT_WORK_WIDTH - CT_STATE_WIDTH) #define CT_RCU_WATCHING_WIDTH (IS_ENABLED(CONFIG_RCU_DYNTICKS_TORTURE) ? 2 : CT_RCU_WATCHING_MAX_WIDTH) -#define CT_RCU_WATCHING_START (CT_STATE_END + 1) +#define CT_RCU_WATCHING_START (CT_WORK_END + 1) #define CT_RCU_WATCHING_END (CT_RCU_WATCHING_START + CT_RCU_WATCHING_WIDTH - 1) #define CT_RCU_WATCHING BIT(CT_RCU_WATCHING_START)

#define CT_STATE_MASK GENMASK(CT_STATE_END, CT_STATE_START) +#define CT_WORK_MASK GENMASK(CT_WORK_END, CT_WORK_START) #define CT_RCU_WATCHING_MASK GENMASK(CT_RCU_WATCHING_END, CT_RCU_WATCHING_START)

#define CT_UNUSED_WIDTH (CT_RCU_WATCHING_MAX_WIDTH - CT_RCU_WATCHING_WIDTH)

static_assert(CT_STATE_WIDTH + + CT_WORK_WIDTH + CT_RCU_WATCHING_WIDTH + CT_UNUSED_WIDTH == CT_SIZE); diff --git a/include/linux/context_tracking_work.h b/include/linux/context_tracking_work.h new file mode 100644 index 0000000000000..c68245f8d77c5 --- /dev/null +++ b/include/linux/context_tracking_work.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_CONTEXT_TRACKING_WORK_H +#define _LINUX_CONTEXT_TRACKING_WORK_H + +#include <linux/bitops.h> + +enum { + CT_WORK_n_OFFSET, + CT_WORK_MAX_OFFSET +}; + +enum ct_work { + CT_WORK_n = BIT(CT_WORK_n_OFFSET), + CT_WORK_MAX = BIT(CT_WORK_MAX_OFFSET) +}; + +#include <asm/context_tracking_work.h> + +#ifdef CONFIG_CONTEXT_TRACKING_WORK +extern bool ct_set_cpu_work(unsigned int cpu, enum ct_work work); +#else +static inline bool +ct_set_cpu_work(unsigned int cpu, unsigned int work) { return false; } +#endif + +#endif diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c index 15f10ddec8cbe..f7019c12269f9 100644 --- a/kernel/context_tracking.c +++ b/kernel/context_tracking.c @@ -72,6 +72,67 @@ static __always_inline void rcu_task_trace_heavyweight_exit(void) #endif /* #ifdef CONFIG_TASKS_TRACE_RCU */ }

+#ifdef CONFIG_CONTEXT_TRACKING_WORK +static noinstr void ct_work_flush(unsigned long seq) +{ + int bit; + + seq = (seq & CT_WORK_MASK) >> CT_WORK_START; + + /* + * arch_context_tracking_work() must be noinstr, non-blocking, + * and NMI safe. + */ + for_each_set_bit(bit, &seq, CT_WORK_MAX) + arch_context_tracking_work(BIT(bit)); +} + +/** + * ct_set_cpu_work - set work to be run at next kernel context entry + * + * If @cpu is not currently executing in kernelspace, it will execute the + * callback mapped to @work (see arch_context_tracking_work()) at its next + * transition to CT_KERNEL_STATE. + * + * If it is already in CT_KERNEL_STATE, this will be a no-op. + */ +bool ct_set_cpu_work(unsigned int cpu, enum ct_work work) +{ + struct context_tracking *ct = per_cpu_ptr(&context_tracking, cpu); + unsigned int old; + bool ret = false; + + if (!ct->active) + return false; + + preempt_disable(); + + old = atomic_read(&ct->state); + + /* + * We only want to set the work bit if the target CPU is not in + * kernelspace, so we clear the KERNEL bit here and let the cmpxchg do + * the check for us - the state could change between the atomic_read() and + * the cmpxchg(). + */ + old &= ~CT_STATE_KERNEL; + /* + * Try setting the work until either + * - the target CPU has entered kernelspace + * - the work has been set + */ + do { + ret = atomic_try_cmpxchg(&ct->state, &old, old | (work << CT_WORK_START)); + } while (!ret && ((old & CT_STATE_MASK) != CT_STATE_KERNEL)); + + preempt_enable(); + return ret; +} +#else +static __always_inline void ct_work_flush(unsigned long work) { } +static __always_inline void ct_work_clear(struct context_tracking *ct) { } +#endif + /* * Record entry into an extended quiescent state. This is only to be * called when not already in an extended quiescent state, that is, @@ -88,7 +149,7 @@ static noinstr void ct_kernel_exit_state(int offset) * next idle sojourn. */ rcu_task_trace_heavyweight_enter(); // Before CT state update! - seq = ct_state_inc(offset); + seq = ct_state_inc_clear_work(offset); // RCU is no longer watching. Better be in extended quiescent state! WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && (seq & CT_RCU_WATCHING)); } @@ -100,7 +161,7 @@ static noinstr void ct_kernel_exit_state(int offset) */ static noinstr void ct_kernel_enter_state(int offset) { - int seq; + unsigned long seq;

/* * CPUs seeing atomic_add_return() must see prior idle sojourns, @@ -108,6 +169,7 @@ static noinstr void ct_kernel_enter_state(int offset) * critical section. */ seq = ct_state_inc(offset); + ct_work_flush(seq); // RCU is now watching. Better not be in an extended quiescent state! rcu_task_trace_heavyweight_exit(); // After CT state update! WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !(seq & CT_RCU_WATCHING)); diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig index b0b97a60aaa6f..7e8106a0d981f 100644 --- a/kernel/time/Kconfig +++ b/kernel/time/Kconfig @@ -181,6 +181,11 @@ config CONTEXT_TRACKING_USER_FORCE Say N otherwise, this option brings an overhead that you don't want in production.

+config CONTEXT_TRACKING_WORK + bool + depends on HAVE_CONTEXT_TRACKING_WORK && CONTEXT_TRACKING_USER + default y + config NO_HZ bool "Old Idle dynticks config" help

From: Peter Zijlstra peterz@infradead.org

Later patches will require issuing a __flush_tlb_all() from noinstr code. This requires making both __flush_tlb_local() and __flush_tlb_global() noinstr-compliant.

For __flush_tlb_global(), both native_flush_tlb_global() and xen_flush_tlb() need to be made noinstr.

Forgo using __native_flush_tlb_global() / native_write_cr4() and have the ASM directly inlined in the native function. For the Xen stuff, __always_inline a handful of helpers.

Not-signed-off-by: Peter Zijlstra peterz@infradead.org [Changelog faff] Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/include/asm/invpcid.h | 13 ++++++------- arch/x86/include/asm/paravirt.h | 2 +- arch/x86/include/asm/xen/hypercall.h | 11 +++++++++-- arch/x86/mm/tlb.c | 15 +++++++++++---- arch/x86/xen/mmu_pv.c | 10 +++++----- arch/x86/xen/xen-ops.h | 12 ++++++++---- 6 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h index 734482afbf81d..ff26136fcd9c6 100644 --- a/arch/x86/include/asm/invpcid.h +++ b/arch/x86/include/asm/invpcid.h @@ -2,7 +2,7 @@ #ifndef _ASM_X86_INVPCID #define _ASM_X86_INVPCID

-static inline void __invpcid(unsigned long pcid, unsigned long addr, +static __always_inline void __invpcid(unsigned long pcid, unsigned long addr, unsigned long type) { struct { u64 d[2]; } desc = { { pcid, addr } }; @@ -13,7 +13,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr, * mappings, we don't want the compiler to reorder any subsequent * memory accesses before the TLB flush. */ - asm volatile("invpcid %[desc], %[type]" + asm_inline volatile("invpcid %[desc], %[type]" :: [desc] "m" (desc), [type] "r" (type) : "memory"); }

@@ -23,26 +23,25 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr, #define INVPCID_TYPE_ALL_NON_GLOBAL 3

/* Flush all mappings for a given pcid and addr, not including globals. */ -static inline void invpcid_flush_one(unsigned long pcid, - unsigned long addr) +static __always_inline void invpcid_flush_one(unsigned long pcid, unsigned long addr) { __invpcid(pcid, addr, INVPCID_TYPE_INDIV_ADDR); }

/* Flush all mappings for a given PCID, not including globals. */ -static inline void invpcid_flush_single_context(unsigned long pcid) +static __always_inline void invpcid_flush_single_context(unsigned long pcid) { __invpcid(pcid, 0, INVPCID_TYPE_SINGLE_CTXT); }

/* Flush all mappings, including globals, for all PCIDs. */ -static inline void invpcid_flush_all(void) +static __always_inline void invpcid_flush_all(void) { __invpcid(0, 0, INVPCID_TYPE_ALL_INCL_GLOBAL); }

/* Flush all mappings for all PCIDs except globals. */ -static inline void invpcid_flush_all_nonglobals(void) +static __always_inline void invpcid_flush_all_nonglobals(void) { __invpcid(0, 0, INVPCID_TYPE_ALL_NON_GLOBAL); } diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index d4eb9e1d61b8e..b3daee3d46677 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -75,7 +75,7 @@ static inline void __flush_tlb_local(void) PVOP_VCALL0(mmu.flush_tlb_user); }

-static inline void __flush_tlb_global(void) +static __always_inline void __flush_tlb_global(void) { PVOP_VCALL0(mmu.flush_tlb_kernel); } diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h index 97771b9d33af3..291e9f8006f62 100644 --- a/arch/x86/include/asm/xen/hypercall.h +++ b/arch/x86/include/asm/xen/hypercall.h @@ -365,8 +365,8 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req, trace_xen_mc_entry(mcl, 4); }

-static inline void -MULTI_mmuext_op(struct multicall_entry *mcl, struct mmuext_op *op, int count, +static __always_inline void +__MULTI_mmuext_op(struct multicall_entry *mcl, struct mmuext_op *op, int count, int *success_count, domid_t domid) { mcl->op = __HYPERVISOR_mmuext_op; @@ -374,6 +374,13 @@ MULTI_mmuext_op(struct multicall_entry *mcl, struct mmuext_op *op, int count, mcl->args[1] = count; mcl->args[2] = (unsigned long)success_count; mcl->args[3] = domid; +} + +static inline void +MULTI_mmuext_op(struct multicall_entry *mcl, struct mmuext_op *op, int count, + int *success_count, domid_t domid) +{ + __MULTI_mmuext_op(mcl, op, count, success_count, domid);

trace_xen_mc_entry(mcl, 4); } diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c index a2becb85bea79..2d2ab3e221f0c 100644 --- a/arch/x86/mm/tlb.c +++ b/arch/x86/mm/tlb.c @@ -1169,9 +1169,10 @@ void flush_tlb_one_user(unsigned long addr) /* * Flush everything */ -STATIC_NOPV void native_flush_tlb_global(void) +STATIC_NOPV noinstr void native_flush_tlb_global(void) { unsigned long flags; + unsigned long cr4;

if (static_cpu_has(X86_FEATURE_INVPCID)) { /* @@ -1190,9 +1191,15 @@ STATIC_NOPV void native_flush_tlb_global(void) * be called from deep inside debugging code.) */ raw_local_irq_save(flags); - - __native_tlb_flush_global(this_cpu_read(cpu_tlbstate.cr4)); - + cr4 = this_cpu_read(cpu_tlbstate.cr4); + asm volatile("mov %0,%%cr4": : "r" (cr4 ^ X86_CR4_PGE) : "memory"); + asm volatile("mov %0,%%cr4": : "r" (cr4) : "memory"); + /* + * In lieu of not having the pinning crap, hard fail if CR4 doesn't + * match the expected value. This ensures that anybody doing dodgy gets + * the fallthrough check. + */ + BUG_ON(cr4 != this_cpu_read(cpu_tlbstate.cr4)); raw_local_irq_restore(flags); }

diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c index 55a4996d0c04f..4eb265eb867af 100644 --- a/arch/x86/xen/mmu_pv.c +++ b/arch/x86/xen/mmu_pv.c @@ -1231,22 +1231,22 @@ static noinstr void xen_write_cr2(unsigned long cr2) this_cpu_read(xen_vcpu)->arch.cr2 = cr2; }

-static noinline void xen_flush_tlb(void) +static noinline noinstr void xen_flush_tlb(void) { struct mmuext_op *op; struct multicall_space mcs;

- preempt_disable(); + preempt_disable_notrace();

mcs = xen_mc_entry(sizeof(*op));

op = mcs.args; op->cmd = MMUEXT_TLB_FLUSH_LOCAL; - MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF); + __MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF);

- xen_mc_issue(XEN_LAZY_MMU); + __xen_mc_issue(XEN_LAZY_MMU);

- preempt_enable(); + preempt_enable_notrace(); }

static void xen_flush_tlb_one_user(unsigned long addr) diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h index 63c13a2ccf556..effb1a54afbd1 100644 --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -235,15 +235,19 @@ static inline struct multicall_space xen_mc_entry(size_t args) void xen_mc_flush(void);

/* Issue a multicall if we're not in a lazy mode */ -static inline void xen_mc_issue(unsigned mode) +static __always_inline void __xen_mc_issue(unsigned mode) { - trace_xen_mc_issue(mode); - if ((xen_get_lazy_mode() & mode) == 0) xen_mc_flush();

/* restore flags saved in xen_mc_batch */ - local_irq_restore(this_cpu_read(xen_mc_irq_flags)); + raw_local_irq_restore(this_cpu_read(xen_mc_irq_flags)); +} + +static inline void xen_mc_issue(unsigned mode) +{ + trace_xen_mc_issue(mode); + __xen_mc_issue(mode); }

/* Set up a callback to be called when the current batch is flushed */

vunmap()'s issued from housekeeping CPUs are a relatively common source of interference for isolated NOHZ_FULL CPUs, as they are hit by the flush_tlb_kernel_range() IPIs.

Given that CPUs executing in userspace do not access data in the vmalloc range, these IPIs could be deferred until their next kernel entry.

Deferral vs early entry danger zone ===================================

This requires a guarantee that nothing in the vmalloc range can be vunmap'd and then accessed in early entry code.

Vmalloc uses are, as reported by vmallocinfo:

$ cat /proc/vmallocinfo | awk '{ print $3 }' | sort | uniq __pci_enable_msix_range+0x32b/0x560 acpi_os_map_iomem+0x22d/0x250 bpf_prog_alloc_no_stats+0x34/0x140 fork_idle+0x79/0x120 gen_pool_add_owner+0x3e/0xb0 ? hpet_enable+0xbf/0x470 irq_init_percpu_irqstack+0x129/0x160 kernel_clone+0xab/0x3b0 memremap+0x164/0x330 n_tty_open+0x18/0xa0 pcpu_create_chunk+0x4e/0x1b0 pcpu_create_chunk+0x75/0x1b0 pcpu_get_vm_areas+0x0/0x1100 unpurged vp_modern_map_capability+0x140/0x270 zisofs_init+0x16/0x30

I've categorized these as:

a) Device or percpu mappings

For these to be unmapped, the device (or CPU) has to be removed and an eventual IRQ freed. Even if the IRQ isn't freed, context tracking entry happens before handling the IRQ itself, per irqentry_enter() usage.

__pci_enable_msix_range() acpi_os_map_iomem() irq_init_percpu_irqstack() (not even unmapped when CPU is hot-unplugged!) memremap() n_tty_open() pcpu_create_chunk() pcpu_get_vm_areas() vp_modern_map_capability()

b) CONFIG_VMAP_STACK

fork_idle() & kernel_clone()

vmalloc'd kernel stacks are AFAICT a safe example, as a task running in userspace needs to enter kernelspace to execute do_exit() before its stack can be vfree'd.

c) Non-issues

bpf_prog_alloc_no_stats() - early entry is noinstr, no BPF! hpet_enable() - hpet_clear_mapping() is only called if __init function fails, no runtime worries zisofs_init () - used for zisofs block decompression, that's way past context tracking entry

d) I'm not sure, have a look?

gen_pool_add_owner() - AIUI this is mainly for PCI / DMA stuff, which again I wouldn't expect to be accessed before context tracking entry.

Changes ======

Blindly deferring any and all flush of the kernel mappings is a risky move, so introduce a variant of flush_tlb_kernel_range() that explicitly allows deferral. Use it for vunmap flushes.

Note that while flush_tlb_kernel_range() may end up issuing a full flush (including user mappings), this only happens when reaching a invalidation range threshold where it is cheaper to do a full flush than to individually invalidate each page in the range via INVLPG. IOW, it doesn't *require* invalidating user mappings, and thus remains safe to defer until a later kernel entry.

Signed-off-by: Valentin Schneider vschneid@redhat.com --- arch/x86/include/asm/context_tracking_work.h | 4 +++ arch/x86/include/asm/tlbflush.h | 1 + arch/x86/mm/tlb.c | 23 +++++++++++-- include/linux/context_tracking_work.h | 4 ++- mm/vmalloc.c | 35 +++++++++++++++++--- 5 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/context_tracking_work.h b/arch/x86/include/asm/context_tracking_work.h index 485b32881fde5..da3d270289836 100644 --- a/arch/x86/include/asm/context_tracking_work.h +++ b/arch/x86/include/asm/context_tracking_work.h @@ -3,6 +3,7 @@ #define _ASM_X86_CONTEXT_TRACKING_WORK_H

#include <asm/sync_core.h> +#include <asm/tlbflush.h>

static __always_inline void arch_context_tracking_work(enum ct_work work) { @@ -10,6 +11,9 @@ static __always_inline void arch_context_tracking_work(enum ct_work work) case CT_WORK_SYNC: sync_core(); break; + case CT_WORK_TLBI: + __flush_tlb_all(); + break; case CT_WORK_MAX: WARN_ON_ONCE(true); } diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h index 4d11396250999..6e690acc35e53 100644 --- a/arch/x86/include/asm/tlbflush.h +++ b/arch/x86/include/asm/tlbflush.h @@ -248,6 +248,7 @@ extern void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start, unsigned long end, unsigned int stride_shift, bool freed_tables); extern void flush_tlb_kernel_range(unsigned long start, unsigned long end); +extern void flush_tlb_kernel_range_deferrable(unsigned long start, unsigned long end);

static inline void flush_tlb_page(struct vm_area_struct *vma, unsigned long a) { diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c index 119765772ab11..47fb437acf52a 100644 --- a/arch/x86/mm/tlb.c +++ b/arch/x86/mm/tlb.c @@ -12,6 +12,7 @@ #include <linux/task_work.h> #include <linux/mmu_notifier.h> #include <linux/mmu_context.h> +#include <linux/context_tracking.h>

#include <asm/tlbflush.h> #include <asm/mmu_context.h> @@ -1042,6 +1043,11 @@ static void do_flush_tlb_all(void *info) __flush_tlb_all(); }

+static bool do_kernel_flush_defer_cond(int cpu, void *info) +{ + return !ct_set_cpu_work(cpu, CT_WORK_TLBI); +} + void flush_tlb_all(void) { count_vm_tlb_event(NR_TLB_REMOTE_FLUSH); @@ -1058,12 +1064,13 @@ static void do_kernel_range_flush(void *info) flush_tlb_one_kernel(addr); }

-void flush_tlb_kernel_range(unsigned long start, unsigned long end) +static inline void +__flush_tlb_kernel_range(smp_cond_func_t cond_func, unsigned long start, unsigned long end) { /* Balance as user space task's flush, a bit conservative */ if (end == TLB_FLUSH_ALL || (end - start) > tlb_single_page_flush_ceiling << PAGE_SHIFT) { - on_each_cpu(do_flush_tlb_all, NULL, 1); + on_each_cpu_cond(cond_func, do_flush_tlb_all, NULL, 1); } else { struct flush_tlb_info *info;

@@ -1071,13 +1078,23 @@ void flush_tlb_kernel_range(unsigned long start, unsigned long end) info = get_flush_tlb_info(NULL, start, end, 0, false, TLB_GENERATION_INVALID);

- on_each_cpu(do_kernel_range_flush, info, 1); + on_each_cpu_cond(cond_func, do_kernel_range_flush, info, 1);

put_flush_tlb_info(); preempt_enable(); } }

+void flush_tlb_kernel_range(unsigned long start, unsigned long end) +{ + __flush_tlb_kernel_range(NULL, start, end); +} + +void flush_tlb_kernel_range_deferrable(unsigned long start, unsigned long end) +{ + __flush_tlb_kernel_range(do_kernel_flush_defer_cond, start, end); +} + /* * This can be used from process context to figure out what the value of * CR3 is without needing to do a (slow) __read_cr3(). diff --git a/include/linux/context_tracking_work.h b/include/linux/context_tracking_work.h index 931ade1dcbcc2..1be60c64cdeca 100644 --- a/include/linux/context_tracking_work.h +++ b/include/linux/context_tracking_work.h @@ -5,12 +5,14 @@ #include <linux/bitops.h>

enum { - CT_WORK_SYNC, + CT_WORK_SYNC_OFFSET, + CT_WORK_TLBI_OFFSET, CT_WORK_MAX_OFFSET };

enum ct_work { CT_WORK_SYNC = BIT(CT_WORK_SYNC_OFFSET), + CT_WORK_TLBI = BIT(CT_WORK_TLBI_OFFSET), CT_WORK_MAX = BIT(CT_WORK_MAX_OFFSET) };

diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 5c88d0e90c209..e8aad4d55e955 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -467,6 +467,31 @@ void vunmap_range_noflush(unsigned long start, unsigned long end) __vunmap_range_noflush(start, end); }

+#ifdef CONFIG_CONTEXT_TRACKING_WORK +/* + * !!! BIG FAT WARNING !!! + * + * The CPU is free to cache any part of the paging hierarchy it wants at any + * time. It's also free to set accessed and dirty bits at any time, even for + * instructions that may never execute architecturally. + * + * This means that deferring a TLB flush affecting freed page-table-pages (IOW, + * keeping them in a CPU's paging hierarchy cache) is akin to dancing in a + * minefield. + * + * This isn't a problem for deferral of TLB flushes in vmalloc, because + * page-table-pages used for vmap() mappings are never freed - see how + * __vunmap_range_noflush() walks the whole mapping but only clears the leaf PTEs. + * If this ever changes, TLB flush deferral will cause misery. + */ +void __weak flush_tlb_kernel_range_deferrable(unsigned long start, unsigned long end) +{ + flush_tlb_kernel_range(start, end); +} +#else +#define flush_tlb_kernel_range_deferrable(start, end) flush_tlb_kernel_range(start, end) +#endif + /** * vunmap_range - unmap kernel virtual addresses * @addr: start of the VM area to unmap @@ -480,7 +505,7 @@ void vunmap_range(unsigned long addr, unsigned long end) { flush_cache_vunmap(addr, end); vunmap_range_noflush(addr, end); - flush_tlb_kernel_range(addr, end); + flush_tlb_kernel_range_deferrable(addr, end); }

static int vmap_pages_pte_range(pmd_t *pmd, unsigned long addr, @@ -2281,7 +2306,7 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end,

nr_purge_nodes = cpumask_weight(&purge_nodes); if (nr_purge_nodes > 0) { - flush_tlb_kernel_range(start, end); + flush_tlb_kernel_range_deferrable(start, end);

/* One extra worker is per a lazy_max_pages() full set minus one. */ nr_purge_helpers = atomic_long_read(&vmap_lazy_nr) / lazy_max_pages(); @@ -2384,7 +2409,7 @@ static void free_unmap_vmap_area(struct vmap_area *va) flush_cache_vunmap(va->va_start, va->va_end); vunmap_range_noflush(va->va_start, va->va_end); if (debug_pagealloc_enabled_static()) - flush_tlb_kernel_range(va->va_start, va->va_end); + flush_tlb_kernel_range_deferrable(va->va_start, va->va_end);

free_vmap_area_noflush(va); } @@ -2832,7 +2857,7 @@ static void vb_free(unsigned long addr, unsigned long size) vunmap_range_noflush(addr, addr + size);

if (debug_pagealloc_enabled_static()) - flush_tlb_kernel_range(addr, addr + size); + flush_tlb_kernel_range_deferrable(addr, addr + size);

spin_lock(&vb->lock);

@@ -2897,7 +2922,7 @@ static void _vm_unmap_aliases(unsigned long start, unsigned long end, int flush) free_purged_blocks(&purge_list);

if (!__purge_vmap_area_lazy(start, end, false) && flush) - flush_tlb_kernel_range(start, end); + flush_tlb_kernel_range_deferrable(start, end); mutex_unlock(&vmap_purge_lock); }