ftrace: Fix bug with function tracing and lockdep

Ingo,

Dave Jones reported a bug from ftrace. I saw the same bug but thought it was a compile problem because it would trigger everytime I booted, but after rebuilding the kernel, it stopped triggering. I mistook this as a build bug (something not cleaned properly).

What also confused me was that Dave also experienced the bug going away after doing a make clean.

Seems that was just shear luck!

After rebuilding and booting I was able to get it to trigger again. I found that it was something going wrong with the updating
of the functions via the breakpoints (the removal of stop machine for function tracing). I added a mdelay(1) where I usually sync the updates (add breakpoint, wait 1ms, modify code, wait 1ms, remove breakpoint) and this happened to trigger the bug 90% of the time.

Finally figured it out. It was a combination of lockdep and the int3 handler:

.macro paranoidzeroentry_ist sym do_sym ist
ENTRY(\sym)
INTR_FRAME
PARAVIRT_ADJUST_EXCEPTION_FRAME
pushq_cfi $-1 /* ORIG_RAX: no syscall to restart */
subq $ORIG_RAX-R15, %rsp
CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
call save_paranoid
TRACE_IRQS_OFF
movq %rsp,%rdi /* pt_regs pointer */
xorl %esi,%esi /* no error code */
subq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)
call \do_sym
addq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)
jmp paranoid_exit /* %ebx: no swapgs flag */
CFI_ENDPROC
END(\sym)
.endm

The above macro is called by the do_int3 code. When a int3 is hit, the stack jumps to the debug stack (like what happens on NMI). The
subq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)
is a little trick incase the \do_sym happens to hit a breakpoint too. And for the most part, that works.

But that TRACE_IRQS_OFF up there (and several more called by the paranoid_exit), is not protected by that trick. Thus, if the tracing code hits a breakpoint we reset the debug stack and start clobbering all over the previous code's stack.

Usually this is not an issue, unless lockdep sees the TRACE_IRQS_OFF or TRACE_IRQS_ON while holding locks that have been taken for the first time (with interrupts disabled). This explains why if the bug did not trigger on the first try of starting the function tracer, it usually would not trigger. It requires a new lock to be taken. The longer you keep the box running, the less likely it will acquire a lock for the first time.

Once lockdep sees a new lock changing its interrupts status, it starts recording it (and calling lots of code). Unfortunately, some of this code is traced by the function tracer.

I first started making a lot of this code 'notrace' but found that it just wasn't reliable enough. The list of functions to mark just kept growing, and I would only know if I got them all if I happen to run all the paths.

Then I tried to play tricks by doing a switch of the stack. I tried to modify sync_regs() but it would partially work, and it seemed that NMIs would cause it to crash as well. I spent too much time on it and finally figured to go with the easiest approach.

As this is only needed when LOCKDEP (and just in case, I did it when TRACE_IRQFLAGS was enabled), I figured breakpoint performance wasn't an issue (LOCKDEP and TRACE_IRQSFLAGS has a much bigger overhead).

I decided to reuse the code from the NMI to switch the IDT before calling the TRACE_IRQS_ON/OFF and switch it back afterward. It would be nice to do it just once on entering do_int3, and once leaving, but because the TRACE_IRQS_ON/OFF and TRACE_IRQS_IRETQ are called in several branches of the paranoid_exit, I decided to keep it simple and modify it every time.

In the future, we can work to make this rely on modifying_ftrace_code, but there's races to deal with, and this bug needs to be fixed now.

Also, while debugging this, I found a few more little bugs.

The first patch fixes a race with setting the modifying_ftrace_code and the executing of the int3 handlers.

The second fixes a bug where we don't do the breakpoint update of the function trace callback (the one called in the mcount trampoline). This could cause a GPF when tracing is running and the function tracer changes.

The third patch is a minor bug fix in the NMI handling of the idt switch. If a NMI preempts a int3 handler, it switches the idt, properly, and sets a variable to tell the handler to put it back when it exits. The problem is that the variable never got reset, and it constantly loaded the old IDT back on every NMI from then on. This is more of a performance and correctness fix.

The last two patches fix the lockdep and breakpoint issue.

Comments are welcomed, but if there's no problem then ...

Please pull the latest tip/perf/urgent tree, which can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent

Head SHA1: 1f9f5e93a9ab9cdf2504a25c24f3f8715c4ba785

Steven Rostedt (5):
ftrace: Synchronize variable setting with breakpoints
ftrace: Use breakpoint method to update ftrace caller
x86: Reset the debug_stack update counter
x86: Allow nesting of the debug stack IDT setting ftrace/x86: Do not change stacks in DEBUG when calling lockdep

----

 arch/x86/include/asm/ftrace.h |    2 +-
 arch/x86/kernel/cpu/common.c  |    8 ++++-
 arch/x86/kernel/entry_64.S    |   44 ++++++++++++++++++++++++--
 arch/x86/kernel/ftrace.c      |   69 ++++++++++++++++++++++++++++++++++++-----
 arch/x86/kernel/nmi.c         |    4 ++-
 arch/x86/kernel/traps.c       |    3 +-
 6 files changed, 116 insertions(+), 14 deletions(-)

From: Steven Rostedt <[email protected]>

When the function tracer starts modifying the code via breakpoints it sets a variable (modifying_ftrace_code) to inform the breakpoint handler to call the ftrace int3 code.

But there's no synchronization between setting this code and the handler, thus it is possible for the handler to be called on another CPU before it sees the variable. This will cause a kernel crash as the int3 handler will not know what to do with it.

