Skip to content

Commit 2a8db5e

Browse files
ConchuODpalmer-dabbelt
authored andcommitted
RISC-V: Don't check text_mutex during stop_machine
We're currently using stop_machine() to update ftrace & kprobes, which means that the thread that takes text_mutex during may not be the same as the thread that eventually patches the code. This isn't actually a race because the lock is still held (preventing any other concurrent accesses) and there is only one thread running during stop_machine(), but it does trigger a lockdep failure. This patch just elides the lockdep check during stop_machine. Fixes: c15ac4f ("riscv/ftrace: Add dynamic function tracer support") Suggested-by: Steven Rostedt <rostedt@goodmis.org> Reported-by: Changbin Du <changbin.du@gmail.com> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/20230303143754.4005217-1-conor.dooley@microchip.com Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
1 parent 7695034 commit 2a8db5e

4 files changed

Lines changed: 39 additions & 6 deletions

File tree

arch/riscv/include/asm/ftrace.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,6 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
109109
#define ftrace_init_nop ftrace_init_nop
110110
#endif
111111

112-
#endif
112+
#endif /* CONFIG_DYNAMIC_FTRACE */
113113

114114
#endif /* _ASM_RISCV_FTRACE_H */

arch/riscv/include/asm/patch.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,6 @@
99
int patch_text_nosync(void *addr, const void *insns, size_t len);
1010
int patch_text(void *addr, u32 *insns, int ninsns);
1111

12+
extern int riscv_patch_in_stop_machine;
13+
1214
#endif /* _ASM_RISCV_PATCH_H */

arch/riscv/kernel/ftrace.c

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,19 @@
1515
void ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)
1616
{
1717
mutex_lock(&text_mutex);
18+
19+
/*
20+
* The code sequences we use for ftrace can't be patched while the
21+
* kernel is running, so we need to use stop_machine() to modify them
22+
* for now. This doesn't play nice with text_mutex, we use this flag
23+
* to elide the check.
24+
*/
25+
riscv_patch_in_stop_machine = true;
1826
}
1927

2028
void ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
2129
{
30+
riscv_patch_in_stop_machine = false;
2231
mutex_unlock(&text_mutex);
2332
}
2433

@@ -107,9 +116,9 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
107116
{
108117
int out;
109118

110-
ftrace_arch_code_modify_prepare();
119+
mutex_lock(&text_mutex);
111120
out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
112-
ftrace_arch_code_modify_post_process();
121+
mutex_unlock(&text_mutex);
113122

114123
return out;
115124
}

arch/riscv/kernel/patch.c

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include <asm/kprobes.h>
1212
#include <asm/cacheflush.h>
1313
#include <asm/fixmap.h>
14+
#include <asm/ftrace.h>
1415
#include <asm/patch.h>
1516

1617
struct patch_insn {
@@ -20,6 +21,8 @@ struct patch_insn {
2021
atomic_t cpu_count;
2122
};
2223

24+
int riscv_patch_in_stop_machine = false;
25+
2326
#ifdef CONFIG_MMU
2427
/*
2528
* The fix_to_virt(, idx) needs a const value (not a dynamic variable of
@@ -60,8 +63,15 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
6063
* Before reaching here, it was expected to lock the text_mutex
6164
* already, so we don't need to give another lock here and could
6265
* ensure that it was safe between each cores.
66+
*
67+
* We're currently using stop_machine() for ftrace & kprobes, and while
68+
* that ensures text_mutex is held before installing the mappings it
69+
* does not ensure text_mutex is held by the calling thread. That's
70+
* safe but triggers a lockdep failure, so just elide it for that
71+
* specific case.
6372
*/
64-
lockdep_assert_held(&text_mutex);
73+
if (!riscv_patch_in_stop_machine)
74+
lockdep_assert_held(&text_mutex);
6575

6676
if (across_pages)
6777
patch_map(addr + len, FIX_TEXT_POKE1);
@@ -125,14 +135,26 @@ NOKPROBE_SYMBOL(patch_text_cb);
125135

126136
int patch_text(void *addr, u32 *insns, int ninsns)
127137
{
138+
int ret;
128139
struct patch_insn patch = {
129140
.addr = addr,
130141
.insns = insns,
131142
.ninsns = ninsns,
132143
.cpu_count = ATOMIC_INIT(0),
133144
};
134145

135-
return stop_machine_cpuslocked(patch_text_cb,
136-
&patch, cpu_online_mask);
146+
/*
147+
* kprobes takes text_mutex, before calling patch_text(), but as we call
148+
* calls stop_machine(), the lockdep assertion in patch_insn_write()
149+
* gets confused by the context in which the lock is taken.
150+
* Instead, ensure the lock is held before calling stop_machine(), and
151+
* set riscv_patch_in_stop_machine to skip the check in
152+
* patch_insn_write().
153+
*/
154+
lockdep_assert_held(&text_mutex);
155+
riscv_patch_in_stop_machine = true;
156+
ret = stop_machine_cpuslocked(patch_text_cb, &patch, cpu_online_mask);
157+
riscv_patch_in_stop_machine = false;
158+
return ret;
137159
}
138160
NOKPROBE_SYMBOL(patch_text);

0 commit comments

Comments
 (0)