Re: [PATCH v4 1/5] powerpc/code-patching: Add generic memory patching

[Hari Bathini]

On 15/05/24 8:14 am, Benjamin Gray wrote:

patch_instruction() is designed for patching instructions in otherwise
readonly memory. Other consumers also sometimes need to patch readonly
memory, so have abused patch_instruction() for arbitrary data patches.

This is a problem on ppc64 as patch_instruction() decides on the patch
width using the 'instruction' opcode to see if it's a prefixed
instruction. Data that triggers this can lead to larger writes, possibly
crossing a page boundary and failing the write altogether.

Introduce patch_uint(), and patch_ulong(), with aliases patch_u32(), and
patch_u64() (on ppc64) designed for aligned data patches. The patch
size is now determined by the called function, and is passed as an
additional parameter to generic internals.

While the instruction flushing is not required for data patches, it
remains unconditional in this patch. A followup series is possible if
benchmarking shows fewer flushes gives an improvement in some
data-patching workload.

ppc32 does not support prefixed instructions, so is unaffected by the
original issue. Care is taken in not exposing the size parameter in the
public (non-static) interface, so the compiler can const-propagate it
away.

Signed-off-by: Benjamin Gray <bg...@linux.ibm.com>

Reviewed-by: Hari Bathini <hbath...@linux.ibm.com>

---

v3: * Rename from *_memory to *_mem
     * Change type of ppc32 patch_uint() address to void*
     * Explain introduction of val32 for big endian
     * Some formatting

v2: * Deduplicate patch_32() definition
     * Use u32 for val32
     * Remove noinline
---
  arch/powerpc/include/asm/code-patching.h | 31 ++++++++++++
  arch/powerpc/lib/code-patching.c         | 64 ++++++++++++++++++------
  2 files changed, 80 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h 
b/arch/powerpc/include/asm/code-patching.h
index 0e29ccf903d0..21a36e2c4e26 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -76,6 +76,37 @@ int patch_instruction(u32 *addr, ppc_inst_t instr);
  int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
  int patch_instructions(u32 *addr, u32 *code, size_t len, bool repeat_instr);

+/*

+ * The data patching functions patch_uint() and patch_ulong(), etc., must be
+ * called on aligned addresses.
+ *
+ * The instruction patching functions patch_instruction() and similar must be
+ * called on addresses satisfying instruction alignment requirements.
+ */
+
+#ifdef CONFIG_PPC64
+
+int patch_uint(void *addr, unsigned int val);
+int patch_ulong(void *addr, unsigned long val);
+
+#define patch_u64 patch_ulong
+
+#else
+
+static inline int patch_uint(void *addr, unsigned int val)
+{
+       return patch_instruction(addr, ppc_inst(val));
+}
+
+static inline int patch_ulong(void *addr, unsigned long val)
+{
+       return patch_instruction(addr, ppc_inst(val));
+}
+
+#endif
+
+#define patch_u32 patch_uint
+
  static inline unsigned long patch_site_addr(s32 *site)
  {
        return (unsigned long)site + *site;
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index df64343b9214..18f762616db9 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -20,15 +20,14 @@
  #include <asm/code-patching.h>
  #include <asm/inst.h>

-static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr)

+static int __patch_mem(void *exec_addr, unsigned long val, void *patch_addr, 
bool is_dword)
  {
-       if (!ppc_inst_prefixed(instr)) {
-               u32 val = ppc_inst_val(instr);
+       if (!IS_ENABLED(CONFIG_PPC64) || likely(!is_dword)) {
+               /* For big endian correctness: plain address would use the 
wrong half */
+               u32 val32 = val;

- __put_kernel_nofault(patch_addr, &val, u32, failed);

+               __put_kernel_nofault(patch_addr, &val32, u32, failed);
        } else {
-               u64 val = ppc_inst_as_ulong(instr);
-
                __put_kernel_nofault(patch_addr, &val, u64, failed);
        }

@@ -44,7 +43,10 @@ static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr int raw_patch_instruction(u32 *addr, ppc_inst_t instr)

  {
-       return __patch_instruction(addr, instr, addr);
+       if (ppc_inst_prefixed(instr))
+               return __patch_mem(addr, ppc_inst_as_ulong(instr), addr, true);
+       else
+               return __patch_mem(addr, ppc_inst_val(instr), addr, false);
  }

struct patch_context {

@@ -276,7 +278,7 @@ static void unmap_patch_area(unsigned long addr)
        flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
  }

-static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)

+static int __do_patch_mem_mm(void *addr, unsigned long val, bool is_dword)
  {
        int err;
        u32 *patch_addr;
@@ -305,7 +307,7 @@ static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t 
instr)

orig_mm = start_using_temp_mm(patching_mm); - err = __patch_instruction(addr, instr, patch_addr);

+       err = __patch_mem(addr, val, patch_addr, is_dword);

/* context synchronisation performed by __patch_instruction (isync or exception) */

        stop_using_temp_mm(patching_mm, orig_mm);
@@ -322,7 +324,7 @@ static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t 
instr)
        return err;
  }

-static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)

+static int __do_patch_mem(void *addr, unsigned long val, bool is_dword)
  {
        int err;
        u32 *patch_addr;
@@ -339,7 +341,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t 
instr)
        if (radix_enabled())
                asm volatile("ptesync": : :"memory");

- err = __patch_instruction(addr, instr, patch_addr);

+       err = __patch_mem(addr, val, patch_addr, is_dword);

pte_clear(&init_mm, text_poke_addr, pte);

        flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
@@ -347,7 +349,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t 
instr)
        return err;
  }

-int patch_instruction(u32 *addr, ppc_inst_t instr)

+static int patch_mem(void *addr, unsigned long val, bool is_dword)
  {
        int err;
        unsigned long flags;
@@ -359,19 +361,51 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
         */
        if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) ||
            !static_branch_likely(&poking_init_done))
-               return raw_patch_instruction(addr, instr);
+               return __patch_mem(addr, val, addr, is_dword);

local_irq_save(flags);

        if (mm_patch_enabled())
-               err = __do_patch_instruction_mm(addr, instr);
+               err = __do_patch_mem_mm(addr, val, is_dword);
        else
-               err = __do_patch_instruction(addr, instr);
+               err = __do_patch_mem(addr, val, is_dword);
        local_irq_restore(flags);

return err;

  }
+
+#ifdef CONFIG_PPC64
+
+int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+       if (ppc_inst_prefixed(instr))
+               return patch_mem(addr, ppc_inst_as_ulong(instr), true);
+       else
+               return patch_mem(addr, ppc_inst_val(instr), false);
+}
  NOKPROBE_SYMBOL(patch_instruction);

+int patch_uint(void *addr, unsigned int val)

+{
+       return patch_mem(addr, val, false);
+}
+NOKPROBE_SYMBOL(patch_uint);
+
+int patch_ulong(void *addr, unsigned long val)
+{
+       return patch_mem(addr, val, true);
+}
+NOKPROBE_SYMBOL(patch_ulong);
+
+#else
+
+int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+       return patch_mem(addr, ppc_inst_val(instr), false);
+}
+NOKPROBE_SYMBOL(patch_instruction)
+
+#endif
+
  static int patch_memset64(u64 *addr, u64 val, size_t count)
  {
        for (u64 *end = addr + count; addr < end; addr++)