[PATCH v2 0/3] Add generic data patching functions

[PATCH v2 0/3] 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 text 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).

In the first case, patch_memory() is very large and can only be inlined
if a single function calls it. While the source only calls it in
patch_instruction(), an earlier optimisation pass inlined
patch_instruction() into patch_branch(), so now there are 'two' references
to patch_memory() and it cannot be inlined into patch_instruction().
Instead patch_instruction() becomes a single branch directly to
patch_memory().

We can fix this by marking patch_instruction() as noinline, but this
prevents patch_memory() from being directly inlined into patch_branch()
when RWX is disabled and patch_memory() is very small.

It may be possible to avoid this by merging together patch_instruction()
and patch_memory() on ppc32, but the only way I can think to do this
without duplicating the implementation involves using the preprocessor
to change if is_dword is a parameter or a local variable, which is gross.

For now I've removed the noinline, because at least the compiler might
get smarter in future and do the inlines correctly. If noinline remains
then there is no chance of it working.

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

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

Benjamin Gray (3):
  powerpc/code-patching: Add generic memory patching
  powerpc/64: Convert patch_instruction() to patch_u32()
  powerpc/32: Convert patch_instruction() to patch_uint()

 arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
 arch/powerpc/kernel/module_64.c          |  5 +-
 arch/powerpc/kernel/static_call.c        |  2 +-
 arch/powerpc/lib/code-patching.c         | 66 ++++++++++++++++++------
 arch/powerpc/platforms/powermac/smp.c    |  2 +-
 5 files changed, 87 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] 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, the
use cases for data patching (mainly module loading and static calls)
are less performance sensitive than for instruction patching
(ftrace activation). So the instruction flushing remains unconditional
in this patch.

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>

---

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

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 3f881548fb61..7c6056bb1706 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long target, int flags);
 int patch_instruction(u32 *addr, ppc_inst_t instr);
 int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
 
+/*
+ * patch_uint() and patch_ulong() should only be called on addresses where the
+ * patch does not cross a cacheline, otherwise it may not be flushed properly
+ * and mixes of new and stale data may be observed. It cannot cross a page
+ * boundary, as only the target page is mapped as writable.
+ *
+ * patch_instruction() and other instruction patchers automatically satisfy this
+ * requirement due to 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(u32 *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 b00112d7ad46..60289332412f 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_memory(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)) {
+		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);
 	}
 
@@ -43,7 +42,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_memory(addr, ppc_inst_as_ulong(instr), addr, true);
+	else
+		return __patch_memory(addr, ppc_inst_val(instr), addr, false);
 }
 
 struct patch_context {
@@ -278,7 +280,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_memory_mm(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	u32 *patch_addr;
@@ -307,7 +309,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_memory(addr, val, patch_addr, is_dword);
 
 	/* hwsync performed by __patch_instruction (sync) if successful */
 	if (err)
@@ -328,7 +330,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_memory(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	u32 *patch_addr;
@@ -345,7 +347,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_memory(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);
@@ -353,7 +355,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_memory(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	unsigned long flags;
@@ -365,18 +367,50 @@ 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_memory(addr, val, addr, is_dword);
 
 	local_irq_save(flags);
 	if (mm_patch_enabled())
-		err = __do_patch_instruction_mm(addr, instr);
+		err = __do_patch_memory_mm(addr, val, is_dword);
 	else
-		err = __do_patch_instruction(addr, instr);
+		err = __do_patch_memory(addr, val, is_dword);
 	local_irq_restore(flags);
 
 	return err;
 }
-NOKPROBE_SYMBOL(patch_instruction);
+
+#ifdef CONFIG_PPC64
+
+int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+	if (ppc_inst_prefixed(instr))
+		return patch_memory(addr, ppc_inst_as_ulong(instr), true);
+	else
+		return patch_memory(addr, ppc_inst_val(instr), false);
+}
+NOKPROBE_SYMBOL(patch_instruction)
+
+int patch_uint(void *addr, unsigned int val)
+{
+	return patch_memory(addr, val, false);
+}
+NOKPROBE_SYMBOL(patch_uint)
+
+int patch_ulong(void *addr, unsigned long val)
+{
+	return patch_memory(addr, val, true);
+}
+NOKPROBE_SYMBOL(patch_ulong)
+
+#else
+
+int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+	return patch_memory(addr, ppc_inst_val(instr), false);
+}
+NOKPROBE_SYMBOL(patch_instruction)
+
+#endif
 
 int patch_branch(u32 *addr, unsigned long target, int flags)
 {
-- 
2.39.2

[PATCH v2 2/3] 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.39.2

[PATCH v2 3/3] 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 c83d1e14077e..cfc1cd10135d 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.39.2

Re: [PATCH v2 0/3] Add generic data patching functions

[Christophe Leroy]

Le 16/10/2023 à 07:01, Benjamin Gray a écrit :
> 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 text 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).
> 
> In the first case, patch_memory() is very large and can only be inlined
> if a single function calls it. While the source only calls it in
> patch_instruction(), an earlier optimisation pass inlined
> patch_instruction() into patch_branch(), so now there are 'two' references
> to patch_memory() and it cannot be inlined into patch_instruction().
> Instead patch_instruction() becomes a single branch directly to
> patch_memory().
> 
> We can fix this by marking patch_instruction() as noinline, but this
> prevents patch_memory() from being directly inlined into patch_branch()
> when RWX is disabled and patch_memory() is very small.
> 
> It may be possible to avoid this by merging together patch_instruction()
> and patch_memory() on ppc32, but the only way I can think to do this
> without duplicating the implementation involves using the preprocessor
> to change if is_dword is a parameter or a local variable, which is gross.