I originally added smp_mb()'s to force the visibility of the variable but H. Peter Anvin suggested that I just make it atomic.

---
 arch/x86/include/asm/ftrace.h |    2 +-
 arch/x86/kernel/ftrace.c      |    6 +++---
 arch/x86/kernel/traps.c       |    3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)

index 18d9005..b0767bc 100644
@@ -34,7 +34,7 @@
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);
-extern int modifying_ftrace_code;
+extern atomic_t modifying_ftrace_code;
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {

index 32ff365..ea9904f 100644
@@ -168,7 +168,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ret;
 }
 
-int modifying_ftrace_code __read_mostly;
+atomic_t modifying_ftrace_code __read_mostly;
 
 /*
  * A breakpoint was added to the code address we are about to

@@ -491,11 +491,11 @@ void ftrace_replace_code(int enable)
 
 void arch_ftrace_update_code(int command)
 {
-	modifying_ftrace_code++;
+	atomic_inc(&modifying_ftrace_code);
 
 	ftrace_modify_all_code(command);
 
-	modifying_ftrace_code--;
+	atomic_dec(&modifying_ftrace_code);
 }
 
 int __init ftrace_dyn_arch_init(void *data)

index ff08457..32443ae 100644
@@ -304,7 +304,8 @@ dotraplinkage void __kprobes notrace do_int3(struct pt_regs *regs, long error_co
 {
 #ifdef CONFIG_DYNAMIC_FTRACE
 	/* ftrace must be first, everything else may cause a recursive crash */
-	if (unlikely(modifying_ftrace_code) && ftrace_int3_handler(regs))
+	if (unlikely(atomic_read(&modifying_ftrace_code)) &&
+	    ftrace_int3_handler(regs))
 		return;
 #endif
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP

1.7.10

Uhm,. maybe. atomic_{inc,dec}() implies a full memory barrier on x86, but atomic_read() never has the smp_rmb() required.

Now smp_rmb() is mostly a nop on x86, except for CONFIG_X86_PPRO_FENCE.

So this should mostly work, but yuck.

Also, why does this stuff live in ftrace? I always thought you were going to replace text_poke() so everybody that uses cross-modifying code could profit?

Yeah, I believe (and H. Peter can correct me) that this is all that's required for x86.

No rmb() is required, as that's supplied by the breakpoint itself. Basically, rmb() is used for ordering:

	load(A);
	rmb();
	loab(B);

To keep the machine from actually doing:

	load(B);
	load(A);

But what this is:

	<breakpoint>
	     |
	     +---------> <handler>
			    |
			load(A);

We need the load(A) to be after the breakpoint. Is it possible for the machine to do it before?:

		     load(A)
	      |
	      |
	<breakpoint>
	      +----------> test(A)

??

If another breakpoint is hit (one other than one put in by ftrace) then we don't care. It wont crash the system whether or not A is 1 or 0. We just need to make sure that a ftrace breakpoint that is hit knows that it was a ftrace breakpoint (calls the ftrace handler). No other breakpoint should be on a ftrace nop anyway.

It should work, not just mostly.

I discussed this with Masami at Collaboration Summit. The two are similar but also very different. But we want to start merging the two together where it makes sense.

Currently text_poke maps 2 pages into a FIXMAP area to make the modification so that it does not need to change the protection of the kernel (from ro to rw). Ftrace changes the text protection while it makes the update then reverts it back when done. The reason for the difference is that text_poke only handles 1 change at a time. There is a batch mode, but it does the mapping 1 at a time even for that.

Now ftrace must change 30,000 locations at once!

# cat /debug/tracing/available_filter_functions | wc -l
29467

Could you imagine ftrace mapping 30,000 pages one at a time to do the update?

Also, the text_poke() batch mode requires allocation of an array. This could cause failures to start function tracing to allocate 30,000 structures. Ftrace has its own tight structure that's done in blocks (the struct dyn_ftrace), that is (on x86_64) 16 byte items. This is allocated at boot up, and module load, and is persistent throughout the life of the system uptime (or module loaded). You can see it in the dmesg:

ftrace: allocating 20503 entries in 81 pages

- note this is just core code, before modules are added

The dyn_ftrace has a pointer to the mcount location, and a flags field. The flags tells the update code what to change.

Now Masami said he thinks the text_poke should also do the change of kernel protection as well, and not do the FIXMAP mapping. At least for bulk changes. The added breakpoint code that we have can be shared between ftrace and text_poke. That is something we want to do. After we get ftrace working, we'll go ahead and make it work for text_poke as well ;-)

-- Steve

Indeed, ftrace can be a better test case for breakpoint-based self- modifying. So after we can make sure that is safely work on x86 widely, it is better to rewrite text_poke with that method.

Perhaps, kprobes-*jump*-optimization may be better to handle it because the target probe is simultaneously working while optimizing (modifying code). This means that if someone hits breakpoint of such kprobe, it must be handled by kprobes, not only just tweaking IP address.

Thank you,

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Yeah, ftrace is easier on what the breakpoint does. For kprobes, it's a bit more invasive. Ftrace only deals with a nop, or a "trace-me" call. As the modifications are being done, we always treat it as a nop until the modification is complete.

With kprobes, the text_poke() is a bit more difficult, because it can't treat the location as a nop, as what is changing actually performs some task for the kernel. It requires that the breakpoint know what to do with the text (out-of-line execution, and such). ftrace has the benefit of just "skip this instruction" and return.

-- Steve

I know what rmb is for.. I also know you need to pair barriers. Hiding them in atomic doesn't make the ordering any more obvious.

I don't know, nor did you explain the implicit ordering there. Also in such diagrams you need the other side as well.

