Re: [PATCH 1/6] powerpc/code-patching: Implement generic text patching function

Le 16/09/2022 à 08:23, Benjamin Gray a écrit :

Adds a generic text patching mechanism for patches of 1, 2, 4, or 8
bytes. The patcher conditionally syncs the icache depending on if
the content will be executed (as opposed to, e.g., read-only data).

The `patch_instruction` function is reimplemented in terms of this
more generic function. This generic implementation allows patching of
arbitrary 64-bit data, whereas the original `patch_instruction` decided
the size based on the 'instruction' opcode, so was not suitable for
arbitrary data.

With CONFIG_STRICT_KERNEL_RWX, this patches causes a 15% time increase 
for activation/deactivation of ftrace.

Without CONFIG_STRICT_KERNEL_RWX, it doesn't build.

Is 'text' a good name ? For me text mean executable code. Should it be 
__patch_memory() ?


Why pass src as a void * ? This forces data to go via the stack. Can't 
you pass it as a 'long' ?

+{
+	if (virt_to_pfn(dest) != virt_to_pfn(dest + size - 1))
+		return -EFAULT;

Why do you need that new check ?

+
+	switch (size) {
+		case 1:
+			__put_kernel_nofault(dest, src, u8, failed);
+			break;
+		case 2:
+			__put_kernel_nofault(dest, src, u16, failed);
+			break;
+		case 4:
+			__put_kernel_nofault(dest, src, u32, failed);
+			break;
+		case 8:
+			__put_kernel_nofault(dest, src, u64, failed);
+			break;

Is case 8 needed for PPC32 ?

+	}

Do you catch it when size if none of 1,2,4,8 ?

+
+	asm ("dcbst 0, %0; sync" :: "r" (dest));

Maybe write it in C:

	dcbst(dest);
	mb(); /* sync */

+
+	if (is_exec)
+		asm ("icbi 0,%0; sync; isync" :: "r" (exec_addr));

Same, can be:

	if (is_exec) {
		icbi(exec_addr);
		mb(); /* sync */
		isync();
	}

Or keep it flat:

	if (!is_exec)
		return 0;

	icbi(exec_addr);
	mb(); /* sync */
	isync();

	return 0;

Leave a blank line after variables declaration.

  	pte_clear(&init_mm, text_poke_addr, pte);
  	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
+}
+
+static int do_patch_text(void *dest, const void *src, size_t size, bool is_exec)
+{
+	int err;
+	pte_t *pte;
+	u32 *patch_addr;
+
+	pte = start_text_patch(dest, &patch_addr);
+	err = __patch_text(patch_addr, src, size, is_exec, dest);
+	finish_text_patch(pte);

Why do you need to split this function in three parts ? I can't see the 
added value, all it does is reduce readability.

Did you check the impact of calling __this_cpu_read() twice ?

  
  	return err;
  }
  
-static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
+static int patch_text(void *dest, const void *src, size_t size, bool is_exec)

Same, do you need the source data to go via stack ?

  {
  	int err;
  	unsigned long flags;
  
-	/*
-	 * During early early boot patch_instruction is called
-	 * when text_poke_area is not ready, but we still need
-	 * to allow patching. We just do the plain old patching
-	 */
+	/* Make sure we aren't patching a freed init section */
+	if (static_branch_likely(&init_mem_is_free) && init_section_contains(dest, 4))
+		return 0;
+
  	if (!static_branch_likely(&poking_init_done))
-		return raw_patch_instruction(addr, instr);
+		return __patch_text(dest, src, size, is_exec, dest);
  
  	local_irq_save(flags);
-	err = __do_patch_instruction(addr, instr);
+	err = do_patch_text(dest, src, size, is_exec);
  	local_irq_restore(flags);
  
  	return err;
  }
+
+int patch_text_data(void *dest, const void *src, size_t size) {

{ must be on next line for functions start.

What is this function used for ?

+{
+	return memcpy(dest, src, size);
+}
+
  #endif /* CONFIG_STRICT_KERNEL_RWX */
  
  __ro_after_init DEFINE_STATIC_KEY_FALSE(init_mem_is_free);
  
  int patch_instruction(u32 *addr, ppc_inst_t instr)
  {
-	/* Make sure we aren't patching a freed init section */
-	if (static_branch_likely(&init_mem_is_free) && init_section_contains(addr, 4))
-		return 0;
-
-	return do_patch_instruction(addr, instr);
+	if (!ppc_inst_prefixed(instr)) {
+		u32 val = ppc_inst_val(instr);
+		return patch_text(addr, &val, sizeof(val), true);
+	} else {
+		u64 val = ppc_inst_as_ulong(instr);
+		return patch_text(addr, &val, sizeof(val), true);
+	}
  }
  NOKPROBE_SYMBOL(patch_instruction);