[PATCH v4 0/5] Add generic data patching functions

[PATCH v4 0/5] Add generic data patching functions

[Benjamin Gray]

Currently patch_instruction() bases the write length on the value being
written. If the value looks like a prefixed instruction it writes 8 bytes,
otherwise it writes 4 bytes. This makes it potentially buggy to use for
writing arbitrary data, as if you want to write 4 bytes but it decides to
write 8 bytes it may clobber the following memory or be unaligned and
trigger an oops if it tries to cross a page boundary.

To solve this, this series pulls out the size parameter to the 'top' of
the memory patching logic, and propagates it through the various functions.

The two sizes supported are int and long; this allows for patching
instructions and pointers on both ppc32 and ppc64. On ppc32 these are the
same size, so care is taken to only use the size parameter on static
functions, so the compiler can optimise it out entirely. Unfortunately
GCC trips over its own feet here and won't optimise in a way that is
optimal for strict RWX (mpc85xx_smp_defconfig) and no RWX
(pmac32_defconfig). More details in the v2 cover letter.

Changes from v3:
  * Improved the boot test. Now that alignment is enforced,
    it checks the word (but not doubleword) aligned patch_ulong().

Changes from v2:
  * Various changes noted on each patch
  * Data patching now enforced to be aligned
  * Restore page aligned flushing optimisation

Changes from v1:
  * Addressed the v1 review actions
  * Removed noinline (for now)

v3: https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20240325055302.876434-1-bgray@linux.ibm.com/
v2: https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20231016050147.115686-1-bgray@linux.ibm.com/
v1: https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20230207015643.590684-1-bgray@linux.ibm.com/

Benjamin Gray (5):
  powerpc/code-patching: Add generic memory patching
  powerpc/code-patching: Add data patch alignment check
  powerpc/64: Convert patch_instruction() to patch_u32()
  powerpc/32: Convert patch_instruction() to patch_uint()
  powerpc/code-patching: Add boot selftest for data patching

 arch/powerpc/include/asm/code-patching.h | 37 +++++++++++++
 arch/powerpc/kernel/module_64.c          |  5 +-
 arch/powerpc/kernel/static_call.c        |  2 +-
 arch/powerpc/lib/code-patching.c         | 70 +++++++++++++++++++-----
 arch/powerpc/lib/test-code-patching.c    | 41 ++++++++++++++
 arch/powerpc/platforms/powermac/smp.c    |  2 +-
 6 files changed, 137 insertions(+), 20 deletions(-)

--
2.45.0

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

[Benjamin Gray]

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 <bgray@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++)
-- 
2.45.0

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 <bgray@linux.ibm.com>

Reviewed-by: Hari Bathini <hbathini@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++)

[PATCH v4 2/5] powerpc/code-patching: Add data patch alignment check

[Benjamin Gray]

The new data patching still needs to be aligned within a
cacheline too for the flushes to work correctly. To simplify
this requirement, we just say data patches must be aligned.

Detect when data patching is not aligned, returning an invalid
argument error.

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

---

v3: * New in v3
---
 arch/powerpc/include/asm/code-patching.h | 6 ++++++
 arch/powerpc/lib/code-patching.c         | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 21a36e2c4e26..e7f14720f630 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -95,11 +95,17 @@ int patch_ulong(void *addr, unsigned long val);
 
 static inline int patch_uint(void *addr, unsigned int val)
 {
+	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned int)))
+		return -EINVAL;
+
 	return patch_instruction(addr, ppc_inst(val));
 }
 
 static inline int patch_ulong(void *addr, unsigned long val)
 {
+	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)))
+		return -EINVAL;
+
 	return patch_instruction(addr, ppc_inst(val));
 }
 
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 18f762616db9..384b275d1bc5 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -386,12 +386,18 @@ NOKPROBE_SYMBOL(patch_instruction);
 
 int patch_uint(void *addr, unsigned int val)
 {
+	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned int)))
+		return -EINVAL;
+
 	return patch_mem(addr, val, false);
 }
 NOKPROBE_SYMBOL(patch_uint);
 
 int patch_ulong(void *addr, unsigned long val)
 {
+	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)))
+		return -EINVAL;
+
 	return patch_mem(addr, val, true);
 }
 NOKPROBE_SYMBOL(patch_ulong);
-- 
2.45.0

Re: [PATCH v4 2/5] powerpc/code-patching: Add data patch alignment check

[Hari Bathini]

On 15/05/24 8:14 am, Benjamin Gray wrote:
> The new data patching still needs to be aligned within a
> cacheline too for the flushes to work correctly. To simplify
> this requirement, we just say data patches must be aligned.
> 
> Detect when data patching is not aligned, returning an invalid
> argument error.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

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