So the ordering is like:

---

CPU-0 CPU-1

lock inc mod-count /* implicit (w)mb */
write int3

				<trap-int3> /* implicit (r)mb */
				load mod-count

sync-ipi-broadcast
write rest-of-instruction
sync-ipi-broadcast
write head-of-instruction
sync-ipi-broadcast
lock dec mod-count /* implicit (w)mb */

Such that when we observe the int3 on CPU-1 we also must see the increment on mod-count.

---

A simple something like the above makes it very clear what we're doing and what we're expecting. I think a (local) trap should imply a barrier of sorts but will have to defer to others (hpa?) to confirm. But at the very least write it down someplace that you are assuming that.

fwiw run_sync() could do with a much bigger comment on why its sane to enable interrupts.. That simply reeks, enabling interrupts too early can wreck stuff properly.

Right, I forgot to add that.

You are obviously better at drawing such diagrams than I am. Which also explains why I needed to do the lockdep annotations to understand them better. Because I don't have your ability to visualize it in such a nice simple diagram. My vision is more of a bayesian tree than an oak. I guess that explains why I like complex code so much :-)

I would think it does, but lets have hpa or others confirm.

I'll cut and paste this into the comments.

Hmm, actually, I think these patches avoid the need for run_sync() at boot up now anyway. Thus it shouldn't need to enable interrupts.

The ftrace.c code was what disabled interrupts on start up (and re-enables them) before calling into this, this is because the old way needed interrupts disabled, and other archs still use the old way. But, your point stands, it should be commented better.

-- Steve

Argh,. I so disagree. You're doing it backwards.

First you merge whatever is there, regardless of who came first.

Then, when everybody doing text modification is using the same interface, do a second implementation using a Kconfig knob. If the scary new one breaks, no sweat, flip the config. If its proven stable, kill off the old one.

I really don't see why ftrace would be special here.. if you have all text_poke() users use the magic new way you'll have more coverage and better chances of hitting any snags if there are any.

The thing is, that's useful even if the new stuff never works. Having two code-bases doing cross-modifying code in different ways just doesn't sound right. That stuff is tricky enough as it is.

The comment about coming first was more about 're-inventing' then about merging. You can't reinvent something that didn't exist.

That said, I didn't even think about text poke while doing this. I was just simply thinking about removing stop_machine from ftrace, that required this. It was only a after thought that text_poke() could do the same. And this came up at Collab, where I thought, oh yeah! we can incorporate this with text poke.

What do you suggest then? To revert the code and rewrite it so that text_poke() does a similar thing?

-- Steve

Well, the fail is before that, how could we grow two pieces of code doing similar things in the first place?

But but but but.. the thing far back when Mathieu proposed the int3 scheme it was text_poke().. how.. did you not think of it this time!?

Too late for that now I guess.. I just wonder why you all thought it was a good idea to have two pieces of code doing cross-modifying-code. I always assumed ftrace used text_poke().

I hardly ever use dyn-ftrace but I do use some text_poke() through jump_labels.

I would still like to end up with one code base doing CMC with two implementations depending on a Kconfig knob.

Again, ftrace is slightly different as it does 30,000 changes at once, on top of known nops. This was done through stop_machine(), thus any slowdown was a large hit to system performance. text_poke() took the way of mapping a page to do the change, and Mathieu didn't want to change that (IIRC). But now we want the two to be similar.

Right, but we disagreed on its implementation, and yes, the idea came from that, but I thought text_poke() was so different that it wouldn't apply. It wasn't until Masami suggested changing text_poke() to be more like what ftrace does, that I really took thought into it.

Well, it was more of two different people working on two different things. We really didn't look too closely at each others work. It was more a social failure than a technical one.

You don't use function tracer? That's dyn-ftrace.

But still, we need to keep the record as small as possible because it is persistent throughout the life of the system running. Every location must be recorded, and maintain a state (flags).

Text_poke() mostly grew out of the jump-label work. But yes, there's still a lot that can be shared. The actual code modification may be.

You mean keep stop_machine around?

-- Steve

We could give text_poke a function argument to do the actual modification, leaving all the magic centralized.

Also, why did Mathieu insist on keeping that kmap()?

Not much no.. I do use trace_printk() and ftrace_dump_on_oops a lot though.

Afaicr we didn't change text_poke() for the jump-label stuff, except in trivial ways (added a #ifdef and exposed a function etc..).

Yeah, like have CONFIG_CMC_STOPMACHINE and CONFIG_CMC_FANCY for a little while.

If we find a problem with the fancy approach going back is easy, once its proven stable we could remove the stop-machine one.

Not sure about the entire context here, but the goal of using kmap() is to allow modification of text in configurations where the kernel text is read-only: the kmap does a temporary shadow RW mapping that allows modification of the text. Presumably that Ftrace's 30k changes are done before the kernel text mapping is set to read-only ? If this is the case, then it is similar to text_poke_early, which don't use the kmap since it happens before kernel text gets write-protected. But text_poke has to deal with RO pages.

Hopefully my answer makes sense in the context of your discussion.

Thanks,

Mathieu

Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

No this is also when ftrace is enabled at runtime. The trick that ftrace does is to temporarially convert the kernel text from ro to rw, and then back to ro when done. You can argue that this degrades the security of the system, but tracing every function in the kernel does too ;-) That's why it's a root only utility.

Hmm, this brings up another question. By default, perf does not allow users to profile trace_events correct? IOW, perf does not let unprivileged users call text_poke()? I just tried it and got the:

