[Fault-injection-developer] Re:[PATCH] 47-kp2-fi2 features

[Rusty L. <ru...@li...>]

> diff -Nur -X /home/louis/dontdiff 47-kp/Makefile 47-kp-fi/Makefile
> --- 47-kp/Makefile	Tue Nov 26 14:42:30 2002
> +++ 47-kp-fi/Makefile	Mon Nov 25 15:15:38 2002
> @@ -1,7 +1,7 @@
>  VERSION = 2
>  PATCHLEVEL = 5
>  SUBLEVEL = 47
> -EXTRAVERSION =
> +EXTRAVERSION =kp-fi

Why are you doing this?

>  
>  # *DOCUMENTATION*
>  # To see a list of typical targets execute "make help"
> diff -Nur -X /home/louis/dontdiff 47-kp/arch/i386/Kconfig 47-kp-fi/arch/i386/Kconfig
> --- 47-kp/arch/i386/Kconfig	Tue Nov 26 14:42:37 2002
> +++ 47-kp-fi/arch/i386/Kconfig	Mon Nov 25 15:15:40 2002
> @@ -1562,11 +1562,20 @@
>  
>  config DEBUGREG
>  	bool "Global Debug Registers"
> -	depend on DEBUG_KERNEL
> +	depends on DEBUG_KERNEL
> +	help

Hmm... fixing an error from your last patch?  How about fix the code
before making the first patch.  If your first patch only introduces
kprobes and kwatch points, then this patch should not.

>  
>  config KWATCH
>  	bool "Kwatch points"
> -	depend on DEBUGREG
> +	depends on DEBUGREG
> +	help
> +

Same comment as my last

> +config FI
> +	bool "Fault Injection"
> +	depends on KPROBES
> +	help
> +	  Say Y here if you want to enable Fault Injection (FI) mechanism. 
> +	  The FI can monitor MMIO/IO access in kernel.

If you want the choices to be module or nothing, then just code it that way.

Also please add a more descriptive comment for this component.  Keep 
in mind that the world will be stumbling across this option while
trying to configure a kernel, and would like to know something more then
the name of the module.

>  
>  config DEBUG_STACKOVERFLOW
>  	bool "Check for stack overflows"
> diff -Nur -X /home/louis/dontdiff 47-kp/arch/i386/kernel/i386_ksyms.c 47-kp-fi/arch/i386/kernel/i386_ksyms.c
> --- 47-kp/arch/i386/kernel/i386_ksyms.c	Tue Nov 26 14:42:37 2002
> +++ 47-kp-fi/arch/i386/kernel/i386_ksyms.c	Mon Nov 25 15:15:40 2002
> @@ -32,6 +32,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/nmi.h>
>  #include <asm/edd.h>
> +#include <asm/fi.h>
>  
>  extern void dump_thread(struct pt_regs *, struct user *);
>  extern spinlock_t rtc_lock;
> @@ -216,3 +217,9 @@
>  EXPORT_SYMBOL(edd);
>  EXPORT_SYMBOL(eddnr);
>  #endif
> +
> +#ifdef CONFIG_FI
> +EXPORT_SYMBOL(irq_desc);

None of your other patches even use this symbol.  If you don't have a 
good argument for why this symbol should be exported then just don't 
do it.

If something comes up in the future then fight that fight then.

