Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
On Wed, 2022-11-02 at 11:13 +0100, Christophe Leroy wrote: > 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 > > > --- > > > 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 ? Disabling IRQ's also serves to prevent the writeable mapping being visible outside of the patching function from my understanding. But I would move the check inside the disabled section if I were keeping it.
Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
On Wed, 2022-11-02 at 09:43 +, Christophe Leroy wrote: > 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 ? It's required as much as any sanity check in the kernel. I agree running it all the time is not great though, I would prefer to make it a debug-only check. I've seen VM_WARN_ON be used for this purpose I think? It's especially useful now with churn on the code-patching code. I don't expect the new method to be unreliable—I wouldn't be submitting it if I did—but I'd much prefer to have an obvious tell if it does turn out so. But the above is all moot because we allow parallel patching, so the check is just plain incorrect.
Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
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 --- 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 */
Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
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 > --- > 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))); > + > return err; > } > #else /* !CONFIG_STRICT_KERNEL_RWX */
Re: [PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
It occurred to me that we don't require holding a lock when patching text. Many use cases do hold text_mutex, but it's not required, so it's possible for this warning to show false positives. If we do want text_mutex be held, then lockdep_assert_held(_mutex) ought to be added too.
[PATCH v9 4/7] powerpc/code-patching: Verify instruction patch succeeded
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. Signed-off-by: Benjamin Gray --- 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))); + return err; } #else /* !CONFIG_STRICT_KERNEL_RWX */ -- 2.37.3