$ ~/bin/perf record -e sched:sched_switch sleep 1
Permission error - are you root?
Consider tweaking /proc/sys/kernel/perf_event_paranoid:
 -1 - Not paranoid at all
  0 - Disallow raw tracepoint access for unpriv
  1 - Disallow cpu events for unpriv
  2 - Disallow kernel profiling for unpriv

Almost,

-- Steve

Right, but when you loose stop-machine you could simply do 30k kmap_atomic/kunmap_atomic's consecutively since you're not holding anybody up.

It would, except tools/perf does stupid, its unconditionally adding PERF_SAMPLE_RAW (even for non-sampling events), which is the bit that requires privs.

It requires 3 IPIs per update too. Thus that's 90,000 IPIs you are blasting^Wsending to all CPUs.

-- Steve

Note, currently ftrace adds breakpoints to all locations, sends out the IPIs, modifies the code, sends out the IPIs and then removes the breakpoint, sends out the IPIs.

Just three IPIs for those 30,000 updates.

-- Steve

Uhm, no.

for_each() {
kmap_atomic()
frob int3
kunmap_atomic();
}

sync-ipi-broadcast();

for_each() {
kmap_atomic();
frob tail
kunmap_atomic();
}

sync-ipi-broadcast();

for_each() {
kmap_atomic();
frob head
kunmap_atomic();
};

sync-ipi-broadcast();

How is that sending 90k ipis?

----------------------------------+

----------------------------------+

Note, for the above time, the entire kernel has breakpoints added, and every function is taking a hit due to it. By slowing down this process, the rest of the system *will* be impacted. Ideally, we want to finish it as quick as possible. Not to mention, the kmap_atomics will be slowed down by the breakpoints that are attached to them.

-- Steve

Hmm, why don't we have two text_poke interfaces for single and batch? As like dyn_ftrace, since modifying massive points takes a lot time, so we may have additional kconfig something like CONFIG_QUICK_BATCH_TEXT_POKE which switches text area to rw while batch text_poke.

Thank you,

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

I'll be working on patches to consolidate the two after I get everything else working :-) I still need to work on the ftrace kprobe stuff too.

I hate having a config option to switch between the two, except for something that can be there if we find the new approach is buggy (like we have with lockdep). That may be a solution for this if we don't agree on one now. That is, bring back stop_machine() when LOCKDEP is enabled. But that should only be a temporary work around not a true fix.

I have no problem with having most of the modifying code be shared between text_poke and ftrace. I guess the question is, do we want to do it only with the FIXMAP or do we want text_poke and ftrace to use the rw method for large batches. Heck, we can set a limit. If we are going to update more that 100 locations, we switch the kernel to rw, otherwise we do the FIXMAP update.

-- Steve

That's reasonable for me :). You can feel free to change/remove text_poke_smp_batch which is already old-style interface.

Thanks!

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Em Thu, May 31, 2012 at 10:26:09PM +0200, Peter Zijlstra escreveu:

Looserland, bah, I'll check that, thanks.

- Arnaldo

From: Steven Rostedt <[email protected]>

On boot up and module load, it is fine to modify the code directly, without the use of breakpoints. This is because boot up modification is done before SMP is initialized, thus the modification is serial, and module load is done before the module executes.

But after that we must use a SMP safe method to modify running code.

This has been done to change the nops at all the functions, but the change of the ftrace callback handler itself was still using a direct modification. If tracing was enabled and the function callback was changed then another CPU could fault if it was currently calling the original callback. This modification must use the breakpoint method too.

Note, the direct method is still used for boot up and module load.

---
 arch/x86/kernel/ftrace.c |   65 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

index ea9904f..0d50ee2 100644
@@ -100,7 +100,7 @@ static const unsigned char *ftrace_nop_replace(void)
 }
 
 static int
-ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
+ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code,
 		   unsigned const char *new_code)
 {
 	unsigned char replaced[MCOUNT_INSN_SIZE];

@@ -141,7 +141,20 @@ int ftrace_make_nop(struct module *mod,
 	old = ftrace_call_replace(ip, addr);
 	new = ftrace_nop_replace();
 
-	return ftrace_modify_code(rec->ip, old, new);
+	/*
+	 * On boot up, and when modules are loaded, the MCOUNT_ADDR
+	 * is converted to a nop, and will never become MCOUNT_ADDR
+	 * again. This code is either running before SMP (on boot up)
+	 * or before the code will ever be executed (module load).
+	 * We do not want to use the breakpoint version in this case,
+	 * just modify the code directly.
+	 */
+	if (addr == MCOUNT_ADDR)
+		return ftrace_modify_code_direct(rec->ip, old, new);
+
+	/* Normal cases use add_brk_on_nop */
+	WARN_ONCE(1, "invalid use of ftrace_make_nop");
+	return -EINVAL;
 }
 
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)

@@ -152,9 +165,16 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	old = ftrace_nop_replace();
 	new = ftrace_call_replace(ip, addr);
 
-	return ftrace_modify_code(rec->ip, old, new);
+	/* Should only be called when module is loaded */
+	return ftrace_modify_code_direct(rec->ip, old, new);
 }
 
+atomic_t modifying_ftrace_code __read_mostly;
+
+static int
+ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
+		   unsigned const char *new_code);
+
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
 	unsigned long ip = (unsigned long)(&ftrace_call);

@@ -163,13 +183,16 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	memcpy(old, &ftrace_call, MCOUNT_INSN_SIZE);
 	new = ftrace_call_replace(ip, (unsigned long)func);
+
+	atomic_inc(&modifying_ftrace_code);
+
 	ret = ftrace_modify_code(ip, old, new);
 
