jump label v4

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

Quoted 4 lines · Jason Baron

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

Quoted 8 lines

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

We (Intel OTC) have been able to get an *unofficial* answer as to the validity of this procedure; specifically as it applies to Intel hardware (obviously). We are working on getting an officially approved answer, but as far as we currently know, the procedure as outlined above should work on all Intel hardware. In fact, we believe the synchronization in step 3 is in fact unnecessary (as the synchronization in step 4 provides sufficient guard.)

In fact, if a suitable int3 handler is left permanently in place then step 5 is unnecessary as well. This would slow down other uses of int3 slightly, but might be a worthwhile tradeoff.

Such a permanent int3 handler would need to keep track of two potentially-spurious breakpoints: the current and the previous. The reason for needing two is that one could get a #BP from either the current or the previous modification site between the insertion of int3 and the synchronization in step 2. This, of course, assumes that the actual code poking is forcibly single-threaded (running under a spinlock or other mutex) -- if modifications are allowed to run in parallel you need to consider all possible current or stale #BP sites.

-hpa

Quoted 14 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 4 lines · Jason Baron

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

Quoted 8 lines

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

Quoted 7 lines

We (Intel OTC) have been able to get an *unofficial* answer as to the validity of this procedure; specifically as it applies to Intel hardware (obviously). We are working on getting an officially approved answer, but as far as we currently know, the procedure as outlined above should work on all Intel hardware. In fact, we believe the synchronization in step 3 is in fact unnecessary (as the synchronization in step 4 provides sufficient guard.)

Hi Peter,

This is great news! Thanks to Intel OTC and yourself for looking into this. In the immediate values patches, I am doing the synchronization at the end of step (3) to ensure that all remote CPUs issue read memory barriers, so the stores to the instruction are done in this order:

spin lock
store int3 to 1st byte
smp_wmb()
sync all cores
store new instruction in all but 1st byte
smp_wmb()
issue smp_rmb() on all cores (a sync all cores has this effect) store new instruction to 1st byte
send IPI to all cores (or call synchronize_sched()) to wait for all
breakpoint handlers to complete.
spin unlock

So the question is: are these wmb/rmb pairs actually needed ? As the instruction fetch is not performed by instructions per se, I doubt a rmb() will have any effect on them. I always prefer to stay on the safe side, but it wouldn't hurt to know.

Hrm. Assuming we have a spinlock protecting all this, given that we synchronize all cores at step (4) _after_ removing the breakpoint, and given that the breakpoint handler is an interrupt gate (thus executes with interrupts off), I am inclined to think that sending the IPIs at the end of step (4) (and waiting for them to complete) should be enough to ensure that all in-flight breakpoint handlers for this site have completed their execution. This would mean that we only have to keep track of a single site at a time. Or am I missing something ?

Quoted 42 lines · Mathieu Desnoyers

Quoted 14 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 4 lines · Jason Baron

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

Quoted 8 lines

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

Quoted 7 lines

We (Intel OTC) have been able to get an *unofficial* answer as to the validity of this procedure; specifically as it applies to Intel hardware (obviously). We are working on getting an officially approved answer, but as far as we currently know, the procedure as outlined above should work on all Intel hardware. In fact, we believe the synchronization in step 3 is in fact unnecessary (as the synchronization in step 4 provides sufficient guard.)

Hi Peter,

This is great news! Thanks to Intel OTC and yourself for looking into this. In the immediate values patches, I am doing the synchronization at the end of step (3) to ensure that all remote CPUs issue read memory barriers, so the stores to the instruction are done in this order:

spin lock
store int3 to 1st byte
smp_wmb()
sync all cores
store new instruction in all but 1st byte
smp_wmb()
issue smp_rmb() on all cores (a sync all cores has this effect) store new instruction to 1st byte
send IPI to all cores (or call synchronize_sched()) to wait for all
breakpoint handlers to complete.
spin unlock

Quoted 5 lines

So the question is: are these wmb/rmb pairs actually needed ? As the instruction fetch is not performed by instructions per se, I doubt a rmb() will have any effect on them. I always prefer to stay on the safe side, but it wouldn't hurt to know.

I don't think the smp_rmb() has any function.

