Skip to contentSkip to conversation

kprobes: fix build break in 2.6.15-rc5-mm3

PATCH In review Ananth N Mavinakayanahalli 2 participants 3 messages Dec 20, 2005 – Jan 4, 2006

Subsystems (2)
PowerPC kprobes
lore
Share & tools
lore

Ananth N Mavinakayanahalli

author
#0 lore raw

PATCH 1/1 kprobes: fix build break in 2.6.15-rc5-mm3

From: Ananth N Mavinkayanahalli <[email protected]>

The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build break due to changes introduced in the kprobe locking in 2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding of kprobe_mutex.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>
Acked-by Anil S Keshavamurthy <[email protected]>

---

 arch/powerpc/kernel/kprobes.c |    6 +++---
 arch/x86_64/kernel/kprobes.c  |    6 +++---
 include/asm-i386/kprobes.h    |    2 +-
 include/asm-ia64/kprobes.h    |    2 +-
 include/asm-powerpc/kprobes.h |    3 ++-
 include/asm-sparc64/kprobes.h |    2 +-
 include/asm-x86_64/kprobes.h  |    3 ++-
 include/linux/kprobes.h       |    1 +
 kernel/kprobes.c              |    4 ++--
 9 files changed, 16 insertions(+), 13 deletions(-)
arch/powerpc/kernel/kprobes.c +3 -3
Index: linux-2.6.15-rc5/arch/powerpc/kernel/kprobes.c
===================================================================
@@ -80,11 +80,11 @@ void __kprobes arch_disarm_kprobe(struct
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
-void __kprobes arch_remove_kprobe(struct kprobe *p, struct semaphore *s)
+void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	down(s);
+	down(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn);
-	up(s);
+	up(&kprobe_mutex);
 }
 
 static inline void prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
arch/x86_64/kernel/kprobes.c +3 -3
Index: linux-2.6.15-rc5/arch/x86_64/kernel/kprobes.c
===================================================================
@@ -220,11 +220,11 @@ void __kprobes arch_disarm_kprobe(struct
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
-void __kprobes arch_remove_kprobe(struct kprobe *p, struct semaphore *s)
+void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	down(s);
+	down(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn);
-	up(s);
+	up(&kprobe_mutex);
 }
 
 static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
include/linux/kprobes.h +1 -0
Index: linux-2.6.15-rc5/include/linux/kprobes.h
===================================================================
@@ -149,6 +149,7 @@ struct kretprobe_instance {
 };
 
 extern spinlock_t kretprobe_lock;
+extern struct semaphore kprobe_mutex;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);
 extern void arch_disarm_kprobe(struct kprobe *p);
kernel/kprobes.c +2 -2
Index: linux-2.6.15-rc5/kernel/kprobes.c
===================================================================
@@ -48,7 +48,7 @@
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
-static DECLARE_MUTEX(kprobe_mutex);	/* Protects kprobe_table */
+DECLARE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
 DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 

@@ -532,7 +532,7 @@ valid_p:
 			list_del_rcu(&p->list);
 			kfree(old_p);
 		}
-		arch_remove_kprobe(p, &kprobe_mutex);
+		arch_remove_kprobe(p);
 	}
 }
include/asm-powerpc/kprobes.h +2 -1
Index: linux-2.6.15-rc5/include/asm-powerpc/kprobes.h
===================================================================
@@ -32,6 +32,7 @@
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
 
 struct pt_regs;
+struct kprobe;
 
 typedef unsigned int kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION	0x7fe00008	/* trap */

@@ -49,7 +50,7 @@ typedef unsigned int kprobe_opcode_t;
 
 #define ARCH_SUPPORTS_KRETPROBES
 void kretprobe_trampoline(void);
