ftrace: clean ups and sanity checks

include/linux/init.h +4 -4

@@ -75,15 +75,15 @@
 
 
 #ifdef MODULE
-#define __exitused
+#define __exitused  notrace
 #else
-#define __exitused  __used
+#define __exitused  __used  notrace
 #endif
 
 #define __exit          __section(.exit.text) __exitused __cold
 
 /* Used for HOTPLUG */
-#define __devinit        __section(.devinit.text) __cold
+#define __devinit        __section(.devinit.text) __cold notrace
 #define __devinitdata    __section(.devinit.data)
 #define __devinitconst   __section(.devinit.rodata)
 #define __devexit        __section(.devexit.text) __exitused __cold

@@ -91,7 +91,7 @@
 #define __devexitconst   __section(.devexit.rodata)
 
 /* Used for HOTPLUG_CPU */
-#define __cpuinit        __section(.cpuinit.text) __cold
+#define __cpuinit        __section(.cpuinit.text) __cold notrace
 #define __cpuinitdata    __section(.cpuinit.data)
 #define __cpuinitconst   __section(.cpuinit.rodata)
 #define __cpuexit        __section(.cpuexit.text) __exitused __cold

include/linux/ftrace.h static inline void start_boot_trace(void · line 236

Quoted 2 lines · Steven Rostedt

include/linux/ftrace.h +2 -0

+enum {
+	FTRACE_CODE_MODIFIED,

i'd suggest to name it FTRACE_CODE_MODIFIED_OK here, to make it stand out from the failure codes.

include/linux/ftrace.h static inline void start_boot_trace(void · line 239

Quoted 3 lines

include/linux/ftrace.h +3 -0

+	FTRACE_CODE_FAILED_READ,
+	FTRACE_CODE_FAILED_CMP,
+	FTRACE_CODE_FAILED_WRITE,

but maybe we should just use the standard kernel return codes. 0 for success, -EINVAL for the rest. Is there any real value to know exactly why it failed? We just know the modification was fishy (this is an exception situation), and want to stop ftrace ASAP and then print a warning so a kernel developer can debug it.

Complicating error handling by introducing similar-looking return code names just makes it easier to mess up accidentally, hence it _reduces_ robustness.

Quoted 34 lines

include/linux/init.h +4 -4

@@ -75,15 +75,15 @@
 
 
 #ifdef MODULE
-#define __exitused
+#define __exitused  notrace
 #else
-#define __exitused  __used
+#define __exitused  __used  notrace
 #endif
 
 #define __exit          __section(.exit.text) __exitused __cold
 
 /* Used for HOTPLUG */
-#define __devinit        __section(.devinit.text) __cold
+#define __devinit        __section(.devinit.text) __cold notrace
 #define __devinitdata    __section(.devinit.data)
 #define __devinitconst   __section(.devinit.rodata)
 #define __devexit        __section(.devexit.text) __exitused __cold

@@ -91,7 +91,7 @@
 #define __devexitconst   __section(.devexit.rodata)
 
 /* Used for HOTPLUG_CPU */
-#define __cpuinit        __section(.cpuinit.text) __cold
+#define __cpuinit        __section(.cpuinit.text) __cold notrace
 #define __cpuinitdata    __section(.cpuinit.data)
 #define __cpuinitconst   __section(.cpuinit.rodata)
 #define __cpuexit        __section(.cpuexit.text) __exitused __cold

@@ -99,7 +99,7 @@
 #define __cpuexitconst   __section(.cpuexit.rodata)
 

include/linux/init.h line 105

Quoted 6 lines

include/linux/init.h +1 -1

 /* Used for MEMORY_HOTPLUG */
-#define __meminit        __section(.meminit.text) __cold
+#define __meminit        __section(.meminit.text) __cold notrace
 #define __meminitdata    __section(.meminit.data)
 #define __meminitconst   __section(.meminit.rodata)
 #define __memexit        __section(.memexit.text) __exitused __cold

there's no justification given for this in the changelog and the change looks fishy.

include/linux/init.h

Quoted 10 lines

static void ftrace_free_rec(struct dyn_ftrace *rec) {

include/linux/init.h +7 -0

+	/*
+	 * No locking, only called from kstop_machine, or
+	 * from module unloading with module locks and interrupts
+	 * disabled to prevent kstop machine from running.
+	 */
+
+	WARN_ON(rec->flags & FTRACE_FL_FREE);

this should _NOT_ be just a WARN_ON(). It should immediately stop ftrace entirely, then print _one_ warning. Then it should never ever run up to the next reboot.

this is a basic principle for instrumentation. If we detect a bug we disable ourselves immediately and print a _single_ warning.

Do _not_ print possibly thousands of warnings and continue as if nothing happened ...

include/linux/init.h

Quoted 3 lines

include/linux/init.h +2 -0

+					/* kprobes was not the fault */
+					ftrace_kill_atomic();

while at it, ftrace_kill_atomic() is a misnomer.

Please use something more understandable and less ambigious, like "ftrace_turn_off()". Both 'kill' and 'atomic' are heavily laden phrases used for many other things in the kernel.

And any such facility must work from any context, because we might call it from crash paths, etc. So dont name it _atomic() - it must obviously be atomic.

Quoted 9 lines

kernel/trace/ftrace.c static void ftrace_replace_code(int enab · line 571

Quoted 3 lines

kernel/trace/ftrace.c +2 -0

+					/* kprobes was not the fault */
+					ftrace_kill_atomic();

while at it, ftrace_kill_atomic() is a misnomer.

Please use something more understandable and less ambigious, like "ftrace_turn_off()". Both 'kill' and 'atomic' are heavily laden phrases used for many other things in the kernel.

Quoted 12 lines

i'd suggest to name it FTRACE_CODE_MODIFIED_OK here, to make it stand out from the failure codes.

include/linux/ftrace.h static inline void start_boot_trace(void · line 239

Quoted 3 lines

include/linux/ftrace.h +3 -0

+	FTRACE_CODE_FAILED_READ,
+	FTRACE_CODE_FAILED_CMP,
+	FTRACE_CODE_FAILED_WRITE,

but maybe we should just use the standard kernel return codes. 0 for success, -EINVAL for the rest. Is there any real value to know exactly why it failed? We just know the modification was fishy (this is an exception situation), and want to stop ftrace ASAP and then print a warning so a kernel developer can debug it.

Yes it is important to know the reason of failure, since it helps with diagnosing the issue.

Quoted 3 lines

Complicating error handling by introducing similar-looking return code names just makes it easier to mess up accidentally, hence it _reduces_ robustness.

I had in mind for 2.6.29 that I would let an arch add another non-error code that says, "FAIL NICELY". [...]

Quoted 9 lines

include/linux/init.h line 105

Quoted 6 lines

/* Used for MEMORY_HOTPLUG */

include/linux/init.h +1 -1

-#define __meminit        __section(.meminit.text) __cold
+#define __meminit        __section(.meminit.text) __cold notrace
 #define __meminitdata    __section(.meminit.data)
 #define __meminitconst   __section(.meminit.rodata)
 #define __memexit        __section(.memexit.text) __exitused __cold

there's no justification given for this in the changelog and the change looks fishy.

Sorry, I missed writing this. I had it in other patches, but forgot to add the change log here. These are areas, just like the __init section that I have no way ok finding out in an arch independent way, what to remove from the ftrace records. So by not adding these notraces, we are guaranteed to hit the warnings above!

Quoted 11 lines

Quoted 10 lines

Review

Quoted 7 lines

/* Used for MEMORY_HOTPLUG */

(quoted diff) +1 -1

-#define __meminit        __section(.meminit.text) __cold
+#define __meminit        __section(.meminit.text) __cold notrace
 #define __meminitdata    __section(.meminit.data)
 #define __meminitconst   __section(.meminit.rodata)
 #define __memexit        __section(.memexit.text) __exitused __cold

there's no justification given for this in the changelog and the change looks fishy.

Quoted 5 lines

Sorry, I missed writing this. I had it in other patches, but forgot to add the change log here. These are areas, just like the __init section that I have no way ok finding out in an arch independent way, what to remove from the ftrace records. So by not adding these notraces, we are guaranteed to hit the warnings above!

this is utterly fragile and might miss places that insert symbols into some of these sections manually.

the robust approach is to make sure these things are never in an ftrace record to begin with. scripts/recordmcount.pl should be taught to only record places that it is _100% sure of is traceable_. Not "everything and we'll sort out the stuff that we think is not okay".

if that needs arch dependent smarts then so be it - ftrace has to be enabled per arch anyway.

The x86 architecture uses a static recording of mcount caller locations and is not affected by this patch.

For architectures still using the dynamic ftrace daemon, this patch is critical. It removes the race between the recording of a function that calls mcount, the unloading of a module, and the ftrace daemon updating the call sites.

This patch adds the releasing of the hash functions that the daemon uses to update the mcount call sites. When a module is unloaded, not only are the replaced call site table update, but now so is the hash recorded functions that the ftrace daemon will use.

Again, architectures that implement MCOUNT_RECORD are not affected by this (which currently only x86 has).

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/ftrace.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Index: linux-compile.git/kernel/trace/ftrace.c
===================================================================

kernel/trace/ftrace.c +44 -0

@@ -164,10 +164,14 @@ static DEFINE_SPINLOCK(ftrace_hash_lock)
 #define ftrace_hash_lock(flags)	  spin_lock_irqsave(&ftrace_hash_lock, flags)
 #define ftrace_hash_unlock(flags) \
 			spin_unlock_irqrestore(&ftrace_hash_lock, flags)
+static void ftrace_release_hash(unsigned long start, unsigned long end);
 #else
 /* This is protected via the ftrace_lock with MCOUNT_RECORD. */
 #define ftrace_hash_lock(flags)   do { (void)(flags); } while (0)
 #define ftrace_hash_unlock(flags) do { } while(0)
+static inline void ftrace_release_hash(unsigned long start, unsigned long end)
+{
+}
 #endif
 
 /*

@@ -354,6 +358,8 @@ void ftrace_release(void *start, unsigne
 		}
 	}
 	spin_unlock(&ftrace_lock);
+
+	ftrace_release_hash(s, e);
 }
 
 static struct dyn_ftrace *ftrace_alloc_dyn_node(unsigned long ip)

@@ -1686,6 +1692,44 @@ void __init ftrace_init(void)
 	ftrace_disabled = 1;
 }
 #else /* CONFIG_FTRACE_MCOUNT_RECORD */
+
+static void ftrace_release_hash(unsigned long start, unsigned long end)
+{
+	struct dyn_ftrace *rec;
+	struct hlist_node *t, *n;
+	struct hlist_head *head, temp_list;
+	unsigned long flags;
+	int i, cpu;
+
+	preempt_disable_notrace();
+
+	/* disable incase we call something that calls mcount */
+	cpu = raw_smp_processor_id();
+	per_cpu(ftrace_shutdown_disable_cpu, cpu)++;
+
+	ftrace_hash_lock(flags);
+
+	for (i = 0; i < FTRACE_HASHSIZE; i++) {
+		INIT_HLIST_HEAD(&temp_list);
+		head = &ftrace_hash[i];
+
+		/* all CPUS are stopped, we are safe to modify code */
+		hlist_for_each_entry_safe(rec, t, n, head, node) {
+			if (rec->flags & FTRACE_FL_FREE)
+				continue;
+
+			if ((rec->ip >= start) && (rec->ip < end))
+				ftrace_free_rec(rec);
+		}
+	}
+
+	ftrace_hash_unlock(flags);
+
+	per_cpu(ftrace_shutdown_disable_cpu, cpu)--;
+	preempt_enable_notrace();
+
+}
+
 static int ftraced(void *ignore)
 {
 	unsigned long usecs;

Signature · 1 line

--