However, you're being quite inconsistent in your terminology here. The assumption above is that the "synchronize code on all CPU" step is sending an IPI to all cores and waiting for it to return, so that each core has executed IPI/IRET before continuation.

It is *not* necessary to wait for the breakpoint handlers to return, as long as they will get to IRET eventually, since IRET is a jump and a serializing instruction.

Quoted 9 lines

Hrm. Assuming we have a spinlock protecting all this, given that we synchronize all cores at step (4) _after_ removing the breakpoint, and given that the breakpoint handler is an interrupt gate (thus executes with interrupts off), I am inclined to think that sending the IPIs at the end of step (4) (and waiting for them to complete) should be enough to ensure that all in-flight breakpoint handlers for this site have completed their execution. This would mean that we only have to keep track of a single site at a time. Or am I missing something ?

Yes: the whole point was that you can omit the synchronization in step 4 if you leave the breakpoint handler in place (I said "omit step 5", but that wasn't really what I meant.)

That means that at the cost of two compares in the standard #BP handler, we can get away with only one IPI per atomic instruction poke.

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Quoted 43 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 42 lines · Mathieu Desnoyers

Quoted 14 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 4 lines · Jason Baron

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

Quoted 8 lines

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

Quoted 7 lines

We (Intel OTC) have been able to get an *unofficial* answer as to the validity of this procedure; specifically as it applies to Intel hardware (obviously). We are working on getting an officially approved answer, but as far as we currently know, the procedure as outlined above should work on all Intel hardware. In fact, we believe the synchronization in step 3 is in fact unnecessary (as the synchronization in step 4 provides sufficient guard.)

Hi Peter,

This is great news! Thanks to Intel OTC and yourself for looking into this. In the immediate values patches, I am doing the synchronization at the end of step (3) to ensure that all remote CPUs issue read memory barriers, so the stores to the instruction are done in this order:

spin lock
store int3 to 1st byte
smp_wmb()
sync all cores
store new instruction in all but 1st byte
smp_wmb()
issue smp_rmb() on all cores (a sync all cores has this effect) store new instruction to 1st byte
send IPI to all cores (or call synchronize_sched()) to wait for all
breakpoint handlers to complete.
spin unlock

Quoted 7 lines

Quoted 5 lines

So the question is: are these wmb/rmb pairs actually needed ? As the instruction fetch is not performed by instructions per se, I doubt a rmb() will have any effect on them. I always prefer to stay on the safe side, but it wouldn't hurt to know.

I don't think the smp_rmb() has any function.

OK, that's good to know.

Quoted 5 lines

However, you're being quite inconsistent in your terminology here. The assumption above is that the "synchronize code on all CPU" step is sending an IPI to all cores and waiting for it to return, so that each core has executed IPI/IRET before continuation.

To be strictly correct, we cannot assume that the IPI handler issues IRET before signaling its completion. It's rather the other way around. This is why I add a smp_mb() in the IPI handler for the "synchronize code on all CPUs" step.

Quoted 4 lines

It is *not* necessary to wait for the breakpoint handlers to return, as long as they will get to IRET eventually, since IRET is a jump and a serializing instruction.

Ah, I see. So the added smp_mb() would not be needed then, as long as we know that the other CPUs either are currently running the IPI handler or have executed it. IOW: they will execute IRET very soon or they just executed it since the int3 have been written. I am a bit concerned about NMIs coming in this race window, but as they need to have started after we have put the breakpoint, that should be OK. (note: entry_*.S modifications are needed to support nesting breakpoint handlers in NMIs)

Quoted 13 lines

Quoted 9 lines

Hrm. Assuming we have a spinlock protecting all this, given that we synchronize all cores at step (4) _after_ removing the breakpoint, and given that the breakpoint handler is an interrupt gate (thus executes with interrupts off), I am inclined to think that sending the IPIs at the end of step (4) (and waiting for them to complete) should be enough to ensure that all in-flight breakpoint handlers for this site have completed their execution. This would mean that we only have to keep track of a single site at a time. Or am I missing something ?

Yes: the whole point was that you can omit the synchronization in step 4 if you leave the breakpoint handler in place (I said "omit step 5", but that wasn't really what I meant.)

Quoted 3 lines

That means that at the cost of two compares in the standard #BP handler, we can get away with only one IPI per atomic instruction poke.

OK. That makes sense now.

Quoted 3 lines

It is *not* necessary to wait for the breakpoint handlers to return, as long as they will get to IRET eventually, since IRET is a jump and a serializing instruction.

Ah, I see. So the added smp_mb() would not be needed then, as long as we know that the other CPUs either are currently running the IPI handler or have executed it. IOW: they will execute IRET very soon or they just executed it since the int3 have been written. I am a bit concerned about NMIs coming in this race window, but as they need to have started after we have put the breakpoint, that should be OK. (note: entry_*.S modifications are needed to support nesting breakpoint handlers in NMIs)

From Intel's Software Developer’s Manual Vol.3A 5.7.1 Handling Multiple NMIs

Quoted 11 lines · Mathieu Desnoyers

Quoted 3 lines

It is *not* necessary to wait for the breakpoint handlers to return, as long as they will get to IRET eventually, since IRET is a jump and a serializing instruction.

Ah, I see. So the added smp_mb() would not be needed then, as long as we know that the other CPUs either are currently running the IPI handler or have executed it. IOW: they will execute IRET very soon or they just executed it since the int3 have been written. I am a bit concerned about NMIs coming in this race window, but as they need to have started after we have put the breakpoint, that should be OK. (note: entry_*.S modifications are needed to support nesting breakpoint handlers in NMIs)

Hmm, if we support this to modify NMI code, it seems that we need to support not only nesting breakpoint handling but also nesting NMIs, because nesting NMI is unblocked when next IRET (of breakpoint) is issued.

From Intel's Software Developer’s Manual Vol.3A 5.7.1 Handling Multiple NMIs said below.
---
While an NMI interrupt handler is executing, the processor disables additional calls to the NMI handler until the next IRET instruction is executed. This blocking of subse- quent NMIs prevents stacking up calls to the NMI handler. [...]
---

I assume that your below patch tried to solve this issue, right?
http://lkml.indiana.edu/hypermail/linux/kernel/0804.1/0965.html

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 4 lines · Jason Baron

Add text_poke_fixup() which takes a fixup address to where a processor jumps if it hits the modifying address while code modifying. text_poke_fixup() does following steps for this purpose.

Quoted 8 lines

1. Setup int3 handler for fixup.
2. Put a breakpoint (int3) on the first byte of modifying region, and synchronize code on all CPUs.
3. Modify other bytes of modifying region, and synchronize code on all CPUs.
4. Modify the first byte of modifying region, and synchronize code on all CPUs.
5. Clear int3 handler.

We (Intel OTC) have been able to get an *unofficial* answer as to the validity of this procedure; specifically as it applies to Intel hardware (obviously). We are working on getting an officially approved answer, but as far as we currently know, the procedure as outlined above should work on all Intel hardware. In fact, we believe the synchronization in step 3 is in fact unnecessary (as the synchronization in step 4 provides sufficient guard.)

In fact, if a suitable int3 handler is left permanently in place then step 5 is unnecessary as well. This would slow down other uses of int3 slightly, but might be a worthwhile tradeoff.

Such a permanent int3 handler would need to keep track of two potentially-spurious breakpoints: the current and the previous. The reason for needing two is that one could get a #BP from either the current or the previous modification site between the insertion of int3 and the synchronization in step 2. This, of course, assumes that the actual code poking is forcibly single-threaded (running under a spinlock or other mutex) -- if modifications are allowed to run in parallel you need to consider all possible current or stale #BP sites.

arch/x86/kernel/alternative.c +10 -0

+/**
+ * text_poke_fixup() -- cross-modifying kernel text with fixup address.
+ * @addr:	Modifying address.
+ * @opcode:	New instruction.
+ * @len:	length of modifying bytes.
+ * @fixup:	Fixup address.
+ *
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.

arch/x86/kernel/alternative.c +25 -0

+
+	/* Preparing */
+	patch_fixup_addr = fixup;
+	wmb();
+	patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
+
+	/* Cap by an int3 */
+	text_poke(addr, &int3_insn, int3_size);
+	sync_core_all();
+
+	/* Replace tail bytes */
+	text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
+		  len - int3_size);
+	sync_core_all();
+
+	/* Replace int3 with head byte */
+	text_poke(addr, opcode, int3_size);
+	sync_core_all();
+
+	/* Cleanup */
+	patch_fixup_from = NULL;
+	wmb();
+	return addr;
+}
+