+	atomic_dec(&modifying_ftrace_code);
+
 	return ret;
 }
 
-atomic_t modifying_ftrace_code __read_mostly;
-
 /*
  * A breakpoint was added to the code address we are about to
  * modify, and this is the handle that will just skip over it.

@@ -489,6 +512,38 @@ void ftrace_replace_code(int enable)
 	}
 }
 
+static int
+ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
+		   unsigned const char *new_code)
+{
+	int ret;
+
+	ret = add_break(ip, old_code);
+	if (ret)
+		goto out;
+
+	run_sync();
+
+	ret = add_update_code(ip, new_code);
+	if (ret)
+		goto fail_update;
+
+	run_sync();
+
+	ret = ftrace_write(ip, new_code, 1);
+	if (ret) {
+		ret = -EPERM;
+		goto out;
+	}
+	run_sync();
+ out:
+	return ret;
+
+ fail_update:
+	probe_kernel_write((void *)ip, &old_code[0], 1);
+	goto out;
+}
+
 void arch_ftrace_update_code(int command)
 {
 	atomic_inc(&modifying_ftrace_code);

1.7.10

The changelog isn't clear if this is a fix or optimization. I suspect the latter.

Still, you're now re-inventing text_poke() and text_poke_early().

Why are you keeping all this inside of ftrace?

It's a bug fix. There's a GPF kernel crash creeping around in the current code. I can add to the change log what I put into my cover letter:

"The second fixes a bug where we don't do the breakpoint update of the function trace callback (the one called in the mcount trampoline). This could cause a GPF when tracing is running and the function tracer changes."

I explained this in my other email. Also, it wasn't really re-invented, as I believe ftrace did this first (back in 2007). text_poke() looks to have come into play around 2008. Ftrace was the first to do live updates of code while the system was up and running. Before that, the alternatives code just changed the system at boot up (or going from SMP to UP or vice versa). It never did the change in an SMP environment.

This patchset is not a implementation of text_poke() but a fix to what was done earlier. The breakpoint method was something that text_poke() tried earlier, but H. Peter, warned that Intel has not said it was safe to do so. Thus we waited. When we got the unofficial OK, I added the breakpoint method to get rid of stop_machine() from ftrace. That was the main motivation for this.

I'm not ;-) Well, actually, I wont be. As I explained in my last email, I talked to Masami about it, and we are discussing ways to change text poke to do basically what ftrace is doing. I'm doing this with ftrace first because it removes stop_machine() from it, and it also a greater test case. If something is to go wrong, it will more likely go wrong with 30,000 changes at once, than just 100.

-- Steve

From: Steven Rostedt <[email protected]>

When an NMI goes off and it sees that it preempted the debug stack, to keep the debug stack safe, it changes the IDT to point to one that does not modify the stack on breakpoint (to allow breakpoints in NMIs).

But the variable that gets set to know to undo it on exit never gets cleared on exit. Thus every NMI will reset it on exit the first time it is done even if it does not need to be reset.

---
 arch/x86/kernel/nmi.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 9087527..c1fffc5 100644
@@ -450,8 +450,10 @@ static inline void nmi_nesting_preprocess(struct pt_regs *regs)
 
 static inline void nmi_nesting_postprocess(void)
 {
-	if (unlikely(__get_cpu_var(update_debug_stack)))
+	if (unlikely(__get_cpu_var(update_debug_stack))) {
 		debug_stack_reset();
+		__get_cpu_var(update_debug_stack) = 0;
+	}
 }
 #endif
 

1.7.10

Please don't use __get_cpu_var(); it causes a pointer to be manifest, which is free or almost free on most architectures but quite expensive on x86.

Instead use this_cpu_read()/this_cpu_write().

Consider:

#include <linux/types.h>
#include <linux/percpu.h>

static DEFINE_PER_CPU(int, percpu_test);

int read_foo(void)

{
	return __get_cpu_var(percpu_test);
}

void write_foo(int x)

{
	__get_cpu_var(percpu_test) = x;
}

int read_bar(void)

{
	return this_cpu_read(percpu_test);
}

void write_bar(int x)

{
	this_cpu_write(percpu_test, x);
}

... and the corresponding assembly code (with gcc boilerplate removed):

read_foo:
movq $percpu_test, %rax #, tcp_ptr__
add %gs:this_cpu_off, %rax # this_cpu_off, tcp_ptr__
movl (%rax), %eax # *D.8429_3, *D.8429_3
ret

write_foo:
movq $percpu_test, %rax #, tcp_ptr__
add %gs:this_cpu_off, %rax # this_cpu_off, tcp_ptr__
movl %edi, (%rax) # x, *D.8435_3
ret

read_bar:
movl %gs:percpu_test,%eax # percpu_test, pfo_ret__

write_bar:
movl %edi,%gs:percpu_test # x, percpu_test
ret

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

OK, will change.

Thanks!

-- Steve

From: Steven Rostedt <[email protected]>

When the NMI handler runs, it checks if it preempted a debug handler and if that handler is using the debug stack. If it is, it changes the IDT table not to update the stack, otherwise it will reset the debug stack and corrupt the debug handler it preempted.

Now that ftrace uses breakpoints to change functions from nops to callers, many more places may hit a breakpoint. Unfortunately this includes some of the calls that lockdep performs. Which causes issues with the debug stack. It too needs to change the debug stack before tracing (if called from the debug handler).

Allow the debug_stack_set_zero() and debug_stack_reset() to be nested so that the debug handlers can take advantage of them too.

---
 arch/x86/kernel/cpu/common.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 82f29e7..63fc083 100644
@@ -1101,14 +1101,20 @@ int is_debug_stack(unsigned long addr)
 		 addr > (__get_cpu_var(debug_stack_addr) - DEBUG_STKSZ));
 }
 
+static DEFINE_PER_CPU(atomic_t, debug_idt_zero);
+
 void debug_stack_set_zero(void)
 {
+	atomic_inc(&__get_cpu_var(debug_idt_zero));
 	load_idt((const struct desc_ptr *)&nmi_idt_descr);
 }
 
 void debug_stack_reset(void)
 {
-	load_idt((const struct desc_ptr *)&idt_descr);
+	if (WARN_ON(!atomic_read(&__get_cpu_var(debug_idt_zero))))
+		return;
+	if (atomic_dec_and_test(&__get_cpu_var(debug_idt_zero)))
+		load_idt((const struct desc_ptr *)&idt_descr);
 }
 
 #else	/* CONFIG_X86_64 */

