Date:   Thu, 29 Apr 2021 13:25:14 +0530
From:   Anup Patel <anup@...infault.org>
To:     Palmer Dabbelt <palmer@...belt.com>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Peter Zijlstra <peterz@...radead.org>, jpoimboe@...hat.com,
        jbaron@...mai.com, rostedt@...dmis.org,
        Ard Biesheuvel <ardb@...nel.org>,
        Atish Patra <Atish.Patra@....com>,
        Anup Patel <Anup.Patel@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...nel.org>, mhiramat@...nel.org,
        Zong Li <zong.li@...ive.com>,
        Guo Ren <guoren@...ux.alibaba.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>, 0x7f454c46@...il.com,
        chenhuang5@...wei.com,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        kernel-team@...roid.com, Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Changbin Du <changbin.du@...il.com>
Subject: Re: [PATCH] RISC-V: insn: Use a raw spinlock to protect TEXT_POKE*

On Thu, Apr 29, 2021 at 11:50 AM Palmer Dabbelt <palmer@...belt.com> wrote:
>
> From: Palmer Dabbelt <palmerdabbelt@...gle.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@...il.com>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@...gle.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@...infault.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists