All of lore.kernel.org
 help / color / mirror / Atom feed
From: Anup Patel <Anup.Patel@wdc.com>
To: Steven Rostedt <rostedt@goodmis.org>,
	Changbin Du <changbin.du@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"jpoimboe@redhat.com" <jpoimboe@redhat.com>,
	"jbaron@akamai.com" <jbaron@akamai.com>,
	"ardb@kernel.org" <ardb@kernel.org>,
	Atish Patra <Atish.Patra@wdc.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"rppt@kernel.org" <rppt@kernel.org>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>,
	"zong.li@sifive.com" <zong.li@sifive.com>,
	"guoren@linux.alibaba.com" <guoren@linux.alibaba.com>,
	"wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
	"0x7f454c46@gmail.com" <0x7f454c46@gmail.com>,
	"chenhuang5@huawei.com" <chenhuang5@huawei.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kernel-team@android.com" <kernel-team@android.com>,
	Palmer Dabbelt <palmerdabbelt@google.com>
Subject: RE: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*
Date: Fri, 30 Apr 2021 04:06:35 +0000	[thread overview]
Message-ID: <MN2PR04MB6207E102997326E6513416AB8D5E9@MN2PR04MB6207.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20210429224742.391154ae@oasis.local.home>



> -----Original Message-----
> From: Steven Rostedt <rostedt@goodmis.org>
> Sent: 30 April 2021 08:18
> To: Changbin Du <changbin.du@gmail.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>; linux-riscv@lists.infradead.org;
> Paul Walmsley <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu;
> peterz@infradead.org; jpoimboe@redhat.com; jbaron@akamai.com;
> ardb@kernel.org; Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> <Anup.Patel@wdc.com>; akpm@linux-foundation.org; rppt@kernel.org;
> mhiramat@kernel.org; zong.li@sifive.com; guoren@linux.alibaba.com;
> wangkefeng.wang@huawei.com; 0x7f454c46@gmail.com;
> chenhuang5@huawei.com; linux-kernel@vger.kernel.org; kernel-
> team@android.com; Palmer Dabbelt <palmerdabbelt@google.com>
> Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*
> 
> On Fri, 30 Apr 2021 05:54:51 +0800
> Changbin Du <changbin.du@gmail.com> wrote:
> 
> > The problem is that lockdep cannot handle locks across tasks since we
> > use stopmachine to patch code for risc-v. So there's a false positive report.
> > See privious disscussion here
> 
> > https://lkml.org/lkml/2021/4/29/63
> 
> Please use lore.kernel.org, lkml.org is highly unreliable, and is considered
> deprecated for use of referencing linux kernel archives.
> 
> Would the following patch work?

This patch only takes care of ftrace path.

The RISC-V instruction patching is used by RISC-V jump label implementation
as well and will called from various critical parts of core kernel.

The RAW spinlock approach allows same instruction patching to be used
for kprobes, ftrace, and jump label.

Regards,
Anup

> 
> (note, I did not even compile test it)
> 
> -- Steve
> 
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 845002cc2e57..19acbb4aaeff 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -25,6 +25,8 @@ struct dyn_arch_ftrace {  };  #endif
> 
> +extern int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  /*
>   * A general call in RISC-V is a pair of insts:
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c index
> 7f1e5203de88..834ab4fad637 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -11,15 +11,19 @@
>  #include <asm/cacheflush.h>
>  #include <asm/patch.h>
> 
> +int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)  {
>  	mutex_lock(&text_mutex);
> +	running_ftrace = 1;
>  	return 0;
>  }
> 
>  int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
> {
> +	running_ftrace = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c index
> 0b552873a577..4cd1c79a9689 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -12,6 +12,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/fixmap.h>
>  #include <asm/patch.h>
> +#include <asm/ftrace.h>
> 
>  struct patch_insn {
>  	void *addr;
> @@ -59,8 +60,13 @@ static int patch_insn_write(void *addr, const void
> *insn, size_t len)
>  	 * 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.
> +	 *
> +	 * ftrace uses stop machine, and even though the text_mutex is
> +	 * held, the stop machine task that calls this function will not
> +	 * be the owner.
>  	 */
> -	lockdep_assert_held(&text_mutex);
> +	if (!running_ftrace)
> +		lockdep_assert_held(&text_mutex);
> 
>  	if (across_pages)
>  		patch_map(addr + len, FIX_TEXT_POKE1);

WARNING: multiple messages have this Message-ID (diff)
From: Anup Patel <Anup.Patel@wdc.com>
To: Steven Rostedt <rostedt@goodmis.org>,
	Changbin Du <changbin.du@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"jpoimboe@redhat.com" <jpoimboe@redhat.com>,
	"jbaron@akamai.com" <jbaron@akamai.com>,
	"ardb@kernel.org" <ardb@kernel.org>,
	 Atish Patra <Atish.Patra@wdc.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"rppt@kernel.org" <rppt@kernel.org>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>,
	"zong.li@sifive.com" <zong.li@sifive.com>,
	"guoren@linux.alibaba.com" <guoren@linux.alibaba.com>,
	"wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
	"0x7f454c46@gmail.com" <0x7f454c46@gmail.com>,
	"chenhuang5@huawei.com" <chenhuang5@huawei.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kernel-team@android.com" <kernel-team@android.com>,
	Palmer Dabbelt <palmerdabbelt@google.com>