> 
> ---
> 
> v3: * New in v3
> ---
>   arch/powerpc/include/asm/code-patching.h | 6 ++++++
>   arch/powerpc/lib/code-patching.c         | 6 ++++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 21a36e2c4e26..e7f14720f630 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -95,11 +95,17 @@ int patch_ulong(void *addr, unsigned long val);
>   
>   static inline int patch_uint(void *addr, unsigned int val)
>   {
> +	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned int)))
> +		return -EINVAL;
> +
>   	return patch_instruction(addr, ppc_inst(val));
>   }
>   
>   static inline int patch_ulong(void *addr, unsigned long val)
>   {
> +	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)))
> +		return -EINVAL;
> +
>   	return patch_instruction(addr, ppc_inst(val));
>   }
>   
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 18f762616db9..384b275d1bc5 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -386,12 +386,18 @@ NOKPROBE_SYMBOL(patch_instruction);
>   
>   int patch_uint(void *addr, unsigned int val)
>   {
> +	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned int)))
> +		return -EINVAL;
> +
>   	return patch_mem(addr, val, false);
>   }
>   NOKPROBE_SYMBOL(patch_uint);
>   
>   int patch_ulong(void *addr, unsigned long val)
>   {
> +	if (!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)))
> +		return -EINVAL;
> +
>   	return patch_mem(addr, val, true);
>   }
>   NOKPROBE_SYMBOL(patch_ulong);

[PATCH v4 3/5] powerpc/64: Convert patch_instruction() to patch_u32()

[Benjamin Gray]

This use of patch_instruction() is working on 32 bit data, and can fail
if the data looks like a prefixed instruction and the extra write
crosses a page boundary. Use patch_u32() to fix the write size.

Fixes: 8734b41b3efe ("powerpc/module_64: Fix livepatching for RO modules")
Link: https://lore.kernel.org/all/20230203004649.1f59dbd4@yea/
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

v2: * Added the fixes tag, it seems appropriate even if the subject does
      mention a more robust solution being required.

patch_u64() should be more efficient, but judging from the bug report
it doesn't seem like the data is doubleword aligned.
---
 arch/powerpc/kernel/module_64.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 7112adc597a8..e9bab599d0c2 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -651,12 +651,11 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
 	// func_desc_t is 8 bytes if ABIv2, else 16 bytes
 	desc = func_desc(addr);
 	for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) {
-		if (patch_instruction(((u32 *)&entry->funcdata) + i,
-				      ppc_inst(((u32 *)(&desc))[i])))
+		if (patch_u32(((u32 *)&entry->funcdata) + i, ((u32 *)&desc)[i]))
 			return 0;
 	}
 
-	if (patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)))
+	if (patch_u32(&entry->magic, STUB_MAGIC))
 		return 0;
 
 	return 1;
-- 
2.45.0

Re: [PATCH v4 3/5] powerpc/64: Convert patch_instruction() to patch_u32()

[Hari Bathini]

On 15/05/24 8:14 am, Benjamin Gray wrote:
> This use of patch_instruction() is working on 32 bit data, and can fail
> if the data looks like a prefixed instruction and the extra write
> crosses a page boundary. Use patch_u32() to fix the write size.
> 
> Fixes: 8734b41b3efe ("powerpc/module_64: Fix livepatching for RO modules")
> Link: https://lore.kernel.org/all/20230203004649.1f59dbd4@yea/
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Tested-by: Hari Bathini <hbathini@linux.ibm.com>

> 
> ---
> 
> v2: * Added the fixes tag, it seems appropriate even if the subject does
>        mention a more robust solution being required.
> 
> patch_u64() should be more efficient, but judging from the bug report
> it doesn't seem like the data is doubleword aligned.
> ---
>   arch/powerpc/kernel/module_64.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 7112adc597a8..e9bab599d0c2 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -651,12 +651,11 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
>   	// func_desc_t is 8 bytes if ABIv2, else 16 bytes
>   	desc = func_desc(addr);
>   	for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) {
> -		if (patch_instruction(((u32 *)&entry->funcdata) + i,
> -				      ppc_inst(((u32 *)(&desc))[i])))
> +		if (patch_u32(((u32 *)&entry->funcdata) + i, ((u32 *)&desc)[i]))
>   			return 0;
>   	}
>   
> -	if (patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)))
> +	if (patch_u32(&entry->magic, STUB_MAGIC))
>   		return 0;
>   
>   	return 1;

[PATCH v4 4/5] powerpc/32: Convert patch_instruction() to patch_uint()

[Benjamin Gray]

These changes are for patch_instruction() uses on data. Unlike ppc64
these should not be incorrect as-is, but using the patch_uint() alias
better reflects what kind of data being patched and allows for
benchmarking the effect of different patch_* implementations (e.g.,
skipping instruction flushing when patching data).

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/kernel/static_call.c     | 2 +-
 arch/powerpc/platforms/powermac/smp.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
