Date:   Mon, 30 Jan 2023 15:09:41 +0000
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Changbin Du <changbin.du@huawei.com>
CC:     Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Hui Wang <hw.huiwang@huawei.com>,
        <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        Changbin Du <changbin.du@gmail.com>,
        Guo Ren <guoren@kernel.org>, Zong Li <zong.li@sifive.com>
Subject: Re: [PATCH v3] riscv: patch: Fixup lockdep warning in stop_machine
Message-ID: <Y9fdtcoh8POLZ6CD@wendy>
References: <20230130232659.3374212-1-changbin.du@huawei.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="ZHJ8mDdLEgO7bBXq"
Content-Disposition: inline
In-Reply-To: <20230130232659.3374212-1-changbin.du@huawei.com>
Precedence: bulk

--ZHJ8mDdLEgO7bBXq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey Changbin,

On Tue, Jan 31, 2023 at 07:26:59AM +0800, Changbin Du wrote:
> From: Changbin Du <changbin.du@gmail.com>
>=20
> 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.
>=20
> kernel/locking/lockdep.c:
> int __lock_is_held(const struct lockdep_map *lock, int read)
> {
>         struct task_struct *curr =3D current;
>         int i;
>=20
>         for (i =3D 0; i < curr->lockdep_depth; i++) {
> 			^^^^^^^^^^^^^^^^^^^
>                 struct held_lock *hlock =3D curr->held_locks + i;
> 					  ^^^^^^^^^^^^^^^^
>                 if (match_held_lock(hlock, lock)) {
>                         if (read =3D=3D -1 || !!hlock->read =3D=3D read)
>                                 return LOCK_STATE_HELD;
>=20
> The __lock_is_held depends on current held_locks records; if
> stop_machine makes the checker runing on another task, that's wrong.
>=20
> Here is the log:
> [   15.761523] ------------[ cut here ]------------
> [   15.762125] WARNING: CPU: 0 PID: 15 at arch/riscv/kernel/patch.c:63 pa=
tch_insn_write+0x72/0x364
> [   15.763258] Modules linked in:
> [   15.764154] CPU: 0 PID: 15 Comm: migration/0 Not tainted 6.1.0-rc1-000=
14-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 : ff200000=
0067bca0
> [   15.767622]  gp : ffffffff81603f90 tp : ff60000002432a00 t0 : 73000000=
00000000
> [   15.767919]  t1 : 0000000000000000 t2 : 73695f6b636f6c5f s0 : ff200000=
0067bcf0
> [   15.768238]  s1 : 0000000000000008 a0 : 0000000000000000 a1 : 00000000=
00000000
> [   15.768537]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 00000000=
00000000
> [   15.768837]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000=
00000000
> [   15.769139]  s2 : ffffffff80009faa s3 : ff2000000067bd10 s4 : ffffffff=
ffffffff
> [   15.769447]  s5 : 0000000000000001 s6 : 0000000000000001 s7 : 00000000=
00000003
> [   15.769740]  s8 : 0000000000000002 s9 : 0000000000000004 s10: 00000000=
00000003
> [   15.770027]  s11: 0000000000000002 t3 : 0000000000000000 t4 : ffffffff=
819af097
> [   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 ]---

FWIW, you can always crop the [15.192321] stuff out of commit messages,
as it just adds noise.

> By the way, this also fixes the same issue for patch_text().
>=20
> 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 v3:
>  - denote this also fixes function patch_text().
>=20
> Changes in v2:
>  - Rewrite commit log with lockdep explanation [Guo Ren]
>  - Rebase on v6.1 [Guo Ren]
>=20
> v1:
> https://lore.kernel.org/linux-riscv/20210417023532.354714-1-changbin.du@g=
mail.com/
> ---
>  arch/riscv/kernel/patch.c | 7 -------
>  1 file changed, 7 deletions(-)
>=20
> 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 *in=
sn, size_t len)
>  	bool across_pages =3D (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SI=
ZE;
>  	int ret;
> =20
> -	/*
> -	 * 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);

I must admit, patches like this do concern me a little, as a someone
unfamiliar with the world of probing and tracing.
Seeing an explicit check that the lock was held, leads me to believe
that the original author (Zong Li I think) thought that the text_mutex
lock was insufficient.
Do you think that their fear is unfounded? Explaining why it is safe to
remove this assertion in the commit message would go a long way towards
easing my anxiety!

Also, why delete the comment altogether? The comment provides some
information that doesn't appear to become invalid, even with the
assertion removed?

Thanks,
Conor.

> -
>  	if (across_pages)
>  		patch_map(addr + len, FIX_TEXT_POKE1);
> =20
> --=20
> 2.25.1
>=20
>=20

--ZHJ8mDdLEgO7bBXq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY9fdtQAKCRB4tDGHoIJi
0sslAP49Nvkw+3fXSSaHe03RLitfwfj3l8d8kcrLGD0hdLYxywEAgXyRn/9IxdRM
CufK52dxbWa8JP2SttW8REey/iqxxgU=
=GXtU
-----END PGP SIGNATURE-----

--ZHJ8mDdLEgO7bBXq--