Re: new text patching for review

Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx> writes:

* Andi Kleen (ak@xxxxxxx) wrote:

Ewwwwwwwwwww.... you plan to run this in SMP ? So you actually go byte
by byte changing pieces of instructions non atomically and doing
non-Intel's errata friendly XMC. You are really looking for trouble
there :) Two distinct errors can occur:

In this case it is ok because this only happens when transitioning
from 1 CPU to 2 CPUs or vice versa and in both cases the other CPUs
are essentially stopped.


I agree that it's ok with SMP, but another problem arises: it's not only
a matter of being protected from SMP access, but also a matter of
reentrancy wrt interrupt handlers.

i.e.: if, as we are patching nops non atomically, we have a non maskable
interrupt coming which calls get_cycles_sync() which uses the

Hmm, i didn't think NMI handlers called that. e.g. nmi watchdog just
uses jiffies.

get_cycles_sync patching happens only relatively early at boot, so oprofile
cannot be running yet.

I see that IRQs are disabled in alternative_instructions(), but it does
not protect against NMIs, which could come at a very inappropriate
moment. MCE and SMIs would potentially cause the same kind of trouble.

So unless you can guarantee that any code from NMI handler won't call
basic things such as get_cycles() (nor MCE, nor SMIs), you can't insure
it won't execute an illegal instruction. Also, the option of temporarily
disabling the NMI for the duration of the update simply adds unwanted
latency to the NMI handler which could be unacceptable in some setups.

Ok it's a fair point. But how would you address it ?

Even if we IPIed the other CPUs NMIs or MCEs could still happen.

BTW Jeremy, have you ever considered that problem with paravirt ops
patching?


Another potential problem comes from preemption: if a thread is
preempted in the middle of the instructions you are modifying, let's say
we originally have 4 * 1 byte instructions that we replace with 1 * 4
byte instruction, a thread could iret in the middle of the new
instruction when it will be scheduled again, thus executing an illegal
instruction. This problem could be overcomed by putting the threads in
the freezer. See the djprobe project for details about this.

This cannot happen for the current code:
- full alternative patching happen only at boot when the other CPUs
are not running
- smp lock patching only ever changes a single byte (lock prefix) of
a single instruction
- kprobes only ever change a single byte

For the immediate value patching it also cannot happen because
you'll never modify multiple instructions and all immediate values
can be changed atomically.



In the end, I think the safest solution would be to limit ourselves to
one of these 2 solutions:
- If the update can be done atomically and leaves the site in a coherent
state after each atomic modification, while keeping the same
instruction size, it could be done on the spot.
- If not, care should be taken to first do a copy of the code to modify
into a "bypass", making sure that instructions can be relocated, so
that any context that would try to execute the site during the
modification would execute a breakpoint which would execute the bypass
while we modify the instructions.

kprobes does that, but to write the break point it uses text_poke()
with my patch.

Because of the tricks that must be done to do code modification on a
live system (as explained above), I don't think it makes sense to
provide a falsely-safe infrastructure that would allow code modification
in a non-atomic manner.

Hmm, perhaps it would make sense to add a comment warning about
the limitations of the current text_poke. Can you supply one?

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [PATCH 0/3] ring-buffer: less locking and only disable preemption
    ... but I'm not yet fully convinced about the NMI angle, ... now comes where an NMI can come in and execute the code being modified. ... Immediate Values - x86 Optimization NMI and MCE support ... It uses a breakpoint to bypass the instruction being changed, ...
    (Linux-Kernel)
  • Re: new text patching for review
    ... i didn't think NMI handlers called that. ... it won't execute an illegal instruction. ... Using a int3-based bypass is also ... required on Intel because of the erratum regarding instruction cache. ...
    (Linux-Kernel)
  • Re: [PATCH 0/5] ftrace: do not trace NMI handlers
    ... the other CPUs might execute that same code. ... by issuing an IPI to every CPU, issuing a cpuid instruction to ... Add notrace to functions called by NMI, ...
    (Linux-Kernel)
  • Re: [PATCH 0/5] ftrace: do not trace NMI handlers
    ... the other CPUs might execute that same code. ... by issuing an IPI to every CPU, issuing a cpuid instruction to ... Add notrace to functions called by NMI, ...
    (Linux-Kernel)
  • Re: programming language
    ... you will find the source code to my bf interpreter. ... instruction_pointer is the index of the instruction currently being executed in the instruction array. ... execute() is where the action happens. ... executegets a pointer to a bf_vm, where it executes one instruction, increments the instruction pointer of the bf_vm so that it points to the next instruction (or does a loop), and returns. ...
    (comp.programming)