Le 02/11/2022 à 10:43, Christophe Leroy a écrit :
Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
Verifies that if the instruction patching did not return an error then
the value stored at the given address to patch is now equal to the
instruction we patched it to.
Why do we need that verification ? Until now it wasn't necessary, can
you describe a failure that occurs because we don't have this
verification ? Or is that until now it was reliable but the new method
you are adding will not be as reliable as before ?
What worries me with that new verification is that you are reading a
memory address with is mostly only used for code execution. That means:
- You will almost always take a DATA TLB Miss, hence performance impact.
- If one day we want Exec-only text mappings, it will become problematic.
So really the question is, is that patch really required ?
Signed-off-by: Benjamin Gray <bg...@linux.ibm.com>
---
arch/powerpc/lib/code-patching.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/lib/code-patching.c
b/arch/powerpc/lib/code-patching.c
index 3b3b09d5d2e1..b0a12b2d5a9b 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -192,6 +192,8 @@ static int do_patch_instruction(u32 *addr,
ppc_inst_t instr)
err = __do_patch_instruction(addr, instr);
local_irq_restore(flags);
+ WARN_ON(!err && !ppc_inst_equal(instr, ppc_inst_read(addr)));
+
Another point: you are doing the check outside of IRQ disabled section,
is that correct ? What if an interrupt changed it in-between ?
Or insn't that possible ? In that case what's the real purpose of
disabling IRQs here ?
return err;
}
#else /* !CONFIG_STRICT_KERNEL_RWX */