1.7.10

NAK on this in its current form. A few minutes to let me write up a technical review.

-hpa

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

What you have here is a nesting counter, which is good, but I have some objections to the implementation.

The name "debug_idt_zero" doesn't convey a counter in any way, shape or form; perhaps debug_stack_users or something like that.

Since this is percpu, there is no reason to use the atomic operations (globally locked!), and since it is on the local CPU there is no need to manifest a pointer; we can instead use this_cpu_inc/dec_return:

static DEFINE_PER_CPU(u32, debug_stack_use_ctr);

void debug_stack_set_zero(void)

{
	if (this_cpu_inc_return(debug_stack_use_ctr) == 1)
	 	load_idt((const struct desc_ptr *)&nmi_idt_descr);
}

void debug_stack_reset(void)

{
	if (this_cpu_dec_return(debug_stack_use_ctr) == 0)
		load_idt((const struct desc_ptr *)&idt_descr);
}

-hpa

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

Well, it is 'debug_idt_SET_zero' which is what it is doing, regardless of counter (see below). But perhaps we should change it to:

debug_idt_get()
debug_idt_put()

?

If an NMI comes in here, it will not update the IDT and will corrupt the stack. The load_idt() must happen for all calls. There's only two static IDT tables. It's either one or the other. Thus, if it loads it twice, it will just set it to the same value. The counter is to know when to set it back.

-- Steve

Now I'm really confused. Why would it have to set it if it is already set?

-hpa

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

It doesn't ;-)

But we don't know if it is what it needs to be. Just because the counter is set to 1, doesn't mean that it was already set. Because we are dealing with NMIs, that are totally asynchronous, and the race of setting the counter and setting the idt.

Basically, (as you already know) the IST=0 means to use the same stack if we hit a breakpoint. But usually it's set to '4' which is an index into the TSS to find its per CPU stack.

If the IST is set to 4, and a breakpoint is hit, then the stack is set to a fixed address, even if you are currently using the same stack!

We need to prevent this from happening. There's two places that this can cause issues. The first is in the NMI handler, which is where this code was first developed. The idea was to allow NMIs to hit breakpoints. But if it were to do that after preempting a debug int3 handler, the breakpoint it hit would reset the stack and the NMI would start clobbering the stack of the code it preempted.

The NMI code on entering (and before it is allowed to hit any breakpoints) looks at the stack it preempted. If it is a debug stack, then it updates the IDT to have the int3 vector have a IST of 0 (keep the same stack when triggered). Then if the NMI hits a breakpoint, it just continues to use the NMI stack.

The int3 handler has a little trick that lets the int3 code reuse the stack. It modifies the TSS to point to another stack before calling the debug handler. If a NMI triggers now, or a breakpoint is hit again, it wont corrupt the stack.

subq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)
call \do_sym
addq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)

The debug stack is EXCEPTION_STKSZ * 2 size. Before entering the \do_sym (do_int3 in this case), it moves the TSS to point to another stack for the int3 handler.

The problem this patch set is trying to fix is the case for lockdep. The macro TRACE_IRQS_ON/OFF calls into lockdep code. And these are outside that add/sub TSS trick. If lockdep code hits a breakpoint than we reset back to the original stack address, and start clobbering the stack. This is the bug that Dave Jones was triggering.

Now we could do this add/sub TSS trick instead of loading idt for all the cases before calling lockdep in the debug handler. But this means the paranoid_exit will need to be turned into a macro and we would require it for each ist set.

Now, back to your original question. Why set it if it is already set. Well, it doesn't hurt to set it (except for the performance hit we take), but it does hurt if we should set it but don't, as that means we can reset the stack and clobber what we preempted.

I'm sure there's room to make this more efficient. But I'm currently trying to solve a kernel crash first, and then work on cleaning it up later.

-- Steve

Ouch. This is really way more complex than it has any excuse for being, and it's the complexity that concerns me, not the performance.

I'd like a chart, or list, of the alternate stack environments we can be in and what can transfer to what. I think there might be an easier solution that is more robust.

-hpa

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

Complexity is my Sun, and I am the planet that orbits around it.

Well, it's not as bad as one might think:

#define STACKFAULT_STACK 1
#define DOUBLEFAULT_STACK 2
#define NMI_STACK 3
#define DEBUG_STACK 4
#define MCE_STACK 5
#define N_EXCEPTION_STACKS 5 /* hw limit: 7 */

These are the exceptions that have their own stacks.