-extern void arch_remove_kprobe(struct kprobe *p, struct semaphore *s);
+extern void arch_remove_kprobe(struct kprobe *p);
 
 /* Architecture specific copy of original instruction */
 struct arch_specific_insn {
include/asm-x86_64/kprobes.h +2 -1
Index: linux-2.6.15-rc5/include/asm-x86_64/kprobes.h
===================================================================
@@ -30,6 +30,7 @@
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
 
 struct pt_regs;
+struct kprobe;
 
 typedef u8 kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION	0xcc

@@ -44,6 +45,7 @@ typedef u8 kprobe_opcode_t;
 #define ARCH_SUPPORTS_KRETPROBES
 
 void kretprobe_trampoline(void);
+extern void arch_remove_kprobe(struct kprobe *p);
 
 /* Architecture specific copy of original instruction*/
 struct arch_specific_insn {

@@ -78,7 +80,6 @@ static inline void restore_interrupts(st
 		local_irq_enable();
 }
 
-extern void arch_remove_kprobe(struct kprobe *p, struct semaphore *s);
 extern int post_kprobe_handler(struct pt_regs *regs);
 extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
 extern int kprobe_handler(struct pt_regs *regs);
include/asm-i386/kprobes.h +1 -1
Index: linux-2.6.15-rc5/include/asm-i386/kprobes.h
===================================================================
@@ -40,7 +40,7 @@ typedef u8 kprobe_opcode_t;
 
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define ARCH_SUPPORTS_KRETPROBES
-#define arch_remove_kprobe(p, s)	do { } while(0)
+#define arch_remove_kprobe(p)	do {} while (0)
 
 void kretprobe_trampoline(void);
include/asm-ia64/kprobes.h +1 -1
Index: linux-2.6.15-rc5/include/asm-ia64/kprobes.h
===================================================================
@@ -89,7 +89,7 @@ struct kprobe_ctlblk {
 #define IP_RELATIVE_PREDICT_OPCODE	(7)
 #define LONG_BRANCH_OPCODE		(0xC)
 #define LONG_CALL_OPCODE		(0xD)
-#define arch_remove_kprobe(p, s)	do { } while(0)
+#define arch_remove_kprobe(p)		do {} while (0)
 
 typedef struct kprobe_opcode {
 	bundle_t bundle;
include/asm-sparc64/kprobes.h +1 -1
Index: linux-2.6.15-rc5/include/asm-sparc64/kprobes.h
===================================================================
@@ -12,7 +12,7 @@ typedef u32 kprobe_opcode_t;
 #define MAX_INSN_SIZE 2
 
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
-#define arch_remove_kprobe(p, s)	do { } while(0)
+#define arch_remove_kprobe(p)	do {} while (0)
 
 /* Architecture specific copy of original instruction*/
 struct arch_specific_insn {
#1 lore raw
Quoted 5 lines · Ananth N Mavinakayanahalli

The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build
break due to changes introduced in the kprobe locking in 2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding of kprobe_mutex.

Complaints:

a) Your changelog failed to describe what the build breakage was. It helps.

b) the changelog fails to describe _why_ we've reverted the locking

c) The patch does multiple things.

See, what I would _like_ to do is to fold the fixes in this patch into the patches which are already in -mm. That way, the patches which hit Linus's tree will be neater and won't introduce build breakage at any point.

And they won't add stuff and then immediately take it away again. That's for git losers ;)

But the patch which you've sent doesn't have a hope of applying anywhere except at the end of the patches which I already have.

The net result is that we'll hit Linus's tree with a bunch of patches, and then a followup patch which fixes those patches. Which is a dumb way in which to present the permanent kernel record, given that we have an opportunity to get it right first time, no?

Here's the bottom line: please never ever ever ever ever ever do more than one thing in a single patch. Ever. Did I mention "ever"? There are soooo many reasons for this....

author
#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 5 lines · Ananth N Mavinakayanahalli

The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build
break due to changes introduced in the kprobe locking in 2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding of kprobe_mutex.

Andrew,

Quoted 1 line

Complaints:

Sorry for the goofups..

Quoted 1 line

a) Your changelog failed to describe what the build breakage was. It helps.

The arch_remove_kprobe() prototype declaration in include/asm-*/kprobes.h needs a forward declaration of "struct kprobe" due to the order in which the kprobe.h files (include/linux/kprobe.h and the arch specific ones) are included. Though other archs build fine, the powerpc compiler throws out an error:

include/asm/kprobes.h:53: warning: its scope is only this definition or declaration, which is probably not what you want arch/powerpc/kernel/kprobes.c:84: error: conflicting types for `arch_remove_kprobe'
include/asm/kprobes.h:53: error: previous declaration of `arch_remove_kprobe'
make[1]: *** [arch/powerpc/kernel/kprobes.o] Error 1
make: *** [arch/powerpc/kernel] Error 2

Quoted 1 line

b) the changelog fails to describe _why_ we've reverted the locking

The patch that introduced kprobe_mutex instead of the spinlock had an undesirable portion that passed a struct semaphore * as a parameter. This obfuscates locking and will lead to hard to maintain code.

Quoted 11 lines

c) The patch does multiple things.

See, what I would _like_ to do is to fold the fixes in this patch into the patches which are already in -mm. That way, the patches which hit Linus's tree will be neater and won't introduce build breakage at any point.

And they won't add stuff and then immediately take it away again. That's for git losers ;)

But the patch which you've sent doesn't have a hope of applying anywhere except at the end of the patches which I already have.

My understanding was that the preferred method is incremental patches over the released -mm.

Quoted 4 lines

The net result is that we'll hit Linus's tree with a bunch of patches, and then a followup patch which fixes those patches. Which is a dumb way in which to present the permanent kernel record, given that we have an opportunity to get it right first time, no?

You are right. The patch in question fixes a build break and other issues in the kprobe spinlock to mutex patch. And it is indeed better to redo the patch with these fixes included. Does that sound reasonable?

Quoted 3 lines

Here's the bottom line: please never ever ever ever ever ever do more than one thing in a single patch. Ever. Did I mention "ever"? There are soooo many reasons for this....

Point taken :)

Ananth