> +EXPORT_SYMBOL(fi_page_fault);
> +EXPORT_SYMBOL(fi_post_page_fault);
> +#endif
> diff -Nur -X /home/louis/dontdiff 47-kp/arch/i386/kernel/traps.c 47-kp-fi/arch/i386/kernel/traps.c
> --- 47-kp/arch/i386/kernel/traps.c	Tue Nov 26 14:42:37 2002
> +++ 47-kp-fi/arch/i386/kernel/traps.c	Mon Nov 25 15:15:41 2002
> @@ -47,6 +47,7 @@
>  #include <asm/smp.h>
>  #include <asm/pgalloc.h>
>  #include <asm/arch_hooks.h>
> +#include <asm/fi.h>
>  
>  #include <linux/irq.h>
>  #include <linux/module.h>
> @@ -564,6 +565,9 @@
>  	return 0;
>  }
>  
> +int (*fi_post_page_fault) (unsigned long condition,
> +		struct pt_regs *reg);
> +
>  /*
>   * Our handling of the processor debug registers is non-trivial.
>   * We do not clear them on entry and exit from the kernel. Therefore
> @@ -599,7 +603,10 @@
>  
>  	if (kwatch_handler(condition, regs))
>  		return 1;
> -
> +#ifdef CONFIG_FI
> +	if (fi_post_page_fault && fi_post_page_fault(condition, regs))
> +		return 1;
> +#endif

I don't think this is the right way of hooking into do_debug.
Change this to the model that kprobes follows.  You don't have to 
have ugly #if/#endif wrappers (see Documentation/SubmittingPatches)
in this file, and you don't have to rely on black magic with symbol 
names.


>  	/* Interrupts not disabled for normal trap handling. */
>  	restore_interrupts(regs);
>  
> diff -Nur -X /home/louis/dontdiff 47-kp/arch/i386/mm/fault.c 47-kp-fi/arch/i386/mm/fault.c
> --- 47-kp/arch/i386/mm/fault.c	Tue Nov 26 14:42:37 2002
> +++ 47-kp-fi/arch/i386/mm/fault.c	Mon Nov 25 15:15:41 2002
> @@ -26,6 +26,7 @@
>  #include <asm/pgalloc.h>
>  #include <asm/hardirq.h>
>  #include <asm/desc.h>
> +#include <asm/fi.h>
>  
>  extern void die(const char *,struct pt_regs *,long);
>  
> @@ -139,6 +140,7 @@
>  }
>  
>  asmlinkage void do_invalid_op(struct pt_regs *, unsigned long);
> +int (*fi_page_fault) ( struct pt_regs *regs, unsigned long address);
>  
>  /*
>   * This routine handles page faults.  It determines the address,
> @@ -166,7 +168,10 @@
>  
>  	if (kprobe_running() && kprobe_fault_handler(regs, 14))
>  		return;
> -
> +#ifdef CONFIG_FI
> +	if (fi_page_fault && fi_page_fault(regs, address))
> +		return;
> +#endif

Same comment about hooking into code that I said above.

>  	/* It's safe to allow irq's after cr2 has been saved */
>  	if (regs->eflags & X86_EFLAGS_IF)
>  		local_irq_enable();
> diff -Nur -X /home/louis/dontdiff 47-kp/include/asm-i386/fi.h 47-kp-fi/include/asm-i386/fi.h
> --- 47-kp/include/asm-i386/fi.h	Thu Jan  1 08:00:00 1970
> +++ 47-kp-fi/include/asm-i386/fi.h	Mon Nov 25 15:16:18 2002
> @@ -0,0 +1,8 @@
> +#ifndef _ASM_FI_H
> +#define _ASM_FI_H
> +#ifdef CONFIG_FI
> +extern int (*fi_page_fault) ( struct pt_regs *regs, unsigned long address);
> +extern int (*fi_post_page_fault) (unsigned long condition, 
> +		struct pt_regs *reg);
> +#endif
> +#endif /* _ASM_FI_H */
> diff -Nur -X /home/louis/dontdiff 47-kp/kernel/ksyms.c 47-kp-fi/kernel/ksyms.c
> --- 47-kp/kernel/ksyms.c	Tue Nov 26 14:43:31 2002
> +++ 47-kp-fi/kernel/ksyms.c	Mon Nov 25 15:16:27 2002
> @@ -601,3 +601,7 @@
>  
>  /* debug */
>  EXPORT_SYMBOL(dump_stack);
> +
> +#ifdef CONFIG_FI
> +EXPORT_SYMBOL(module_list);

I don't think the current in-kernel linking implementation uses
module_list anymore. At least not for i386.

> +#endif
>