arch/x86/kernel/traps.c:        set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
arch/x86/kernel/traps.c:        set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
arch/x86/kernel/traps.c:        set_intr_gate_ist(X86_TRAP_NMI, &nmi, NMI_STACK);
arch/x86/kernel/traps.c:        set_intr_gate_ist(X86_TRAP_DF, &double_fault, DOUBLEFAULT_STACK);
arch/x86/kernel/traps.c:        set_intr_gate_ist(X86_TRAP_SS, &stack_segment, STACKFAULT_STACK);
arch/x86/kernel/traps.c:        set_intr_gate_ist(X86_TRAP_MC, &machine_check, MCE_STACK);

We only have two IDT tables that are fixed and are switched via the NMI handler (debug_stack_set_zero), as well as this patch set.

head_64.S:

ENTRY(idt_table)
.skip IDT_ENTRIES * 16

.align L1_CACHE_BYTES
ENTRY(nmi_idt_table)
.skip IDT_ENTRIES * 16

Only the DEBUG stack has a double size:

arch/x89/kernel/cpu/common.c:

static const unsigned int exception_stack_sizes[N_EXCEPTION_STACKS] = {
	  [0 ... N_EXCEPTION_STACKS - 1]	= EXCEPTION_STKSZ,
	  [DEBUG_STACK - 1]			= DEBUG_STKSZ
};

Thus only the debug stack does the stack TSS trick.

Is this what you were looking for? (God, it just shows how much time I've been spending on this crap, as I was able to find all this by memory and not grepping for it :-p )

-- Steve

That's why I asked instead of pulled it up myself.

However, what is missing is what the permitted transitions are. This affects the options.

-hpa

Only the debug stack changes. These happen in the two places I already mentioned.

One, the switch of the IDT in NMI (to convert the IST from 4 to 0, and just keep the same stack).

Two, is the trick in the paranoidzeroentry_ist code (adding the debug stack). That changes the DEBUG stack pointer.

-- Steve

No, I'm asking what environments (alternate stacks) are permitted where.
That is the important information.

The interaction of the above two things is part of the problem, of course, and I think it can (and should) be avoided.

-hpa

I believe that the IST stack switch is just a convenient way to allow separate stacks for separate events instead of trying to handle all events on a single stack, and risk stack overflow.

Thus, the debug/int3 and NMI events use a separate stack to not stress the current stack that is not expected to switch. The external interrupt stacks are switched on entry of the interrupt and stays on that stack and until its finished. There's a check to see if it is already on the stack before it does the switch so that it doesn't have the HW switch issues that NMI and int3 has.

Thus, if we say we must stay on either the NMI stack or the DEBUG stack, then this is what happens. Well, almost.

For NMIs, if it preempted something on the debug stack, it will always stay on the NMI stack and never switch. A check is made for both debug stacks at once:

int is_debug_stack(unsigned long addr)

{
	return __get_cpu_var(debug_stack_usage) ||
		(addr <= __get_cpu_var(debug_stack_addr) &&
		 addr > (__get_cpu_var(debug_stack_addr) - DEBUG_STKSZ));
}

Where:

#define DEBUG_STACK_ORDER (EXCEPTION_STACK_ORDER + 1) #define DEBUG_STKSZ (PAGE_SIZE << DEBUG_STACK_ORDER)

But if the NMI did not preempt the DEBUG stack, and it hits a breakpoint it will switch to the debug stack. And it is possible to make another switch if the debug code were to somehow.

Thus the transactions are something like this:

NORMAL_STACK (either kernel/user/irq) -> DEBUG1 -> DEBUG2

NORMAL_STACK -> DEBUG1 -> NMI

NORMAL_STACK -> DEBUG1 -> DEBUG2 -> NMI

NORMAL_STACK -> NMI -> DEBUG1 -> DEBUG2

-- Steve

And, yes, I can send a patch to change these to this_cpu_() if you like ;-)

-- Steve

Bah, silly me, I misunderstood you. You were talking about the counter itself, not the function names.

OK, will fix.

Thanks,

-- Steve

From: Steven Rostedt <[email protected]>

When both DYNAMIC_FTRACE and LOCKDEP are set, the TRACE_IRQS_ON/OFF will call into the lockdep code. The lockdep code can call lots of functions that may be traced by ftrace. When ftrace is updating its code and hits a breakpoint, the breakpoint handler will call into lockdep. If lockdep happens to call a function that also has a breakpoint attached, it will jump back into the breakpoint handler resetting the stack to the debug stack and corrupt the contents currently on that stack.

The 'do_sym' call that calls do_int3() is protected by modifying the IST table to point to a different location if another breakpoint is hit. But the TRACE_IRQS_OFF/ON are outside that protection, and if a breakpoint is hit from those, the stack will get corrupted, and the kernel will crash:

[ 1013.243754] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
[ 1013.272665] IP: [<ffff880145cc0000>] 0xffff880145cbffff
[ 1013.285186] PGD 1401b2067 PUD 14324c067 PMD 0
[ 1013.298832] Oops: 0010 [#1] PREEMPT SMP
[ 1013.310600] CPU 2
[ 1013.317904] Modules linked in: ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables crc32c_intel ghash_clmulni_intel microcode usb_debug serio_raw pcspkr iTCO_wdt i2c_i801 iTCO_vendor_support e1000e nfsd nfs_acl auth_rpcgss lockd sunrpc i915 video i2c_algo_bit drm_kms_helper drm i2c_core [last unloaded: scsi_wait_scan]
[ 1013.401848]
[ 1013.407399] Pid: 112, comm: kworker/2:1 Not tainted 3.4.0+ #30
[ 1013.437943] RIP: 8eb8:[<ffff88014630a000>]  [<ffff88014630a000>] 0xffff880146309fff
[ 1013.459871] RSP: ffffffff8165e919:ffff88014780f408  EFLAGS: 00010046
[ 1013.477909] RAX: 0000000000000001 RBX: ffffffff81104020 RCX: 0000000000000000
[ 1013.499458] RDX: ffff880148008ea8 RSI: ffffffff8131ef40 RDI: ffffffff82203b20
[ 1013.521612] RBP: ffffffff81005751 R08: 0000000000000000 R09: 0000000000000000
[ 1013.543121] R10: ffffffff82cdc318 R11: 0000000000000000 R12: ffff880145cc0000
[ 1013.564614] R13: ffff880148008eb8 R14: 0000000000000002 R15: ffff88014780cb40
[ 1013.586108] FS:  0000000000000000(0000) GS:ffff880148000000(0000) knlGS:0000000000000000
[ 1013.609458] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1013.627420] CR2: 0000000000000002 CR3: 0000000141f10000 CR4: 00000000001407e0
[ 1013.649051] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1013.670724] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1013.692376] Process kworker/2:1 (pid: 112, threadinfo ffff88013fe0e000, task ffff88014020a6a0)
[ 1013.717028] Stack:
[ 1013.724131]  ffff88014780f570 ffff880145cc0000 0000400000004000 0000000000000000
[ 1013.745918]  cccccccccccccccc ffff88014780cca8 ffffffff811072bb ffffffff81651627
[ 1013.767870]  ffffffff8118f8a7 ffffffff811072bb ffffffff81f2b6c5 ffffffff81f11bdb
[ 1013.790021] Call Trace:
[ 1013.800701] Code: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a <e7> d7 64 81 ff ff ff ff 01 00 00 00 00 00 00 00 65 d9 64 81 ff
[ 1013.861443] RIP  [<ffff88014630a000>] 0xffff880146309fff
[ 1013.884466]  RSP <ffff88014780f408>
[ 1013.901507] CR2: 0000000000000002

The solution was to reuse the NMI functions that change the IDT table to make the debug stack keep its current stack (in kernel mode) when hitting a breakpoint:

call debug_stack_set_zero
TRACE_IRQS_ON
call debug_stack_reset

If the TRACE_IRQS_ON happens to hit a breakpoint then it will keep the current stack and not crash the box.

---
 arch/x86/kernel/entry_64.S |   44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

index 320852d..7d65133 100644
@@ -191,6 +191,44 @@ ENDPROC(native_usergs_sysret64)
 .endm
 
 /*
+ * When dynamic function tracer is enabled it will add a breakpoint
+ * to all locations that it is about to modify, sync CPUs, update
+ * all the code, sync CPUs, then remove the breakpoints. In this time
+ * if lockdep is enabled, it might jump back into the debug handler
+ * outside the updating of the IST protection. (TRACE_IRQS_ON/OFF).
+ *
+ * We need to change the IDT table before calling TRACE_IRQS_ON/OFF to
+ * make sure the stack pointer does not get reset back to the top
+ * of the debug stack, and instead just reuses the current stack.
+ */
+#if defined(CONFIG_DYNAMIC_FTRACE) && defined(CONFIG_TRACE_IRQFLAGS)
+
+.macro TRACE_IRQS_OFF_DEBUG
+	call debug_stack_set_zero
+	TRACE_IRQS_OFF
+	call debug_stack_reset
+.endm
+
+.macro TRACE_IRQS_ON_DEBUG
+	call debug_stack_set_zero
+	TRACE_IRQS_ON
+	call debug_stack_reset
+.endm
+
+.macro TRACE_IRQS_IRETQ_DEBUG offset=ARGOFFSET
+	bt   $9,EFLAGS-\offset(%rsp)	/* interrupts off? */
+	jnc  1f
+	TRACE_IRQS_ON_DEBUG
+1:
+.endm
+
+#else
+# define TRACE_IRQS_OFF_DEBUG		TRACE_IRQS_OFF
+# define TRACE_IRQS_ON_DEBUG		TRACE_IRQS_ON
+# define TRACE_IRQS_IRETQ_DEBUG		TRACE_IRQS_IRETQ
+#endif
+
+/*
  * C code is not supposed to know about undefined top of stack. Every time
  * a C function with an pt_regs argument is called from the SYSCALL based
  * fast path FIXUP_TOP_OF_STACK is needed.

@@ -1098,7 +1136,7 @@ ENTRY(\sym)
 	subq $ORIG_RAX-R15, %rsp
 	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
-	TRACE_IRQS_OFF
+	TRACE_IRQS_OFF_DEBUG
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
 	subq $EXCEPTION_STKSZ, INIT_TSS_IST(\ist)

@@ -1393,7 +1431,7 @@ paranoidzeroentry machine_check *machine_check_vector(%rip)
 ENTRY(paranoid_exit)
 	DEFAULT_FRAME
 	DISABLE_INTERRUPTS(CLBR_NONE)
-	TRACE_IRQS_OFF
+	TRACE_IRQS_OFF_DEBUG
 	testl %ebx,%ebx				/* swapgs needed? */
 	jnz paranoid_restore
 	testl $3,CS(%rsp)

@@ -1404,7 +1442,7 @@ paranoid_swapgs:
 	RESTORE_ALL 8
 	jmp irq_return
 paranoid_restore:
-	TRACE_IRQS_IRETQ 0
+	TRACE_IRQS_IRETQ_DEBUG 0
 	RESTORE_ALL 8
 	jmp irq_return
 paranoid_userspace:

1.7.10

Oh, I forgot to add in my announcement subject, that this is for 3.5, as this bug came in during this merge window, with the exception of patch 3 which fixes a bug introduced in 3.3.

-- Steve