From: Changbin Du <[email protected]>
The task of ftrace_arch_code_modify(_post)_prepare() caller is
stop_machine, whose caller and work thread are 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 runing 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 ]---
Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
Cc: Changbin Du <[email protected]>
Co-developed-by: Guo Ren <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
Cc: Zong Li <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
---
Changes in v2:
- Rewrite commit log with lockdep explanation [Guo Ren]
- Rebase on v6.1 [Guo Ren]
v1:
https://lore.kernel.org/linux-riscv/[email protected]/
---
arch/riscv/kernel/patch.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 765004b60513..8619706f8dfd 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
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.36.1
On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> From: Changbin Du <[email protected]>
> Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> Cc: Changbin Du <[email protected]>
> Co-developed-by: Guo Ren <[email protected]>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Zong Li <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> ---
Hey Guo Ren,
FYI you're missing a SoB from Chanbin on this patch. They gave one in
their v1 though so you should be able to re-use that?
Thanks,
Conor.
> Changes in v2:
> - Rewrite commit log with lockdep explanation [Guo Ren]
> - Rebase on v6.1 [Guo Ren]
>
> v1:
> https://lore.kernel.org/linux-riscv/[email protected]/
On Mon, Nov 21, 2022 at 6:13 PM Conor Dooley <[email protected]> wrote:
>
> On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> > From: Changbin Du <[email protected]>
>
> > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> > Cc: Changbin Du <[email protected]>
> > Co-developed-by: Guo Ren <[email protected]>
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Zong Li <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > ---
>
> Hey Guo Ren,
>
> FYI you're missing a SoB from Chanbin on this patch. They gave one in
> their v1 though so you should be able to re-use that?
I'm waiting for his SoB. I don't think I could directly use his SoB in
v1. I need him to confirm my rewritten commit log for lockdep
analysis.
>
> Thanks,
> Conor.
>
> > Changes in v2:
> > - Rewrite commit log with lockdep explanation [Guo Ren]
> > - Rebase on v6.1 [Guo Ren]
> >
> > v1:
> > https://lore.kernel.org/linux-riscv/[email protected]/
>
--
Best Regards
Guo Ren
Hello,
Does this patch get merged into riscv tree now? This problem has been there for a long
time. (I suppose you have received my previous reponse.)
On Mon, Nov 21, 2022 at 07:09:45PM +0800, Guo Ren wrote:
> On Mon, Nov 21, 2022 at 6:13 PM Conor Dooley <[email protected]> wrote:
> >
> > On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> > > From: Changbin Du <[email protected]>
> >
> > > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> > > Cc: Changbin Du <[email protected]>
> > > Co-developed-by: Guo Ren <[email protected]>
> > > Signed-off-by: Guo Ren <[email protected]>
> > > Cc: Zong Li <[email protected]>
> > > Cc: Palmer Dabbelt <[email protected]>
> > > ---
> >
> > Hey Guo Ren,
> >
> > FYI you're missing a SoB from Chanbin on this patch. They gave one in
> > their v1 though so you should be able to re-use that?
> I'm waiting for his SoB. I don't think I could directly use his SoB in
> v1. I need him to confirm my rewritten commit log for lockdep
> analysis.
>
> >
> > Thanks,
> > Conor.
> >
> > > Changes in v2:
> > > - Rewrite commit log with lockdep explanation [Guo Ren]
> > > - Rebase on v6.1 [Guo Ren]
> > >
> > > v1:
> > > https://lore.kernel.org/linux-riscv/[email protected]/
> >
>
>
> --
> Best Regards
> Guo Ren
--
Cheers,
Changbin Du
On 22 December 2022 00:19:02 GMT, Changbin Du <[email protected]> wrote:
>Hello,
>Does this patch get merged into riscv tree now? This problem has been there for a long
>time. (I suppose you have received my previous reponse.)
As far as I can tell, this patch is still missing a sign-off from you.
There's nothing in response to this thread that I can see which provides one.
>
>On Mon, Nov 21, 2022 at 07:09:45PM +0800, Guo Ren wrote:
>> On Mon, Nov 21, 2022 at 6:13 PM Conor Dooley <[email protected]> wrote:
>> >
>> > On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
>> > > From: Changbin Du <[email protected]>
>> >
>> > > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
>> > > Cc: Changbin Du <[email protected]>
>> > > Co-developed-by: Guo Ren <[email protected]>
>> > > Signed-off-by: Guo Ren <[email protected]>
>> > > Cc: Zong Li <[email protected]>
>> > > Cc: Palmer Dabbelt <[email protected]>
>> > > ---
>> >
>> > Hey Guo Ren,
>> >
>> > FYI you're missing a SoB from Chanbin on this patch. They gave one in
>> > their v1 though so you should be able to re-use that?
>> I'm waiting for his SoB. I don't think I could directly use his SoB in
>> v1. I need him to confirm my rewritten commit log for lockdep
>> analysis.
>>
>> >
>> > Thanks,
>> > Conor.
>> >
>> > > Changes in v2:
>> > > - Rewrite commit log with lockdep explanation [Guo Ren]
>> > > - Rebase on v6.1 [Guo Ren]
>> > >
>> > > v1:
>> > > https://lore.kernel.org/linux-riscv/[email protected]/
>> >
>>
>>
>> --
>> Best Regards
>> Guo Ren
>
On Thu, Dec 22, 2022 at 8:19 AM Changbin Du <[email protected]> wrote:
>
> Hello,
> Does this patch get merged into riscv tree now? This problem has been there for a long
> time. (I suppose you have received my previous reponse.)
You could take the patch and update it to v3 with your SoB :)
>
> On Mon, Nov 21, 2022 at 07:09:45PM +0800, Guo Ren wrote:
> > On Mon, Nov 21, 2022 at 6:13 PM Conor Dooley <[email protected]> wrote:
> > >
> > > On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> > > > From: Changbin Du <[email protected]>
> > >
> > > > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> > > > Cc: Changbin Du <[email protected]>
> > > > Co-developed-by: Guo Ren <[email protected]>
> > > > Signed-off-by: Guo Ren <[email protected]>
> > > > Cc: Zong Li <[email protected]>
> > > > Cc: Palmer Dabbelt <[email protected]>
> > > > ---
> > >
> > > Hey Guo Ren,
> > >
> > > FYI you're missing a SoB from Chanbin on this patch. They gave one in
> > > their v1 though so you should be able to re-use that?
> > I'm waiting for his SoB. I don't think I could directly use his SoB in
> > v1. I need him to confirm my rewritten commit log for lockdep
> > analysis.
> >
> > >
> > > Thanks,
> > > Conor.
> > >
> > > > Changes in v2:
> > > > - Rewrite commit log with lockdep explanation [Guo Ren]
> > > > - Rebase on v6.1 [Guo Ren]
> > > >
> > > > v1:
> > > > https://lore.kernel.org/linux-riscv/[email protected]/
> > >
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
> --
> Cheers,
> Changbin Du
--
Best Regards
Guo Ren
On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> From: Changbin Du <[email protected]>
>
> The task of ftrace_arch_code_modify(_post)_prepare() caller is
> stop_machine, whose caller and work thread are 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 runing 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 ]---
>
> Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> Cc: Changbin Du <[email protected]>
> Co-developed-by: Guo Ren <[email protected]>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Zong Li <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> ---
> Changes in v2:
> - Rewrite commit log with lockdep explanation [Guo Ren]
> - Rebase on v6.1 [Guo Ren]
>
> v1:
> https://lore.kernel.org/linux-riscv/[email protected]/
> ---
> arch/riscv/kernel/patch.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 765004b60513..8619706f8dfd 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
> 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.36.1
>
Signed-off-by: Changbin Du <[email protected]>
Thanks!
On Thu, Dec 22, 2022 at 12:29:56AM +0000, Conor Dooley wrote:
>
>
> On 22 December 2022 00:19:02 GMT, Changbin Du <[email protected]> wrote:
> >Hello,
> >Does this patch get merged into riscv tree now? This problem has been there for a long
> >time. (I suppose you have received my previous reponse.)
>
> As far as I can tell, this patch is still missing a sign-off from you.
> There's nothing in response to this thread that I can see which provides one.
>
Just replied the patch and added my SoB. Thanks!
> >
> >On Mon, Nov 21, 2022 at 07:09:45PM +0800, Guo Ren wrote:
> >> On Mon, Nov 21, 2022 at 6:13 PM Conor Dooley <[email protected]> wrote:
> >> >
> >> > On Sun, Nov 20, 2022 at 05:10:49AM -0500, [email protected] wrote:
> >> > > From: Changbin Du <[email protected]>
> >> >
> >> > > Fixes: 0ff7c3b33127 ("riscv: Use text_mutex instead of patch_lock")
> >> > > Cc: Changbin Du <[email protected]>
> >> > > Co-developed-by: Guo Ren <[email protected]>
> >> > > Signed-off-by: Guo Ren <[email protected]>
> >> > > Cc: Zong Li <[email protected]>
> >> > > Cc: Palmer Dabbelt <[email protected]>
> >> > > ---
> >> >
> >> > Hey Guo Ren,
> >> >
> >> > FYI you're missing a SoB from Chanbin on this patch. They gave one in
> >> > their v1 though so you should be able to re-use that?
> >> I'm waiting for his SoB. I don't think I could directly use his SoB in
> >> v1. I need him to confirm my rewritten commit log for lockdep
> >> analysis.
> >>
> >> >
> >> > Thanks,
> >> > Conor.
> >> >
> >> > > Changes in v2:
> >> > > - Rewrite commit log with lockdep explanation [Guo Ren]
> >> > > - Rebase on v6.1 [Guo Ren]
> >> > >
> >> > > v1:
> >> > > https://lore.kernel.org/linux-riscv/[email protected]/
> >> >
> >>
> >>
> >> --
> >> Best Regards
> >> Guo Ren
> >
--
Cheers,
Changbin Du