diff mbox series

[v4] riscv: patch: Fixup lockdep warning in stop_machine

Message ID 20230202114116.3695793-1-changbin.du@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Palmer Dabbelt
Headers show
Series [v4] riscv: patch: Fixup lockdep warning in stop_machine | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be fixes
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 13 and now 13
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 2 this patch: 2
conchuod/alphanumeric_selects success Out of order selects before the patch: 57 and now 57
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 2 this patch: 2
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 38 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Changbin Du Feb. 2, 2023, 11:41 a.m. UTC
The task of ftrace_arch_code_modify(_post)_prepare() caller is
stop_machine, whose caller and work thread are of different tasks. The
lockdep checker needs the same task context, or it's wrong. That means
it's a bug here to use lockdep_assert_held because we don't guarantee
the same task context.

kernel/locking/lockdep.c:
int __lock_is_held(const struct lockdep_map *lock, int read)
{
        struct task_struct *curr = current;
        int i;

        for (i = 0; i < curr->lockdep_depth; i++) {
			^^^^^^^^^^^^^^^^^^^
                struct held_lock *hlock = curr->held_locks + i;
					  ^^^^^^^^^^^^^^^^
                if (match_held_lock(hlock, lock)) {
                        if (read == -1 || !!hlock->read == read)
                                return LOCK_STATE_HELD;

The __lock_is_held depends on current held_locks records; if
stop_machine makes the checker running on another task, that's wrong.

Here is the log:
[   15.761523] ------------[ cut here ]------------
[   15.762125] WARNING: CPU: 0 PID: 15 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x72/0x364
[   15.763258] Modules linked in:
[   15.764154] CPU: 0 PID: 15 Comm: migration/0 Not tainted 6.1.0-rc1-00014-g66924be85884-dirty #377
[   15.765339] Hardware name: riscv-virtio,qemu (DT)
[   15.765985] Stopper: multi_cpu_stop+0x0/0x192 <- stop_cpus.constprop.0+0x90/0xe2
[   15.766711] epc : patch_insn_write+0x72/0x364
[   15.767011]  ra : patch_insn_write+0x70/0x364
[   15.767276] epc : ffffffff8000721e ra : ffffffff8000721c sp : ff2000000067bca0
[   15.767622]  gp : ffffffff81603f90 tp : ff60000002432a00 t0 : 7300000000000000
[   15.767919]  t1 : 0000000000000000 t2 : 73695f6b636f6c5f s0 : ff2000000067bcf0
[   15.768238]  s1 : 0000000000000008 a0 : 0000000000000000 a1 : 0000000000000000
[   15.768537]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
[   15.768837]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000000
[   15.769139]  s2 : ffffffff80009faa s3 : ff2000000067bd10 s4 : ffffffffffffffff
[   15.769447]  s5 : 0000000000000001 s6 : 0000000000000001 s7 : 0000000000000003
[   15.769740]  s8 : 0000000000000002 s9 : 0000000000000004 s10: 0000000000000003
[   15.770027]  s11: 0000000000000002 t3 : 0000000000000000 t4 : ffffffff819af097
[   15.770323]  t5 : ffffffff819af098 t6 : ff2000000067ba28
[   15.770574] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[   15.771102] [<ffffffff80007520>] patch_text_nosync+0x10/0x3a
[   15.771421] [<ffffffff80009c66>] ftrace_update_ftrace_func+0x74/0x10a
[   15.771704] [<ffffffff800fa17e>] ftrace_modify_all_code+0xb0/0x16c
[   15.771958] [<ffffffff800fa24c>] __ftrace_modify_code+0x12/0x1c
[   15.772196] [<ffffffff800e110e>] multi_cpu_stop+0x14a/0x192
[   15.772454] [<ffffffff800e0a34>] cpu_stopper_thread+0x96/0x14c
[   15.772699] [<ffffffff8003f4ea>] smpboot_thread_fn+0xf8/0x1cc
[   15.772945] [<ffffffff8003ac9c>] kthread+0xe2/0xf8
[   15.773160] [<ffffffff80003e98>] ret_from_exception+0x0/0x14
[   15.773471] ---[ end trace 0000000000000000 ]---

By the way, this also fixes the same issue for patch_text().

Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
Co-developed-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Guo Ren <guoren@kernel.org>
Cc: Zong Li <zong.li@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
Changes in v4:
 - preserve and update comments.

Changes in v3:
 - denote this also fixes function patch_text().

Changes in v2:
 - Rewrite commit log with lockdep explanation [Guo Ren]
 - Rebase on v6.1 [Guo Ren]

v1:
https://lore.kernel.org/linux-riscv/20210417023532.354714-1-changbin.du@gmail.com/
---
 arch/riscv/kernel/ftrace.c |  5 ++---
 arch/riscv/kernel/patch.c  | 15 ++++++++-------
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

Conor Dooley Feb. 15, 2023, 12:22 a.m. UTC | #1
Hey Changbin,

On Thu, Feb 02, 2023 at 07:41:16PM +0800, Changbin Du wrote:
> The task of ftrace_arch_code_modify(_post)_prepare() caller is
> stop_machine, whose caller and work thread are of different tasks. The
> lockdep checker needs the same task context, or it's wrong. That means
> it's a bug here to use lockdep_assert_held because we don't guarantee
> the same task context.
> 
> kernel/locking/lockdep.c:
> int __lock_is_held(const struct lockdep_map *lock, int read)
> {
>         struct task_struct *curr = current;
>         int i;
> 
>         for (i = 0; i < curr->lockdep_depth; i++) {
> 			^^^^^^^^^^^^^^^^^^^
>                 struct held_lock *hlock = curr->held_locks + i;
> 					  ^^^^^^^^^^^^^^^^
>                 if (match_held_lock(hlock, lock)) {
>                         if (read == -1 || !!hlock->read == read)
>                                 return LOCK_STATE_HELD;
> 
> The __lock_is_held depends on current held_locks records; if
> stop_machine makes the checker running on another task, that's wrong.
> 
> Here is the log:
> [   15.761523] ------------[ cut here ]------------
> [   15.762125] WARNING: CPU: 0 PID: 15 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x72/0x364
> [   15.763258] Modules linked in:
> [   15.764154] CPU: 0 PID: 15 Comm: migration/0 Not tainted 6.1.0-rc1-00014-g66924be85884-dirty #377
> [   15.765339] Hardware name: riscv-virtio,qemu (DT)
> [   15.765985] Stopper: multi_cpu_stop+0x0/0x192 <- stop_cpus.constprop.0+0x90/0xe2
> [   15.766711] epc : patch_insn_write+0x72/0x364
> [   15.767011]  ra : patch_insn_write+0x70/0x364
> [   15.767276] epc : ffffffff8000721e ra : ffffffff8000721c sp : ff2000000067bca0
> [   15.767622]  gp : ffffffff81603f90 tp : ff60000002432a00 t0 : 7300000000000000
> [   15.767919]  t1 : 0000000000000000 t2 : 73695f6b636f6c5f s0 : ff2000000067bcf0
> [   15.768238]  s1 : 0000000000000008 a0 : 0000000000000000 a1 : 0000000000000000
> [   15.768537]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
> [   15.768837]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000000
> [   15.769139]  s2 : ffffffff80009faa s3 : ff2000000067bd10 s4 : ffffffffffffffff
> [   15.769447]  s5 : 0000000000000001 s6 : 0000000000000001 s7 : 0000000000000003
> [   15.769740]  s8 : 0000000000000002 s9 : 0000000000000004 s10: 0000000000000003
> [   15.770027]  s11: 0000000000000002 t3 : 0000000000000000 t4 : ffffffff819af097
> [   15.770323]  t5 : ffffffff819af098 t6 : ff2000000067ba28
> [   15.770574] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> [   15.771102] [<ffffffff80007520>] patch_text_nosync+0x10/0x3a
> [   15.771421] [<ffffffff80009c66>] ftrace_update_ftrace_func+0x74/0x10a
> [   15.771704] [<ffffffff800fa17e>] ftrace_modify_all_code+0xb0/0x16c
> [   15.771958] [<ffffffff800fa24c>] __ftrace_modify_code+0x12/0x1c
> [   15.772196] [<ffffffff800e110e>] multi_cpu_stop+0x14a/0x192
> [   15.772454] [<ffffffff800e0a34>] cpu_stopper_thread+0x96/0x14c
> [   15.772699] [<ffffffff8003f4ea>] smpboot_thread_fn+0xf8/0x1cc
> [   15.772945] [<ffffffff8003ac9c>] kthread+0xe2/0xf8
> [   15.773160] [<ffffffff80003e98>] ret_from_exception+0x0/0x14
> [   15.773471] ---[ end trace 0000000000000000 ]---
> 
> By the way, this also fixes the same issue for patch_text().

Given this lockdep stuff seems to have pointed out that we weren't
taking the lock for alternative patching just this past week [1], I'm
really not convinced that deleting this is a good idea.

Thanks,
Conor.

1 - https://patchwork.kernel.org/project/linux-riscv/patch/20230212194735.491785-1-conor@kernel.org/

> 
> Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> Co-developed-by: Guo Ren <guoren@kernel.org>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Zong Li <zong.li@sifive.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
> Changes in v4:
>  - preserve and update comments.
> 
> Changes in v3:
>  - denote this also fixes function patch_text().
> 
> Changes in v2:
>  - Rewrite commit log with lockdep explanation [Guo Ren]
>  - Rebase on v6.1 [Guo Ren]
> 
> v1:
> https://lore.kernel.org/linux-riscv/20210417023532.354714-1-changbin.du@gmail.com/
> ---
>  arch/riscv/kernel/ftrace.c |  5 ++---
>  arch/riscv/kernel/patch.c  | 15 ++++++++-------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 2086f6585773..f73660e73822 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -126,9 +126,8 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>  /*
>   * This is called early on, and isn't wrapped by
>   * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
> - * text_mutex, which triggers a lockdep failure.  SMP isn't running so we could
> - * just directly poke the text, but it's simpler to just take the lock
> - * ourselves.
> + * text_mutex. SMP isn't running so we could just directly poke the text, but
> + * it's simpler to just take the lock ourselves.
>   */
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>  {
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 765004b60513..8eb243703efe 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -49,19 +49,20 @@ static void patch_unmap(int fixmap)
>  }
>  NOKPROBE_SYMBOL(patch_unmap);
>  
> +/*
> + * Before reaching here, it was expected to lock the text_mutex
> + * already, so we don't need to give another lock here and could
> + * ensure that it was safe between each cores. We do not add
> + * lockdep assertion here since it would trigger a false positive
> + * when called by stop_machine (The lockdep checker requires the
> + * same task context).
> + */
>  static int patch_insn_write(void *addr, const void *insn, size_t len)
>  {
>  	void *waddr = addr;
>  	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
>  	int ret;
>  
> -	/*
> -	 * Before reaching here, it was expected to lock the text_mutex
> -	 * already, so we don't need to give another lock here and could
> -	 * ensure that it was safe between each cores.
> -	 */
> -	lockdep_assert_held(&text_mutex);
> -
>  	if (across_pages)
>  		patch_map(addr + len, FIX_TEXT_POKE1);
>  
> -- 
> 2.25.1
>
Guo Ren Feb. 15, 2023, 1:24 a.m. UTC | #2
On Wed, Feb 15, 2023 at 8:22 AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Changbin,
>
> On Thu, Feb 02, 2023 at 07:41:16PM +0800, Changbin Du wrote:
> > The task of ftrace_arch_code_modify(_post)_prepare() caller is
> > stop_machine, whose caller and work thread are of different tasks. The
> > lockdep checker needs the same task context, or it's wrong. That means
> > it's a bug here to use lockdep_assert_held because we don't guarantee
> > the same task context.
I'm trying to delete all stop_machine in riscv, from ftrace to kprobe.
When I have done, we needn't this patch.

> >
> > kernel/locking/lockdep.c:
> > int __lock_is_held(const struct lockdep_map *lock, int read)
> > {
> >         struct task_struct *curr = current;
> >         int i;
> >
> >         for (i = 0; i < curr->lockdep_depth; i++) {
> >                       ^^^^^^^^^^^^^^^^^^^
> >                 struct held_lock *hlock = curr->held_locks + i;
> >                                         ^^^^^^^^^^^^^^^^
> >                 if (match_held_lock(hlock, lock)) {
> >                         if (read == -1 || !!hlock->read == read)
> >                                 return LOCK_STATE_HELD;
> >
> > The __lock_is_held depends on current held_locks records; if
> > stop_machine makes the checker running on another task, that's wrong.
> >
> > Here is the log:
> > [   15.761523] ------------[ cut here ]------------
> > [   15.762125] WARNING: CPU: 0 PID: 15 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x72/0x364
> > [   15.763258] Modules linked in:
> > [   15.764154] CPU: 0 PID: 15 Comm: migration/0 Not tainted 6.1.0-rc1-00014-g66924be85884-dirty #377
> > [   15.765339] Hardware name: riscv-virtio,qemu (DT)
> > [   15.765985] Stopper: multi_cpu_stop+0x0/0x192 <- stop_cpus.constprop.0+0x90/0xe2
> > [   15.766711] epc : patch_insn_write+0x72/0x364
> > [   15.767011]  ra : patch_insn_write+0x70/0x364
> > [   15.767276] epc : ffffffff8000721e ra : ffffffff8000721c sp : ff2000000067bca0
> > [   15.767622]  gp : ffffffff81603f90 tp : ff60000002432a00 t0 : 7300000000000000
> > [   15.767919]  t1 : 0000000000000000 t2 : 73695f6b636f6c5f s0 : ff2000000067bcf0
> > [   15.768238]  s1 : 0000000000000008 a0 : 0000000000000000 a1 : 0000000000000000
> > [   15.768537]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
> > [   15.768837]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000000
> > [   15.769139]  s2 : ffffffff80009faa s3 : ff2000000067bd10 s4 : ffffffffffffffff
> > [   15.769447]  s5 : 0000000000000001 s6 : 0000000000000001 s7 : 0000000000000003
> > [   15.769740]  s8 : 0000000000000002 s9 : 0000000000000004 s10: 0000000000000003
> > [   15.770027]  s11: 0000000000000002 t3 : 0000000000000000 t4 : ffffffff819af097
> > [   15.770323]  t5 : ffffffff819af098 t6 : ff2000000067ba28
> > [   15.770574] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > [   15.771102] [<ffffffff80007520>] patch_text_nosync+0x10/0x3a
> > [   15.771421] [<ffffffff80009c66>] ftrace_update_ftrace_func+0x74/0x10a
> > [   15.771704] [<ffffffff800fa17e>] ftrace_modify_all_code+0xb0/0x16c
> > [   15.771958] [<ffffffff800fa24c>] __ftrace_modify_code+0x12/0x1c
> > [   15.772196] [<ffffffff800e110e>] multi_cpu_stop+0x14a/0x192
> > [   15.772454] [<ffffffff800e0a34>] cpu_stopper_thread+0x96/0x14c
> > [   15.772699] [<ffffffff8003f4ea>] smpboot_thread_fn+0xf8/0x1cc
> > [   15.772945] [<ffffffff8003ac9c>] kthread+0xe2/0xf8
> > [   15.773160] [<ffffffff80003e98>] ret_from_exception+0x0/0x14
> > [   15.773471] ---[ end trace 0000000000000000 ]---
> >
> > By the way, this also fixes the same issue for patch_text().
>
> Given this lockdep stuff seems to have pointed out that we weren't
> taking the lock for alternative patching just this past week [1], I'm
> really not convinced that deleting this is a good idea.
>
> Thanks,
> Conor.
>
> 1 - https://patchwork.kernel.org/project/linux-riscv/patch/20230212194735.491785-1-conor@kernel.org/
>
> >
> > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> > Co-developed-by: Guo Ren <guoren@kernel.org>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > Cc: Zong Li <zong.li@sifive.com>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Signed-off-by: Changbin Du <changbin.du@huawei.com>
> > ---
> > Changes in v4:
> >  - preserve and update comments.
> >
> > Changes in v3:
> >  - denote this also fixes function patch_text().
> >
> > Changes in v2:
> >  - Rewrite commit log with lockdep explanation [Guo Ren]
> >  - Rebase on v6.1 [Guo Ren]
> >
> > v1:
> > https://lore.kernel.org/linux-riscv/20210417023532.354714-1-changbin.du@gmail.com/
> > ---
> >  arch/riscv/kernel/ftrace.c |  5 ++---
> >  arch/riscv/kernel/patch.c  | 15 ++++++++-------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > index 2086f6585773..f73660e73822 100644
> > --- a/arch/riscv/kernel/ftrace.c
> > +++ b/arch/riscv/kernel/ftrace.c
> > @@ -126,9 +126,8 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> >  /*
> >   * This is called early on, and isn't wrapped by
> >   * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
> > - * text_mutex, which triggers a lockdep failure.  SMP isn't running so we could
> > - * just directly poke the text, but it's simpler to just take the lock
> > - * ourselves.
> > + * text_mutex. SMP isn't running so we could just directly poke the text, but
> > + * it's simpler to just take the lock ourselves.
> >   */
> >  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> >  {
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index 765004b60513..8eb243703efe 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -49,19 +49,20 @@ static void patch_unmap(int fixmap)
> >  }
> >  NOKPROBE_SYMBOL(patch_unmap);
> >
> > +/*
> > + * Before reaching here, it was expected to lock the text_mutex
> > + * already, so we don't need to give another lock here and could
> > + * ensure that it was safe between each cores. We do not add
> > + * lockdep assertion here since it would trigger a false positive
> > + * when called by stop_machine (The lockdep checker requires the
> > + * same task context).
> > + */
> >  static int patch_insn_write(void *addr, const void *insn, size_t len)
> >  {
> >       void *waddr = addr;
> >       bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
> >       int ret;
> >
> > -     /*
> > -      * Before reaching here, it was expected to lock the text_mutex
> > -      * already, so we don't need to give another lock here and could
> > -      * ensure that it was safe between each cores.
> > -      */
> > -     lockdep_assert_held(&text_mutex);
> > -
> >       if (across_pages)
> >               patch_map(addr + len, FIX_TEXT_POKE1);
> >
> > --
> > 2.25.1
> >
Changbin Du Feb. 15, 2023, 3:45 a.m. UTC | #3
On Wed, Feb 15, 2023 at 09:24:33AM +0800, Guo Ren wrote:
> On Wed, Feb 15, 2023 at 8:22 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > Hey Changbin,
> >
> > On Thu, Feb 02, 2023 at 07:41:16PM +0800, Changbin Du wrote:
> > > The task of ftrace_arch_code_modify(_post)_prepare() caller is
> > > stop_machine, whose caller and work thread are of different tasks. The
> > > lockdep checker needs the same task context, or it's wrong. That means
> > > it's a bug here to use lockdep_assert_held because we don't guarantee
> > > the same task context.
> I'm trying to delete all stop_machine in riscv, from ftrace to kprobe.
> When I have done, we needn't this patch.
>
Which approch would you use? I looked through the riscv-spec, but didn't find any
description abount concurrent modification and execution.

> > >
> > > 2.25.1
> > >
> 
> 
> 
> -- 
> Best Regards
>  Guo Ren
Björn Töpel Feb. 15, 2023, 11:01 a.m. UTC | #4
Changbin Du <changbin.du@huawei.com> writes:

> On Wed, Feb 15, 2023 at 09:24:33AM +0800, Guo Ren wrote:
>> On Wed, Feb 15, 2023 at 8:22 AM Conor Dooley <conor@kernel.org> wrote:
>> >
>> > Hey Changbin,
>> >
>> > On Thu, Feb 02, 2023 at 07:41:16PM +0800, Changbin Du wrote:
>> > > The task of ftrace_arch_code_modify(_post)_prepare() caller is
>> > > stop_machine, whose caller and work thread are of different tasks. The
>> > > lockdep checker needs the same task context, or it's wrong. That means
>> > > it's a bug here to use lockdep_assert_held because we don't guarantee
>> > > the same task context.
>> I'm trying to delete all stop_machine in riscv, from ftrace to kprobe.
>> When I have done, we needn't this patch.
>>
> Which approch would you use? I looked through the riscv-spec, but didn't find any
> description abount concurrent modification and execution.

CMODX is not specified for RISC-V yet, unfortunately.

This has been discussed here [1]. Maybe we can start with stating for
which implementations Guo's approach work?

[1] https://lore.kernel.org/all/CAJF2gTS0s4X_uwLaEeSqKAyRmxCR2vxRuHhz7-SP2w4bBqzr+Q@mail.gmail.com/
diff mbox series

Patch

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 2086f6585773..f73660e73822 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -126,9 +126,8 @@  int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 /*
  * This is called early on, and isn't wrapped by
  * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
- * text_mutex, which triggers a lockdep failure.  SMP isn't running so we could
- * just directly poke the text, but it's simpler to just take the lock
- * ourselves.
+ * text_mutex. SMP isn't running so we could just directly poke the text, but
+ * it's simpler to just take the lock ourselves.
  */
 int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
 {
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 765004b60513..8eb243703efe 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -49,19 +49,20 @@  static void patch_unmap(int fixmap)
 }
 NOKPROBE_SYMBOL(patch_unmap);
 
+/*
+ * Before reaching here, it was expected to lock the text_mutex
+ * already, so we don't need to give another lock here and could
+ * ensure that it was safe between each cores. We do not add
+ * lockdep assertion here since it would trigger a false positive
+ * when called by stop_machine (The lockdep checker requires the
+ * same task context).
+ */
 static int patch_insn_write(void *addr, const void *insn, size_t len)
 {
 	void *waddr = addr;
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
-	/*
-	 * Before reaching here, it was expected to lock the text_mutex
-	 * already, so we don't need to give another lock here and could
-	 * ensure that it was safe between each cores.
-	 */
-	lockdep_assert_held(&text_mutex);
-
 	if (across_pages)
 		patch_map(addr + len, FIX_TEXT_POKE1);