Quoted 16 lines · Jason Baron

arch/x86/kernel/alternative.c +16 -0

+/**
+ * text_poke_fixup() -- cross-modifying kernel text with fixup address.
+ * @addr:	Modifying address.
+ * @opcode:	New instruction.
+ * @len:	length of modifying bytes.
+ * @fixup:	Fixup address.
+ *
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.
+ */
+void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
+				void *fixup)
+{
+	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
+	static const int int3_size = sizeof(int3_insn);

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

-- Steve

Quoted 25 lines

arch/x86/kernel/alternative.c +25 -0

+
+	/* Preparing */
+	patch_fixup_addr = fixup;
+	wmb();
+	patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
+
+	/* Cap by an int3 */
+	text_poke(addr, &int3_insn, int3_size);
+	sync_core_all();
+
+	/* Replace tail bytes */
+	text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
+		  len - int3_size);
+	sync_core_all();
+
+	/* Replace int3 with head byte */
+	text_poke(addr, opcode, int3_size);
+	sync_core_all();
+
+	/* Cleanup */
+	patch_fixup_from = NULL;
+	wmb();
+	return addr;
+}
+

Quoted 18 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 16 lines · Jason Baron

arch/x86/kernel/alternative.c +16 -0

+/**
+ * text_poke_fixup() -- cross-modifying kernel text with fixup address.
+ * @addr:	Modifying address.
+ * @opcode:	New instruction.
+ * @len:	length of modifying bytes.
+ * @fixup:	Fixup address.
+ *
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.
+ */
+void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
+				void *fixup)
+{
+	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
+	static const int int3_size = sizeof(int3_insn);

Quoted 8 lines

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

It looks like we are thinking along the same lines.

I'm under the impression that I pointed out this exact same issue in the previous round of review a few weeks ago. Does this submission reflect the up-to-date state of this patch ?

Quoted 28 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 18 lines · Jason Baron

Quoted 17 lines

arch/x86/kernel/alternative.c +16 -0

+/**
+ * text_poke_fixup() -- cross-modifying kernel text with fixup address.
+ * @addr:	Modifying address.
+ * @opcode:	New instruction.
+ * @len:	length of modifying bytes.
+ * @fixup:	Fixup address.
+ *
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.
+ */
+void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
+				void *fixup)
+{
+	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
+	static const int int3_size = sizeof(int3_insn);

Quoted 8 lines

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 5 lines

It looks like we are thinking along the same lines.

I'm under the impression that I pointed out this exact same issue in the previous round of review a few weeks ago. Does this submission reflect the up-to-date state of this patch ?

No, the latest patch just skips step 3 if len == 1. (Jason, could you update your repository?) I thought I sent it the end of the last year ... :)