What about:

diff --git a/arch/powerpc/include/asm/code-patching.h 
b/arch/powerpc/include/asm/code-patching.h
index 7c6056bb1706..af89ef450c93 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -72,7 +72,7 @@ static inline int create_branch(ppc_inst_t *instr, 
const u32 *addr,
  int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
  		       unsigned long target, int flags);
  int patch_branch(u32 *addr, unsigned long target, int flags);
-int patch_instruction(u32 *addr, ppc_inst_t instr);
+int patch_memory(void *addr, unsigned long val, bool is_dword);
  int raw_patch_instruction(u32 *addr, ppc_inst_t instr);

  /*
@@ -87,24 +87,28 @@ int raw_patch_instruction(u32 *addr, ppc_inst_t instr);

  #ifdef CONFIG_PPC64

-int patch_uint(void *addr, unsigned int val);
-int patch_ulong(void *addr, unsigned long val);
+int patch_instruction(u32 *addr, ppc_inst_t instr);

  #define patch_u64 patch_ulong

  #else

-static inline int patch_uint(u32 *addr, unsigned int val)
+static inline int patch_instruction(u32 *addr, ppc_inst_t instr)
  {
-	return patch_instruction(addr, ppc_inst(val));
+	return patch_memory(addr, ppc_inst_val(instr), false);
  }

+#endif
+
  static inline int patch_ulong(void *addr, unsigned long val)
  {
-	return patch_instruction(addr, ppc_inst(val));
+	return patch_memory(addr, val, true);
  }

-#endif
+static inline int patch_uint(void *addr, unsigned int val)
+{
+	return patch_memory(addr, val, false);
+}

  #define patch_u32 patch_uint

diff --git a/arch/powerpc/lib/code-patching.c 
b/arch/powerpc/lib/code-patching.c
index 60289332412f..77418b2a4aa4 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -355,7 +355,7 @@ static int __do_patch_memory(void *addr, unsigned 
long val, bool is_dword)
  	return err;
  }

-static int patch_memory(void *addr, unsigned long val, bool is_dword)
+int patch_memory(void *addr, unsigned long val, bool is_dword)
  {
  	int err;
  	unsigned long flags;
@@ -378,6 +378,7 @@ static int patch_memory(void *addr, unsigned long 
val, bool is_dword)

  	return err;
  }
+NOKPROBE_SYMBOL(patch_memory)

  #ifdef CONFIG_PPC64

@@ -390,26 +391,6 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
  }
  NOKPROBE_SYMBOL(patch_instruction)

-int patch_uint(void *addr, unsigned int val)
-{
-	return patch_memory(addr, val, false);
-}
-NOKPROBE_SYMBOL(patch_uint)
-
-int patch_ulong(void *addr, unsigned long val)
-{
-	return patch_memory(addr, val, true);
-}
-NOKPROBE_SYMBOL(patch_ulong)
-
-#else
-
-int patch_instruction(u32 *addr, ppc_inst_t instr)
-{
-	return patch_memory(addr, ppc_inst_val(instr), false);
-}
-NOKPROBE_SYMBOL(patch_instruction)
-
  #endif

  int patch_branch(u32 *addr, unsigned long target, int flags)


> 
> For now I've removed the noinline, because at least the compiler might
> get smarter in future and do the inlines correctly. If noinline remains
> then there is no chance of it working.
> 
> Changes from v1:
>    * Addressed the v1 review actions
>    * Removed noinline (for now)
> 
> v1: https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20230207015643.590684-1-bgray@linux.ibm.com/
> 
> Benjamin Gray (3):
>    powerpc/code-patching: Add generic memory patching
>    powerpc/64: Convert patch_instruction() to patch_u32()
>    powerpc/32: Convert patch_instruction() to patch_uint()
> 
>   arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
>   arch/powerpc/kernel/module_64.c          |  5 +-
>   arch/powerpc/kernel/static_call.c        |  2 +-
>   arch/powerpc/lib/code-patching.c         | 66 ++++++++++++++++++------
>   arch/powerpc/platforms/powermac/smp.c    |  2 +-
>   5 files changed, 87 insertions(+), 21 deletions(-)
> 

Re: [PATCH v2 0/3] Add generic data patching functions

[Benjamin Gray]

On 17/10/23 5:39 pm, Christophe Leroy wrote:
> Le 16/10/2023 à 07:01, Benjamin Gray a écrit :
>> 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 text 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).
>>
>> In the first case, patch_memory() is very large and can only be inlined
>> if a single function calls it. While the source only calls it in
>> patch_instruction(), an earlier optimisation pass inlined
>> patch_instruction() into patch_branch(), so now there are 'two' references
>> to patch_memory() and it cannot be inlined into patch_instruction().
>> Instead patch_instruction() becomes a single branch directly to
>> patch_memory().
>>
>> We can fix this by marking patch_instruction() as noinline, but this
>> prevents patch_memory() from being directly inlined into patch_branch()
>> when RWX is disabled and patch_memory() is very small.
>>
>> It may be possible to avoid this by merging together patch_instruction()
>> and patch_memory() on ppc32, but the only way I can think to do this
>> without duplicating the implementation involves using the preprocessor
>> to change if is_dword is a parameter or a local variable, which is gross.
> 
> What about:
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h
> b/arch/powerpc/include/asm/code-patching.h
> index 7c6056bb1706..af89ef450c93 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -72,7 +72,7 @@ static inline int create_branch(ppc_inst_t *instr,
> const u32 *addr,
>    int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
>    		       unsigned long target, int flags);
>    int patch_branch(u32 *addr, unsigned long target, int flags);
> -int patch_instruction(u32 *addr, ppc_inst_t instr);
> +int patch_memory(void *addr, unsigned long val, bool is_dword);
>    int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> 
>    /*
> @@ -87,24 +87,28 @@ int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> 
>    #ifdef CONFIG_PPC64
> 
> -int patch_uint(void *addr, unsigned int val);
> -int patch_ulong(void *addr, unsigned long val);
> +int patch_instruction(u32 *addr, ppc_inst_t instr);
> 
>    #define patch_u64 patch_ulong
> 
>    #else
> 
> -static inline int patch_uint(u32 *addr, unsigned int val)
> +static inline int patch_instruction(u32 *addr, ppc_inst_t instr)
>    {
> -	return patch_instruction(addr, ppc_inst(val));
> +	return patch_memory(addr, ppc_inst_val(instr), false);
>    }
> 
> +#endif
> +
>    static inline int patch_ulong(void *addr, unsigned long val)
>    {
> -	return patch_instruction(addr, ppc_inst(val));
> +	return patch_memory(addr, val, true);
>    }
> 
> -#endif
> +static inline int patch_uint(void *addr, unsigned int val)
> +{
> +	return patch_memory(addr, val, false);
> +}
> 
>    #define patch_u32 patch_uint
> 
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index 60289332412f..77418b2a4aa4 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -355,7 +355,7 @@ static int __do_patch_memory(void *addr, unsigned
> long val, bool is_dword)
>    	return err;
>    }
> 
> -static int patch_memory(void *addr, unsigned long val, bool is_dword)
> +int patch_memory(void *addr, unsigned long val, bool is_dword)
>    {
>    	int err;
>    	unsigned long flags;
> @@ -378,6 +378,7 @@ static int patch_memory(void *addr, unsigned long
> val, bool is_dword)
> 
>    	return err;
>    }
> +NOKPROBE_SYMBOL(patch_memory)
> 
>    #ifdef CONFIG_PPC64
> 
> @@ -390,26 +391,6 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
>    }
>    NOKPROBE_SYMBOL(patch_instruction)
> 
> -int patch_uint(void *addr, unsigned int val)
> -{
> -	return patch_memory(addr, val, false);
> -}
> -NOKPROBE_SYMBOL(patch_uint)
> -
> -int patch_ulong(void *addr, unsigned long val)
> -{
> -	return patch_memory(addr, val, true);
> -}
> -NOKPROBE_SYMBOL(patch_ulong)
> -
> -#else
> -
> -int patch_instruction(u32 *addr, ppc_inst_t instr)
> -{
> -	return patch_memory(addr, ppc_inst_val(instr), false);
> -}
> -NOKPROBE_SYMBOL(patch_instruction)
> -
>    #endif
> 
>    int patch_branch(u32 *addr, unsigned long target, int flags)
> 

Wouldn't every caller need to initialise the is_dword parameter in that 
case? It can't tell it's unused across a translation unit boundary 
without LTO.

Re: [PATCH v2 0/3] Add generic data patching functions

[Christophe Leroy]

Le 17/10/2023 à 08:56, Benjamin Gray a écrit :
> On 17/10/23 5:39 pm, Christophe Leroy wrote:
>> Le 16/10/2023 à 07:01, Benjamin Gray a écrit :
>>> 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 text 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).
>>>
>>> In the first case, patch_memory() is very large and can only be inlined
>>> if a single function calls it. While the source only calls it in
>>> patch_instruction(), an earlier optimisation pass inlined
>>> patch_instruction() into patch_branch(), so now there are 'two' 
>>> references
>>> to patch_memory() and it cannot be inlined into patch_instruction().
>>> Instead patch_instruction() becomes a single branch directly to
>>> patch_memory().
>>>
>>> We can fix this by marking patch_instruction() as noinline, but this
>>> prevents patch_memory() from being directly inlined into patch_branch()
>>> when RWX is disabled and patch_memory() is very small.
>>>
>>> It may be possible to avoid this by merging together patch_instruction()
>>> and patch_memory() on ppc32, but the only way I can think to do this
>>> without duplicating the implementation involves using the preprocessor
>>> to change if is_dword is a parameter or a local variable, which is 
>>> gross.
>>
>> What about:
>>
>> diff --git a/arch/powerpc/include/asm/code-patching.h
>> b/arch/powerpc/include/asm/code-patching.h
>> index 7c6056bb1706..af89ef450c93 100644
>> --- a/arch/powerpc/include/asm/code-patching.h
>> +++ b/arch/powerpc/include/asm/code-patching.h
>> @@ -72,7 +72,7 @@ static inline int create_branch(ppc_inst_t *instr,
>> const u32 *addr,
>>    int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
>>                   unsigned long target, int flags);
>>    int patch_branch(u32 *addr, unsigned long target, int flags);
>> -int patch_instruction(u32 *addr, ppc_inst_t instr);
>> +int patch_memory(void *addr, unsigned long val, bool is_dword);
>>    int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
>>
>>    /*
>> @@ -87,24 +87,28 @@ int raw_patch_instruction(u32 *addr, ppc_inst_t 
>> instr);
>>
>>    #ifdef CONFIG_PPC64
>>
>> -int patch_uint(void *addr, unsigned int val);
>> -int patch_ulong(void *addr, unsigned long val);
>> +int patch_instruction(u32 *addr, ppc_inst_t instr);
>>
>>    #define patch_u64 patch_ulong
>>
>>    #else
>>
>> -static inline int patch_uint(u32 *addr, unsigned int val)
>> +static inline int patch_instruction(u32 *addr, ppc_inst_t instr)
>>    {
>> -    return patch_instruction(addr, ppc_inst(val));
>> +    return patch_memory(addr, ppc_inst_val(instr), false);
>>    }
>>
>> +#endif
>> +
>>    static inline int patch_ulong(void *addr, unsigned long val)
>>    {
>> -    return patch_instruction(addr, ppc_inst(val));
>> +    return patch_memory(addr, val, true);
>>    }
>>
>> -#endif
>> +static inline int patch_uint(void *addr, unsigned int val)
>> +{
>> +    return patch_memory(addr, val, false);
>> +}
>>
>>    #define patch_u32 patch_uint
>>
>> diff --git a/arch/powerpc/lib/code-patching.c
>> b/arch/powerpc/lib/code-patching.c
>> index 60289332412f..77418b2a4aa4 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -355,7 +355,7 @@ static int __do_patch_memory(void *addr, unsigned
>> long val, bool is_dword)
>>        return err;
>>    }
>>
>> -static int patch_memory(void *addr, unsigned long val, bool is_dword)
>> +int patch_memory(void *addr, unsigned long val, bool is_dword)
>>    {
>>        int err;
>>        unsigned long flags;
>> @@ -378,6 +378,7 @@ static int patch_memory(void *addr, unsigned long
>> val, bool is_dword)
>>
>>        return err;
>>    }
>> +NOKPROBE_SYMBOL(patch_memory)
>>
>>    #ifdef CONFIG_PPC64
>>
>> @@ -390,26 +391,6 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
>>    }
>>    NOKPROBE_SYMBOL(patch_instruction)
>>
>> -int patch_uint(void *addr, unsigned int val)
>> -{
>> -    return patch_memory(addr, val, false);
>> -}
>> -NOKPROBE_SYMBOL(patch_uint)
>> -
>> -int patch_ulong(void *addr, unsigned long val)
>> -{
>> -    return patch_memory(addr, val, true);
>> -}
>> -NOKPROBE_SYMBOL(patch_ulong)
>> -
>> -#else
>> -
>> -int patch_instruction(u32 *addr, ppc_inst_t instr)
>> -{
>> -    return patch_memory(addr, ppc_inst_val(instr), false);
>> -}
>> -NOKPROBE_SYMBOL(patch_instruction)
>> -
>>    #endif
>>
>>    int patch_branch(u32 *addr, unsigned long target, int flags)
>>
> 
> Wouldn't every caller need to initialise the is_dword parameter in that 
> case? It can't tell it's unused across a translation unit boundary 
> without LTO.
> 

Ah yes you are right.

Christophe

Re: [PATCH v2 1/3] powerpc/code-patching: Add generic memory patching

[Naveen N Rao]

On Mon, Oct 16, 2023 at 04:01:45PM +1100, 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, the
> use cases for data patching (mainly module loading and static calls)
> are less performance sensitive than for instruction patching
> (ftrace activation).

That's debatable. While it is nice to be able to activate function 
tracing quickly, it is not necessarily a hot path. On the flip side, I 
do have a use case for data patching for ftrace activation :)

> So the instruction flushing remains unconditional
> in this patch.
> 
> 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>
> 
> ---
> 
> v2: * Deduplicate patch_32() definition
>     * Use u32 for val32
>     * Remove noinline
> ---
>  arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
>  arch/powerpc/lib/code-patching.c         | 66 ++++++++++++++++++------
>  2 files changed, 83 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 3f881548fb61..7c6056bb1706 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long target, int flags);
>  int patch_instruction(u32 *addr, ppc_inst_t instr);
>  int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
>  
> +/*
> + * patch_uint() and patch_ulong() should only be called on addresses where the
> + * patch does not cross a cacheline, otherwise it may not be flushed properly
> + * and mixes of new and stale data may be observed. It cannot cross a page
> + * boundary, as only the target page is mapped as writable.

Should we enforce alignment requirements, especially for patch_ulong() 
on 64-bit powerpc? I am not sure if there are use cases for unaligned 
64-bit writes. That should also ensure that the write doesn't cross a 
cacheline.

> + *
> + * patch_instruction() and other instruction patchers automatically satisfy this
> + * requirement due to 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(u32 *addr, unsigned int val)

Is there a reason to use u32 * here?

> +{
> +	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 b00112d7ad46..60289332412f 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_memory(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)) {
> +		u32 val32 = val;

Would be good to add a comment indicating the need for this for BE.

- Naveen

Re: [PATCH v2 2/3] powerpc/64: Convert patch_instruction() to patch_u32()

[Naveen N Rao]

On Mon, Oct 16, 2023 at 04:01:46PM +1100, 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>
> 
> ---
> 
> 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.

That doesn't look to be the case anymore due to commits 77e69ee7ce07 
("powerpc/64: modules support building with PCREL addresing") and 
7e3a68be42e1 ("powerpc/64: vmlinux support building with PCREL 
addresing")

- Naveen

Re: [PATCH v2 1/3] powerpc/code-patching: Add generic memory patching

[Benjamin Gray]

On Thu, 2023-11-30 at 15:55 +0530, Naveen N Rao wrote:
> On Mon, Oct 16, 2023 at 04:01:45PM +1100, 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,
> > the
> > use cases for data patching (mainly module loading and static
> > calls)
> > are less performance sensitive than for instruction patching
> > (ftrace activation).
> 
> That's debatable. While it is nice to be able to activate function 
> tracing quickly, it is not necessarily a hot path. On the flip side,
> I 
> do have a use case for data patching for ftrace activation :)
> 

True, but it's still correct to do so at least. Having a different path
for data writes is definitely a possibility, but would be added for
performance. This series is motivated by fixing a correctness issue
with data write sizes.

> > So the instruction flushing remains unconditional
> > in this patch.
> > 
> > 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>
> > 
> > ---
> > 
> > v2: * Deduplicate patch_32() definition
> >     * Use u32 for val32
> >     * Remove noinline
> > ---
> >  arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
> >  arch/powerpc/lib/code-patching.c         | 66 ++++++++++++++++++--
> > ----
> >  2 files changed, 83 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/code-patching.h
> > b/arch/powerpc/include/asm/code-patching.h
> > index 3f881548fb61..7c6056bb1706 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long
> > target, int flags);
> >  int patch_instruction(u32 *addr, ppc_inst_t instr);
> >  int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> >  
> > +/*
> > + * patch_uint() and patch_ulong() should only be called on
> > addresses where the
> > + * patch does not cross a cacheline, otherwise it may not be
> > flushed properly
> > + * and mixes of new and stale data may be observed. It cannot
> > cross a page
> > + * boundary, as only the target page is mapped as writable.
> 
> Should we enforce alignment requirements, especially for
> patch_ulong() 
> on 64-bit powerpc? I am not sure if there are use cases for unaligned
> 64-bit writes. That should also ensure that the write doesn't cross a
> cacheline.

Yeah, the current description is more just the technical restrictions,
not an endorsement of usage. If the caller isn't working with aligned
data, it seems unlikely it would still be cacheline aligned. The caller
should break it into 32bit patches if this is the case.

By enforce, are you suggesting a WARN_ON in the code too?

> > + *
> > + * patch_instruction() and other instruction patchers
> > automatically satisfy this
> > + * requirement due to 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(u32 *addr, unsigned int val)
> 
> Is there a reason to use u32 * here?
> 

No, fixed to void* for next series

> > +{
> > +	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 b00112d7ad46..60289332412f 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_memory(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)) {
> > +		u32 val32 = val;
> 
> Would be good to add a comment indicating the need for this for BE.
> 

Yup, added

> - Naveen
> 

Re: Re: [PATCH v2 1/3] powerpc/code-patching: Add generic memory patching

[Naveen N Rao]

On Mon, Feb 05, 2024 at 01:30:46PM +1100, Benjamin Gray wrote:
> On Thu, 2023-11-30 at 15:55 +0530, Naveen N Rao wrote:
> > On Mon, Oct 16, 2023 at 04:01:45PM +1100, Benjamin Gray wrote:
> > > 
> > > diff --git a/arch/powerpc/include/asm/code-patching.h
> > > b/arch/powerpc/include/asm/code-patching.h
> > > index 3f881548fb61..7c6056bb1706 100644
> > > --- a/arch/powerpc/include/asm/code-patching.h
> > > +++ b/arch/powerpc/include/asm/code-patching.h
> > > @@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long
> > > target, int flags);
> > >  int patch_instruction(u32 *addr, ppc_inst_t instr);
> > >  int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
> > >  
> > > +/*
> > > + * patch_uint() and patch_ulong() should only be called on
> > > addresses where the
> > > + * patch does not cross a cacheline, otherwise it may not be
> > > flushed properly
> > > + * and mixes of new and stale data may be observed. It cannot
> > > cross a page
> > > + * boundary, as only the target page is mapped as writable.
> > 
> > Should we enforce alignment requirements, especially for
> > patch_ulong() 
> > on 64-bit powerpc? I am not sure if there are use cases for unaligned
> > 64-bit writes. That should also ensure that the write doesn't cross a
> > cacheline.
> 
> Yeah, the current description is more just the technical restrictions,
> not an endorsement of usage. If the caller isn't working with aligned
> data, it seems unlikely it would still be cacheline aligned. The caller
> should break it into 32bit patches if this is the case.
> 
> By enforce, are you suggesting a WARN_ON in the code too?

No, just detecting and returning an error code should help detect 
incorrect usage.


- Naveen