index 863a7aa24650..1502b7e439ca 100644
--- a/arch/powerpc/kernel/static_call.c
+++ b/arch/powerpc/kernel/static_call.c
@@ -17,7 +17,7 @@ void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
 	mutex_lock(&text_mutex);
 
 	if (func && !is_short) {
-		err = patch_instruction(tramp + PPC_SCT_DATA, ppc_inst(target));
+		err = patch_ulong(tramp + PPC_SCT_DATA, target);
 		if (err)
 			goto out;
 	}
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 15644be31990..d21b681f52fb 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -827,7 +827,7 @@ static int smp_core99_kick_cpu(int nr)
 	mdelay(1);
 
 	/* Restore our exception vector */
-	patch_instruction(vector, ppc_inst(save_vector));
+	patch_uint(vector, save_vector);
 
 	local_irq_restore(flags);
 	if (ppc_md.progress) ppc_md.progress("smp_core99_kick_cpu done", 0x347);
-- 
2.45.0

Re: [PATCH v4 4/5] powerpc/32: Convert patch_instruction() to patch_uint()

[Hari Bathini]

On 15/05/24 8:14 am, Benjamin Gray wrote:
> These changes are for patch_instruction() uses on data. Unlike ppc64
> these should not be incorrect as-is, but using the patch_uint() alias
> better reflects what kind of data being patched and allows for
> benchmarking the effect of different patch_* implementations (e.g.,
> skipping instruction flushing when patching data).
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Tested-by: Hari Bathini <hbathini@linux.ibm.com>

> ---
>   arch/powerpc/kernel/static_call.c     | 2 +-
>   arch/powerpc/platforms/powermac/smp.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
> index 863a7aa24650..1502b7e439ca 100644
> --- a/arch/powerpc/kernel/static_call.c
> +++ b/arch/powerpc/kernel/static_call.c
> @@ -17,7 +17,7 @@ void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
>   	mutex_lock(&text_mutex);
>   
>   	if (func && !is_short) {
> -		err = patch_instruction(tramp + PPC_SCT_DATA, ppc_inst(target));
> +		err = patch_ulong(tramp + PPC_SCT_DATA, target);
>   		if (err)
>   			goto out;
>   	}
> diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
> index 15644be31990..d21b681f52fb 100644
> --- a/arch/powerpc/platforms/powermac/smp.c
> +++ b/arch/powerpc/platforms/powermac/smp.c
> @@ -827,7 +827,7 @@ static int smp_core99_kick_cpu(int nr)
>   	mdelay(1);
>   
>   	/* Restore our exception vector */
> -	patch_instruction(vector, ppc_inst(save_vector));
> +	patch_uint(vector, save_vector);
>   
>   	local_irq_restore(flags);
>   	if (ppc_md.progress) ppc_md.progress("smp_core99_kick_cpu done", 0x347);

[PATCH v4 5/5] powerpc/code-patching: Add boot selftest for data patching

[Benjamin Gray]

Extend the code patching selftests with some basic coverage of the new
data patching variants too.

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

---

v4: * Change store to a check
    * Account for doubleword alignment
v3: * New in v3
---
 arch/powerpc/lib/test-code-patching.c | 41 +++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/powerpc/lib/test-code-patching.c b/arch/powerpc/lib/test-code-patching.c
index f76030087f98..8cd3b32f805b 100644
--- a/arch/powerpc/lib/test-code-patching.c
+++ b/arch/powerpc/lib/test-code-patching.c
@@ -438,6 +438,46 @@ static void __init test_multi_instruction_patching(void)
 	vfree(buf);
 }
 
+static void __init test_data_patching(void)
+{
+	void *buf;
+	u32 *addr32;
+
+	buf = vzalloc(PAGE_SIZE);
+	check(buf);
+	if (!buf)
+		return;
+
+	addr32 = buf + 128;
+
+	addr32[1] = 0xA0A1A2A3;
+	addr32[2] = 0xB0B1B2B3;
+
+	check(!patch_uint(&addr32[1], 0xC0C1C2C3));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == 0xC0C1C2C3);
+	check(addr32[2] == 0xB0B1B2B3);
+	check(addr32[3] == 0);
+
+	/* Unaligned patch_ulong() should fail */
+	if (IS_ENABLED(CONFIG_PPC64))
+		check(patch_ulong(&addr32[1], 0xD0D1D2D3) == -EINVAL);
+
+	check(!patch_ulong(&addr32[2], 0xD0D1D2D3));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == 0xC0C1C2C3);
+	check(*(unsigned long *)(&addr32[2]) == 0xD0D1D2D3);
+
+	if (!IS_ENABLED(CONFIG_PPC64))
+		check(addr32[3] == 0);
+
+	check(addr32[4] == 0);
+
+	vfree(buf);
+}
+
 static int __init test_code_patching(void)
 {
 	pr_info("Running code patching self-tests ...\n");
@@ -448,6 +488,7 @@ static int __init test_code_patching(void)
 	test_translate_branch();
 	test_prefixed_patching();
 	test_multi_instruction_patching();
+	test_data_patching();
 
 	return 0;
 }