http://lkml.org/lkml/2009/12/18/312

Thank you,

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 9 lines · Steven Rostedt

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Could you clarify why you think that?

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Quoted 11 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 9 lines · Steven Rostedt

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 1 line

Could you clarify why you think that?

Basically, what Steven and I were concerned about in this particular patch version is the fact that this code took a "shortcut" for single-byte text modification, thus bypassing the int3-bypass scheme altogether.

Quoted 12 lines · On Sun, 17 Jan 2010 13:55:39 -0500 Mathieu Desnoyers…

Quoted 11 lines · * H. Peter Anvin ([email protected]) wrote:

Quoted 9 lines · Steven Rostedt

arch/x86/kernel/alternative.c void *__kprobes text_poke(void *addr, const void *opcode, size_t len) · line 614

Quoted 4 lines

arch/x86/kernel/alternative.c +4 -0

+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 6 lines

Quoted 1 line

Could you clarify why you think that?

Basically, what Steven and I were concerned about in this particular patch version is the fact that this code took a "shortcut" for single-byte text modification, thus bypassing the int3-bypass scheme altogether.

single byte instruction updates are likely 100x safer than any scheme of multi-byte instruction scheme that I have seen, other than a full stop_machine().

That does not mean it is safe, it just means it's an order of complexity less to analyze ;-)