Subject: RE: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*
Date: Fri, 30 Apr 2021 04:06:35 +0000	[thread overview]
Message-ID: <MN2PR04MB6207E102997326E6513416AB8D5E9@MN2PR04MB6207.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20210429224742.391154ae@oasis.local.home>



> -----Original Message-----
> From: Steven Rostedt <rostedt@goodmis.org>
> Sent: 30 April 2021 08:18
> To: Changbin Du <changbin.du@gmail.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>; linux-riscv@lists.infradead.org;
> Paul Walmsley <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu;
> peterz@infradead.org; jpoimboe@redhat.com; jbaron@akamai.com;
> ardb@kernel.org; Atish Patra <Atish.Patra@wdc.com>; Anup Patel
> <Anup.Patel@wdc.com>; akpm@linux-foundation.org; rppt@kernel.org;
> mhiramat@kernel.org; zong.li@sifive.com; guoren@linux.alibaba.com;
> wangkefeng.wang@huawei.com; 0x7f454c46@gmail.com;
> chenhuang5@huawei.com; linux-kernel@vger.kernel.org; kernel-
> team@android.com; Palmer Dabbelt <palmerdabbelt@google.com>
> Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*
> 
> On Fri, 30 Apr 2021 05:54:51 +0800
> Changbin Du <changbin.du@gmail.com> wrote:
> 
> > The problem is that lockdep cannot handle locks across tasks since we
> > use stopmachine to patch code for risc-v. So there's a false positive report.
> > See privious disscussion here
> 
> > https://lkml.org/lkml/2021/4/29/63
> 
> Please use lore.kernel.org, lkml.org is highly unreliable, and is considered
> deprecated for use of referencing linux kernel archives.
> 
> Would the following patch work?

This patch only takes care of ftrace path.

The RISC-V instruction patching is used by RISC-V jump label implementation
as well and will called from various critical parts of core kernel.

The RAW spinlock approach allows same instruction patching to be used
for kprobes, ftrace, and jump label.

Regards,
Anup

> 
> (note, I did not even compile test it)
> 
> -- Steve
> 
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 845002cc2e57..19acbb4aaeff 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -25,6 +25,8 @@ struct dyn_arch_ftrace {  };  #endif
> 
> +extern int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  /*
>   * A general call in RISC-V is a pair of insts:
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c index
> 7f1e5203de88..834ab4fad637 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -11,15 +11,19 @@
>  #include <asm/cacheflush.h>
>  #include <asm/patch.h>
> 
> +int running_ftrace;
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)  {
>  	mutex_lock(&text_mutex);
> +	running_ftrace = 1;
>  	return 0;
>  }
> 
>  int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
> {
> +	running_ftrace = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c index
> 0b552873a577..4cd1c79a9689 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -12,6 +12,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/fixmap.h>
>  #include <asm/patch.h>
> +#include <asm/ftrace.h>
> 
>  struct patch_insn {
>  	void *addr;
> @@ -59,8 +60,13 @@ static int patch_insn_write(void *addr, const void
> *insn, size_t len)
>  	 * 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.
> +	 *
> +	 * ftrace uses stop machine, and even though the text_mutex is
> +	 * held, the stop machine task that calls this function will not
> +	 * be the owner.
>  	 */
> -	lockdep_assert_held(&text_mutex);
> +	if (!running_ftrace)
> +		lockdep_assert_held(&text_mutex);
> 
>  	if (across_pages)
>  		patch_map(addr + len, FIX_TEXT_POKE1);

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2021-04-30  4:06 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29  6:17 [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE* Palmer Dabbelt
2021-04-29  6:17 ` Palmer Dabbelt
2021-04-29  7:55 ` Anup Patel
2021-04-29  7:55   ` Anup Patel
2021-04-29 16:30 ` Steven Rostedt
2021-04-29 16:30   ` Steven Rostedt
2021-04-29 21:54   ` Changbin Du
2021-04-29 21:54     ` Changbin Du
2021-04-30  2:47     ` Steven Rostedt
2021-04-30  2:47       ` Steven Rostedt
2021-04-30  4:06       ` Anup Patel [this message]
2021-04-30  4:06         ` Anup Patel
2021-04-30 11:34         ` Steven Rostedt
2021-04-30 11:34           ` Steven Rostedt
2021-04-30 19:44           ` Palmer Dabbelt
2021-04-30 19:44             ` Palmer Dabbelt
2021-05-06  7:11             ` Palmer Dabbelt
2021-05-06  7:11               ` Palmer Dabbelt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR04MB6207E102997326E6513416AB8D5E9@MN2PR04MB6207.namprd04.prod.outlook.com \
    --to=anup.patel@wdc.com \
    --cc=0x7f454c46@gmail.com \
    --cc=Atish.Patra@wdc.com \
    --cc=akpm@linux-foundation.org \
    --cc=aou@eecs.berkeley.edu \
    --cc=ardb@kernel.org \
    --cc=changbin.du@gmail.com \
    --cc=chenhuang5@huawei.com \
    --cc=guoren@linux.alibaba.com \
    --cc=jbaron@akamai.com \
    --cc=jpoimboe@redhat.com \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mhiramat@kernel.org \
    --cc=palmer@dabbelt.com \
    --cc=palmerdabbelt@google.com \
    --cc=paul.walmsley@sifive.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=rppt@kernel.org \
    --cc=wangkefeng.wang@huawei.com \
    --cc=zong.li@sifive.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.