-- 
2.45.0

Re: [PATCH v4 5/5] powerpc/code-patching: Add boot selftest for data patching

[Hari Bathini]

On 15/05/24 8:14 am, Benjamin Gray wrote:
> Extend the code patching selftests with some basic coverage of the new
> data patching variants too.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

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

> 
> ---
> 
> v4: * Change store to a check
>      * Account for doubleword alignment
> v3: * New in v3
> ---
>   arch/powerpc/lib/test-code-patching.c | 41 +++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
> 
> diff --git a/arch/powerpc/lib/test-code-patching.c b/arch/powerpc/lib/test-code-patching.c
> index f76030087f98..8cd3b32f805b 100644
> --- a/arch/powerpc/lib/test-code-patching.c
> +++ b/arch/powerpc/lib/test-code-patching.c
> @@ -438,6 +438,46 @@ static void __init test_multi_instruction_patching(void)
>   	vfree(buf);
>   }
>   
> +static void __init test_data_patching(void)
> +{
> +	void *buf;
> +	u32 *addr32;
> +
> +	buf = vzalloc(PAGE_SIZE);
> +	check(buf);
> +	if (!buf)
> +		return;
> +
> +	addr32 = buf + 128;
> +
> +	addr32[1] = 0xA0A1A2A3;
> +	addr32[2] = 0xB0B1B2B3;
> +
> +	check(!patch_uint(&addr32[1], 0xC0C1C2C3));
> +
> +	check(addr32[0] == 0);
> +	check(addr32[1] == 0xC0C1C2C3);
> +	check(addr32[2] == 0xB0B1B2B3);
> +	check(addr32[3] == 0);
> +
> +	/* Unaligned patch_ulong() should fail */
> +	if (IS_ENABLED(CONFIG_PPC64))
> +		check(patch_ulong(&addr32[1], 0xD0D1D2D3) == -EINVAL);
> +
> +	check(!patch_ulong(&addr32[2], 0xD0D1D2D3));
> +
> +	check(addr32[0] == 0);
> +	check(addr32[1] == 0xC0C1C2C3);
> +	check(*(unsigned long *)(&addr32[2]) == 0xD0D1D2D3);
> +
> +	if (!IS_ENABLED(CONFIG_PPC64))
> +		check(addr32[3] == 0);
> +
> +	check(addr32[4] == 0);
> +
> +	vfree(buf);
> +}
> +
>   static int __init test_code_patching(void)
>   {
>   	pr_info("Running code patching self-tests ...\n");
> @@ -448,6 +488,7 @@ static int __init test_code_patching(void)
>   	test_translate_branch();
>   	test_prefixed_patching();
>   	test_multi_instruction_patching();
> +	test_data_patching();
>   
>   	return 0;
>   }

Re: [PATCH v4 0/5] Add generic data patching functions

[Michael Ellerman]

On Wed, 15 May 2024 12:44:40 +1000, Benjamin Gray wrote:
> Currently patch_instruction() bases the write length on the value being
> written. If the value looks like a prefixed instruction it writes 8 bytes,
> otherwise it writes 4 bytes. This makes it potentially buggy to use for
> writing arbitrary data, as if you want to write 4 bytes but it decides to
> write 8 bytes it may clobber the following memory or be unaligned and
> trigger an oops if it tries to cross a page boundary.
> 
> [...]

Applied to powerpc/next.

[1/5] powerpc/code-patching: Add generic memory patching
      https://git.kernel.org/powerpc/c/e6b8940e7e80cdfe98ba8493214922998920dd9c
[2/5] powerpc/code-patching: Add data patch alignment check
      https://git.kernel.org/powerpc/c/dbf828aab466c6534711d1f1454c409ea68d18d0
[3/5] powerpc/64: Convert patch_instruction() to patch_u32()
      https://git.kernel.org/powerpc/c/90d4fed5b273155c378b1d37595f2209f0a92bed
[4/5] powerpc/32: Convert patch_instruction() to patch_uint()
      https://git.kernel.org/powerpc/c/5799cd765fea93e643d81dbdae76a9c34e06dd18
[5/5] powerpc/code-patching: Add boot selftest for data patching
      https://git.kernel.org/powerpc/c/b7d47339d00d89af559a7068f4a640fc828177ad

cheers