Quoted 17 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 6 lines

Quoted 1 line

Could you clarify why you think that?

Basically, what Steven and I were concerned about in this particular patch version is the fact that this code took a "shortcut" for single-byte text modification, thus bypassing the int3-bypass scheme altogether.

single byte instruction updates are likely 100x safer than any scheme of multi-byte instruction scheme that I have seen, other than a full stop_machine().

Quoted 2 lines

That does not mean it is safe, it just means it's an order of complexity less to analyze ;-)

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

Quoted 20 lines · Masami Hiramatsu

Quoted 19 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

This part bothers me. The text_poke just writes over the text directly (using a separate mapping). But if that memory is in the pipeline of another CPU, I think this could cause a GPF.

Quoted 6 lines

Quoted 1 line

Could you clarify why you think that?

Basically, what Steven and I were concerned about in this particular patch version is the fact that this code took a "shortcut" for single-byte text modification, thus bypassing the int3-bypass scheme altogether.

single byte instruction updates are likely 100x safer than any scheme of multi-byte instruction scheme that I have seen, other than a full stop_machine().

That does not mean it is safe, it just means it's an order of complexity less to analyze ;-)

Quoted 3 lines

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

This really doesn't make much sense to me. The whole basis for the int3 scheme itself is that single-byte updates are atomic, so if single-byte updates can't work -- and as I stated, we at Intel OTC currently believe it safe -- then int3 can't work either.

The one thing to watch out for is that unless you force an IPI/IRET cycle afterwards, you can't know when any particular remote processor will see the update.

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Quoted 5 lines

This really doesn't make much sense to me. The whole basis for the int3 scheme itself is that single-byte updates are atomic, so if single-byte updates can't work -- and as I stated, we at Intel OTC currently believe it safe -- then int3 can't work either.

The additional characteristic of the int3 instruction (compared to the general case of a single-byte instruction) is that, when executed, it will trigger a trap, run a trap handler and return to the original code, typically with iret. This therefore implies that a serializing instruction is executed before returning to the instructions following the modification site when the breakpoint is hit.

So I hand out to Intel's expertise the question of whether single-byte instruction modification is safe or not in the general case. I'm just pointing out that I can very well imagine an aggressive superscalar architecture for which pipeline structure would support single-byte int3 patching without any problem due to the implied serialization, but would not support the general-case single-byte modification due to its lack of serialization.

As we might have to port this algorithm to Itanium in a near future, I prefer to stay on the safe side. Intel's "by the book" recommendation is more or less that a serializing instruction must be executed on all CPUs before new code is executed, without mention of single-vs-multi byte instructions. The int3-based bypass follows this requirement, but the single-byte code patching does not.

Unless there is a visible performance gain to special-case the single-byte instruction, I would recommend to stick to the safest solution, which follows Intel "official" guide-lines too.

Quoted 13 lines · Mathieu Desnoyers

Quoted 5 lines

This really doesn't make much sense to me. The whole basis for the int3 scheme itself is that single-byte updates are atomic, so if single-byte updates can't work -- and as I stated, we at Intel OTC currently believe it safe -- then int3 can't work either.

The additional characteristic of the int3 instruction (compared to the general case of a single-byte instruction) is that, when executed, it will trigger a trap, run a trap handler and return to the original code, typically with iret. This therefore implies that a serializing instruction is executed before returning to the instructions following the modification site when the breakpoint is hit.

Quoted 8 lines

So I hand out to Intel's expertise the question of whether single-byte instruction modification is safe or not in the general case. I'm just pointing out that I can very well imagine an aggressive superscalar architecture for which pipeline structure would support single-byte int3 patching without any problem due to the implied serialization, but would not support the general-case single-byte modification due to its lack of serialization.

This is utter and complete nonsense. You seem to think that everything
is guaranteed to hit the breakpoint, which is obviously false. Furthermore, until you have done the serialization, you're not guaranteed the *breakpoint* is seen, so you have the same condition.

Quoted 5 lines

This is utter and complete nonsense. You seem to think that everything
is guaranteed to hit the breakpoint, which is obviously false. Furthermore, until you have done the serialization, you're not guaranteed the *breakpoint* is seen, so you have the same condition.

