From: Anup Patel <anup@brainfault.org> To: Palmer Dabbelt <palmer@dabbelt.com> Cc: linux-riscv <linux-riscv@lists.infradead.org>, Paul Walmsley <paul.walmsley@sifive.com>, Albert Ou <aou@eecs.berkeley.edu>, Peter Zijlstra <peterz@infradead.org>, jpoimboe@redhat.com, jbaron@akamai.com, rostedt@goodmis.org, Ard Biesheuvel <ardb@kernel.org>, Atish Patra <Atish.Patra@wdc.com>, Anup Patel <Anup.Patel@wdc.com>, Andrew Morton <akpm@linux-foundation.org>, Mike Rapoport <rppt@kernel.org>, mhiramat@kernel.org, Zong Li <zong.li@sifive.com>, Guo Ren <guoren@linux.alibaba.com>, Kefeng Wang <wangkefeng.wang@huawei.com>, 0x7f454c46@gmail.com, chenhuang5@huawei.com, "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>, kernel-team@android.com, Palmer Dabbelt <palmerdabbelt@google.com>, Changbin Du <changbin.du@gmail.com> Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE* Date: Thu, 29 Apr 2021 13:25:14 +0530 [thread overview] Message-ID: <CAAhSdy0Euh_rqpYkqB-_WtRmMJG5YpAk8nXghrz59-ARjD-9FA@mail.gmail.com> (raw) In-Reply-To: <20210429061713.783628-1-palmer@dabbelt.com> On Thu, Apr 29, 2021 at 11:50 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > From: Palmer Dabbelt <palmerdabbelt@google.com> > > We currently use text_mutex to protect the fixmap sections from > concurrent callers. This is convienent for kprobes as the generic code > already holds text_mutex, but ftrace doesn't which triggers a lockdep > assertion. We could take text_mutex for ftrace, but the jump label > implementation (which is currently taking text_mutex) isn't explicitly > listed as being sleepable and it's called from enough places it seems > safer to just avoid sleeping. > > arm64 and parisc, the other two TEXT_POKE-style patching > implemnetations, already use raw spinlocks. abffa6f3b157 ("arm64: > convert patch_lock to raw lock") lays out the case for a raw spinlock as > opposed to a regular spinlock, and while I don't know of anyone using rt > on RISC-V I'm sure it'll eventually show up and I don't see any reason > to wait. > > Fixes: ebc00dde8a97 ("riscv: Add jump-label implementation") > Reported-by: Changbin Du <changbin.du@gmail.com> > Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > arch/riscv/include/asm/fixmap.h | 3 +++ > arch/riscv/kernel/jump_label.c | 2 -- > arch/riscv/kernel/patch.c | 13 +++++++++---- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h > index 54cbf07fb4e9..d1c0a1f123cf 100644 > --- a/arch/riscv/include/asm/fixmap.h > +++ b/arch/riscv/include/asm/fixmap.h > @@ -24,8 +24,11 @@ enum fixed_addresses { > FIX_HOLE, > FIX_PTE, > FIX_PMD, > + > + /* Only used in kernel/insn.c */ > FIX_TEXT_POKE1, > FIX_TEXT_POKE0, > + > FIX_EARLYCON_MEM_BASE, > > __end_of_permanent_fixed_addresses, > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > index 20e09056d141..45bb32f91b5c 100644 > --- a/arch/riscv/kernel/jump_label.c > +++ b/arch/riscv/kernel/jump_label.c > @@ -35,9 +35,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > insn = RISCV_INSN_NOP; > } > > - mutex_lock(&text_mutex); > patch_text_nosync(addr, &insn, sizeof(insn)); > - mutex_unlock(&text_mutex); > } > > void arch_jump_label_transform_static(struct jump_entry *entry, > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > index 0b552873a577..dfa7ee8eb63f 100644 > --- a/arch/riscv/kernel/patch.c > +++ b/arch/riscv/kernel/patch.c > @@ -19,6 +19,8 @@ struct patch_insn { > atomic_t cpu_count; > }; > > +static DEFINE_RAW_SPINLOCK(patch_lock); > + > #ifdef CONFIG_MMU > /* > * The fix_to_virt(, idx) needs a const value (not a dynamic variable of > @@ -54,13 +56,14 @@ 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; > + unsigned long flags = 0; > > /* > - * 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. > + * FIX_TEXT_POKE{0,1} are only used for text patching, but we must > + * ensure that concurrent callers do not re-map these before we're done > + * with them. > */ > - lockdep_assert_held(&text_mutex); > + raw_spin_lock_irqsave(&patch_lock, flags); > > if (across_pages) > patch_map(addr + len, FIX_TEXT_POKE1); > @@ -74,6 +77,8 @@ static int patch_insn_write(void *addr, const void *insn, size_t len) > if (across_pages) > patch_unmap(FIX_TEXT_POKE1); > > + raw_spin_unlock_irqrestore(&patch_lock, flags); > + > return ret; > } > NOKPROBE_SYMBOL(patch_insn_write); > -- > 2.31.1.498.g6c1eba8ee3d-goog > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
WARNING: multiple messages have this Message-ID (diff)
From: Anup Patel <anup@brainfault.org> To: Palmer Dabbelt <palmer@dabbelt.com> Cc: linux-riscv <linux-riscv@lists.infradead.org>, Paul Walmsley <paul.walmsley@sifive.com>, Albert Ou <aou@eecs.berkeley.edu>, Peter Zijlstra <peterz@infradead.org>, jpoimboe@redhat.com, jbaron@akamai.com, rostedt@goodmis.org, Ard Biesheuvel <ardb@kernel.org>, Atish Patra <Atish.Patra@wdc.com>, Anup Patel <Anup.Patel@wdc.com>, Andrew Morton <akpm@linux-foundation.org>, Mike Rapoport <rppt@kernel.org>, mhiramat@kernel.org, Zong Li <zong.li@sifive.com>, Guo Ren <guoren@linux.alibaba.com>, Kefeng Wang <wangkefeng.wang@huawei.com>, 0x7f454c46@gmail.com, chenhuang5@huawei.com, "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>, kernel-team@android.com, Palmer Dabbelt <palmerdabbelt@google.com>, Changbin Du <changbin.du@gmail.com> Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE* Date: Thu, 29 Apr 2021 13:25:14 +0530 [thread overview] Message-ID: <CAAhSdy0Euh_rqpYkqB-_WtRmMJG5YpAk8nXghrz59-ARjD-9FA@mail.gmail.com> (raw) In-Reply-To: <20210429061713.783628-1-palmer@dabbelt.com> On Thu, Apr 29, 2021 at 11:50 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > From: Palmer Dabbelt <palmerdabbelt@google.com> > > We currently use text_mutex to protect the fixmap sections from > concurrent callers. This is convienent for kprobes as the generic code > already holds text_mutex, but ftrace doesn't which triggers a lockdep > assertion. We could take text_mutex for ftrace, but the jump label > implementation (which is currently taking text_mutex) isn't explicitly > listed as being sleepable and it's called from enough places it seems > safer to just avoid sleeping. > > arm64 and parisc, the other two TEXT_POKE-style patching > implemnetations, already use raw spinlocks. abffa6f3b157 ("arm64: > convert patch_lock to raw lock") lays out the case for a raw spinlock as > opposed to a regular spinlock, and while I don't know of anyone using rt > on RISC-V I'm sure it'll eventually show up and I don't see any reason > to wait. > > Fixes: ebc00dde8a97 ("riscv: Add jump-label implementation") > Reported-by: Changbin Du <changbin.du@gmail.com> > Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > arch/riscv/include/asm/fixmap.h | 3 +++ > arch/riscv/kernel/jump_label.c | 2 -- > arch/riscv/kernel/patch.c | 13 +++++++++---- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h > index 54cbf07fb4e9..d1c0a1f123cf 100644 > --- a/arch/riscv/include/asm/fixmap.h > +++ b/arch/riscv/include/asm/fixmap.h > @@ -24,8 +24,11 @@ enum fixed_addresses { > FIX_HOLE, > FIX_PTE, > FIX_PMD, > + > + /* Only used in kernel/insn.c */ > FIX_TEXT_POKE1, > FIX_TEXT_POKE0, > + > FIX_EARLYCON_MEM_BASE, > > __end_of_permanent_fixed_addresses, > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > index 20e09056d141..45bb32f91b5c 100644 > --- a/arch/riscv/kernel/jump_label.c > +++ b/arch/riscv/kernel/jump_label.c > @@ -35,9 +35,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > insn = RISCV_INSN_NOP; > } > > - mutex_lock(&text_mutex); > patch_text_nosync(addr, &insn, sizeof(insn)); > - mutex_unlock(&text_mutex); > } > > void arch_jump_label_transform_static(struct jump_entry *entry, > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > index 0b552873a577..dfa7ee8eb63f 100644 > --- a/arch/riscv/kernel/patch.c > +++ b/arch/riscv/kernel/patch.c > @@ -19,6 +19,8 @@ struct patch_insn { > atomic_t cpu_count; > }; > > +static DEFINE_RAW_SPINLOCK(patch_lock); > + > #ifdef CONFIG_MMU > /* > * The fix_to_virt(, idx) needs a const value (not a dynamic variable of > @@ -54,13 +56,14 @@ 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; > + unsigned long flags = 0; > > /* > - * 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. > + * FIX_TEXT_POKE{0,1} are only used for text patching, but we must > + * ensure that concurrent callers do not re-map these before we're done > + * with them. > */ > - lockdep_assert_held(&text_mutex); > + raw_spin_lock_irqsave(&patch_lock, flags); > > if (across_pages) > patch_map(addr + len, FIX_TEXT_POKE1); > @@ -74,6 +77,8 @@ static int patch_insn_write(void *addr, const void *insn, size_t len) > if (across_pages) > patch_unmap(FIX_TEXT_POKE1); > > + raw_spin_unlock_irqrestore(&patch_lock, flags); > + > return ret; > } > NOKPROBE_SYMBOL(patch_insn_write); > -- > 2.31.1.498.g6c1eba8ee3d-goog > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-04-29 7:55 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 [this message] 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 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=CAAhSdy0Euh_rqpYkqB-_WtRmMJG5YpAk8nXghrz59-ARjD-9FA@mail.gmail.com \ --to=anup@brainfault.org \ --cc=0x7f454c46@gmail.com \ --cc=Anup.Patel@wdc.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: linkBe 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.