In that time frame, I guess that the processor sees non-modified instruction and executes it. Since we'll wait until serializing on each processor, I think it is OK for int3-bypass method.

(Of course, this can depend on chip, it is possible that there is a chip
which causes a fault when it has a cache-discarding signal on current- instruction decoding slot. That's also why we are asking this method is OK for x86 processors.)

Quoted 13 lines · Mathieu Desnoyers

Quoted 5 lines

This really doesn't make much sense to me. The whole basis for the int3 scheme itself is that single-byte updates are atomic, so if single-byte updates can't work -- and as I stated, we at Intel OTC currently believe it safe -- then int3 can't work either.

The additional characteristic of the int3 instruction (compared to the general case of a single-byte instruction) is that, when executed, it will trigger a trap, run a trap handler and return to the original code, typically with iret. This therefore implies that a serializing instruction is executed before returning to the instructions following the modification site when the breakpoint is hit.

Quoted 8 lines

So I hand out to Intel's expertise the question of whether single-byte instruction modification is safe or not in the general case. I'm just pointing out that I can very well imagine an aggressive superscalar architecture for which pipeline structure would support single-byte int3 patching without any problem due to the implied serialization, but would not support the general-case single-byte modification due to its lack of serialization.

This is utter and complete nonsense. You seem to think that everything
is guaranteed to hit the breakpoint, which is obviously false.

Furthermore, until you have done the serialization, you're not guaranteed the *breakpoint* is seen,

so you have the same condition.

Quoted 11 lines

As we might have to port this algorithm to Itanium in a near future, I prefer to stay on the safe side. Intel's "by the book" recommendation is more or less that a serializing instruction must be executed on all CPUs before new code is executed, without mention of single-vs-multi byte instructions. The int3-based bypass follows this requirement, but the single-byte code patching does not.

Unless there is a visible performance gain to special-case the single-byte instruction, I would recommend to stick to the safest solution, which follows Intel "official" guide-lines too.

No, it doesn't. The only thing that follows the "official" guidelines is stop_machine.

As far as other architectures are concerned, other architectures can have very different and much stricter rules for I/D coherence. Trying to extrapolate from the x86 rules is aggravated insanity.

-hpa

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

Quoted 3 lines · On Mon, 18 Jan 2010 10:59:30 -0500 Masami Hiramatsu <[email protected]> wrote:

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

int3 is not making a difference for your case; there is no guarantee that the other processor even sees the "int3 inbetween state" at all; if it's not safe without int3 then it won't be safe with int3 either.

Signature · 4 lines

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Quoted 8 lines · * Arjan van de Ven ([email protected]) wrote:

Quoted 3 lines · On Mon, 18 Jan 2010 10:59:30 -0500 Masami Hiramatsu <[email protected]> wrote:

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

Quoted 3 lines

int3 is not making a difference for your case; there is no guarantee that the other processor even sees the "int3 inbetween state" at all; if it's not safe without int3 then it won't be safe with int3 either.

What Masami means is that he updated his patch to use the int3+IPI broadcast scheme.

Therefore, the CPUs not seeing the int3 inbetween state will be forced to issue a serializing instruction while the int3 is in place anyway.

Quoted 9 lines · Mathieu Desnoyers

Quoted 8 lines · * Arjan van de Ven ([email protected]) wrote:

Quoted 3 lines · On Mon, 18 Jan 2010 10:59:30 -0500 Masami Hiramatsu <[email protected]> wrote:

Yeah, so in the latest patch, I updated it to use int3 even if len == 1. :-)

Quoted 6 lines

Quoted 3 lines

int3 is not making a difference for your case; there is no guarantee that the other processor even sees the "int3 inbetween state" at all; if it's not safe without int3 then it won't be safe with int3 either.

What Masami means is that he updated his patch to use the int3+IPI broadcast scheme.

Right.

Quoted 3 lines

Therefore, the CPUs not seeing the int3 inbetween state will be forced to issue a serializing instruction while the int3 is in place anyway.

By the way, in kprobes, we just use a text_poke() to put int3. I assume that we'd better send IPI afterward, wouldn't it?

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: [email protected]