| Message ID | 1562304629-29376-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | mm/kprobes: Add generic kprobe_fault_handler() fallback definition | expand |
Hi Anshuman, On Fri, 5 Jul 2019 11:00:29 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote: > Architectures like parisc enable CONFIG_KROBES without having a definition > for kprobe_fault_handler() which results in a build failure. Hmm, as far as I can see, kprobe_fault_handler() is closed inside each arch specific code. The reason why include/linux/kprobes.h defines dummy inline function is only for !CONFIG_KPROBES case. > Arch needs to > provide kprobe_fault_handler() as it is platform specific and cannot have > a generic working alternative. But in the event when platform lacks such a > definition there needs to be a fallback. Wait, indeed that each arch need to implement it, but that is for calling kprobe->fault_handler() as user expected. Hmm, why not fixing those architecture implementations? > This adds a stub kprobe_fault_handler() definition which not only prevents > a build failure but also makes sure that kprobe_page_fault() if called will > always return negative in absence of a sane platform specific alternative. I don't like introducing this complicated macro only for avoiding (not fixing) build error. To fix that, kprobes on parisc should implement kprobe_fault_handler correctly (and call kprobe->fault_handler). BTW, even if you need such generic stub, please use a weak function instead of macros for every arch headers. > While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud > definitions for generic kporbe_fault_handler() and kprobes_built_in() can > just be dropped. Only on x86 it needs to be added back locally as it gets > used in a !CONFIG_KPROBES function do_general_protection(). If you want to remove kprobes_built_in(), you should replace it with IS_ENABLED(CONFIG_KPROBES), instead of this... Thank you, > > Cc: Vineet Gupta <vgupta@synopsys.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Fenghua Yu <fenghua.yu@intel.com> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Paul Burton <paul.burton@mips.com> > Cc: James Hogan <jhogan@kernel.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Vasily Gorbik <gor@linux.ibm.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > Cc: Rich Felker <dalias@libc.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Allison Randal <allison@lohutok.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Enrico Weigelt <info@metux.net> > Cc: Richard Fontana <rfontana@redhat.com> > Cc: Kate Stewart <kstewart@linuxfoundation.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: x86@kernel.org > Cc: linux-snps-arc@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-ia64@vger.kernel.org > Cc: linux-mips@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-s390@vger.kernel.org > Cc: linux-sh@vger.kernel.org > Cc: sparclinux@vger.kernel.org > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arc/include/asm/kprobes.h | 1 + > arch/arm/include/asm/kprobes.h | 1 + > arch/arm64/include/asm/kprobes.h | 1 + > arch/ia64/include/asm/kprobes.h | 1 + > arch/mips/include/asm/kprobes.h | 1 + > arch/powerpc/include/asm/kprobes.h | 1 + > arch/s390/include/asm/kprobes.h | 1 + > arch/sh/include/asm/kprobes.h | 1 + > arch/sparc/include/asm/kprobes.h | 1 + > arch/x86/include/asm/kprobes.h | 6 ++++++ > include/linux/kprobes.h | 32 ++++++++++++++++++------------ > 11 files changed, 34 insertions(+), 13 deletions(-) > > diff --git a/arch/arc/include/asm/kprobes.h b/arch/arc/include/asm/kprobes.h > index 2134721dce44..ee8efe256675 100644 > --- a/arch/arc/include/asm/kprobes.h > +++ b/arch/arc/include/asm/kprobes.h > @@ -45,6 +45,7 @@ struct kprobe_ctlblk { > struct prev_kprobe prev_kprobe; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > int kprobe_fault_handler(struct pt_regs *regs, unsigned long cause); > void kretprobe_trampoline(void); > void trap_is_kprobe(unsigned long address, struct pt_regs *regs); > diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h > index 213607a1f45c..660f877b989f 100644 > --- a/arch/arm/include/asm/kprobes.h > +++ b/arch/arm/include/asm/kprobes.h > @@ -38,6 +38,7 @@ struct kprobe_ctlblk { > struct prev_kprobe prev_kprobe; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > void arch_remove_kprobe(struct kprobe *); > int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); > int kprobe_exceptions_notify(struct notifier_block *self, > diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h > index 97e511d645a2..667773f75616 100644 > --- a/arch/arm64/include/asm/kprobes.h > +++ b/arch/arm64/include/asm/kprobes.h > @@ -42,6 +42,7 @@ struct kprobe_ctlblk { > struct kprobe_step_ctx ss_ctx; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > void arch_remove_kprobe(struct kprobe *); > int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); > int kprobe_exceptions_notify(struct notifier_block *self, > diff --git a/arch/ia64/include/asm/kprobes.h b/arch/ia64/include/asm/kprobes.h > index c5cf5e4fb338..c321e8585089 100644 > --- a/arch/ia64/include/asm/kprobes.h > +++ b/arch/ia64/include/asm/kprobes.h > @@ -106,6 +106,7 @@ struct arch_specific_insn { > unsigned short slot; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > extern int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > diff --git a/arch/mips/include/asm/kprobes.h b/arch/mips/include/asm/kprobes.h > index 68b1e5d458cf..d1efe991ea22 100644 > --- a/arch/mips/include/asm/kprobes.h > +++ b/arch/mips/include/asm/kprobes.h > @@ -40,6 +40,7 @@ do { \ > > #define kretprobe_blacklist_size 0 > > +#define kprobe_fault_handler kprobe_fault_handler > void arch_remove_kprobe(struct kprobe *p); > int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h > index 66b3f2983b22..c94f375ec957 100644 > --- a/arch/powerpc/include/asm/kprobes.h > +++ b/arch/powerpc/include/asm/kprobes.h > @@ -84,6 +84,7 @@ struct arch_optimized_insn { > kprobe_opcode_t *insn; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > extern int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > diff --git a/arch/s390/include/asm/kprobes.h b/arch/s390/include/asm/kprobes.h > index b106aa29bf55..0ecaebb78092 100644 > --- a/arch/s390/include/asm/kprobes.h > +++ b/arch/s390/include/asm/kprobes.h > @@ -73,6 +73,7 @@ struct kprobe_ctlblk { > void arch_remove_kprobe(struct kprobe *p); > void kretprobe_trampoline(void); > > +#define kprobe_fault_handler kprobe_fault_handler > int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > diff --git a/arch/sh/include/asm/kprobes.h b/arch/sh/include/asm/kprobes.h > index 6171682f7798..637a698393c0 100644 > --- a/arch/sh/include/asm/kprobes.h > +++ b/arch/sh/include/asm/kprobes.h > @@ -45,6 +45,7 @@ struct kprobe_ctlblk { > struct prev_kprobe prev_kprobe; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > extern int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > diff --git a/arch/sparc/include/asm/kprobes.h b/arch/sparc/include/asm/kprobes.h > index bfcaa6326c20..9aa4d25a45a8 100644 > --- a/arch/sparc/include/asm/kprobes.h > +++ b/arch/sparc/include/asm/kprobes.h > @@ -47,6 +47,7 @@ struct kprobe_ctlblk { > struct prev_kprobe prev_kprobe; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h > index 5dc909d9ad81..1af2b6db13bd 100644 > --- a/arch/x86/include/asm/kprobes.h > +++ b/arch/x86/include/asm/kprobes.h > @@ -101,11 +101,17 @@ struct kprobe_ctlblk { > struct prev_kprobe prev_kprobe; > }; > > +#define kprobe_fault_handler kprobe_fault_handler > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > extern int kprobe_exceptions_notify(struct notifier_block *self, > unsigned long val, void *data); > extern int kprobe_int3_handler(struct pt_regs *regs); > extern int kprobe_debug_handler(struct pt_regs *regs); > +#else > +static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) > +{ > + return 0; > +} > > #endif /* CONFIG_KPROBES */ > #endif /* _ASM_X86_KPROBES_H */ > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 04bdaf01112c..e106f3018804 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -182,11 +182,19 @@ DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); > /* > * For #ifdef avoidance: > */ > -static inline int kprobes_built_in(void) > + > +/* > + * Architectures need to override this with their own implementation > + * if they care to call kprobe_page_fault(). This will just ensure > + * that kprobe_page_fault() returns false when called without having > + * a proper platform specific definition for kprobe_fault_handler(). > + */ > +#ifndef kprobe_fault_handler > +static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) > { > - return 1; > + return 0; > } > - > +#endif > #ifdef CONFIG_KRETPROBES > extern void arch_prepare_kretprobe(struct kretprobe_instance *ri, > struct pt_regs *regs); > @@ -375,14 +383,6 @@ void free_insn_page(void *page); > > #else /* !CONFIG_KPROBES: */ > > -static inline int kprobes_built_in(void) > -{ > - return 0; > -} > -static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) > -{ > - return 0; > -} > static inline struct kprobe *get_kprobe(void *addr) > { > return NULL; > @@ -458,12 +458,11 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr) > } > #endif > > +#ifdef CONFIG_KPROBES > /* Returns true if kprobes handled the fault */ > static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, > unsigned int trap) > { > - if (!kprobes_built_in()) > - return false; > if (user_mode(regs)) > return false; > /* > @@ -476,5 +475,12 @@ static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, > return false; > return kprobe_fault_handler(regs, trap); > } > +#else > +static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, > + unsigned int trap) > +{ > + return false; > +} > +#endif > > #endif /* _LINUX_KPROBES_H */ > -- > 2.20.1 >
On 07/05/2019 04:00 PM, Masami Hiramatsu wrote: > Hi Anshuman, Hello Masami, > > On Fri, 5 Jul 2019 11:00:29 +0530 > Anshuman Khandual <anshuman.khandual@arm.com> wrote: > >> Architectures like parisc enable CONFIG_KROBES without having a definition >> for kprobe_fault_handler() which results in a build failure. > > Hmm, as far as I can see, kprobe_fault_handler() is closed inside each arch > specific code. The reason why include/linux/kprobes.h defines > dummy inline function is only for !CONFIG_KPROBES case. IIRC Andrew mentioned [1] that we should remove this stub from the generic kprobes header because this is very much architecture specific. As we see in this proposed patch, except x86 there is no other current user which actually calls this from some where when CONFIG_KPROBES is not enabled. [1] https://www.spinics.net/lists/linux-mm/msg182649.html > >> Arch needs to >> provide kprobe_fault_handler() as it is platform specific and cannot have >> a generic working alternative. But in the event when platform lacks such a >> definition there needs to be a fallback. > > Wait, indeed that each arch need to implement it, but that is for calling > kprobe->fault_handler() as user expected. > Hmm, why not fixing those architecture implementations? After the recent change which introduced a generic kprobe_page_fault() every architecture enabling CONFIG_KPROBES must have a kprobe_fault_handler() which was not the case earlier. Architectures like parisc which does enable KPROBES but never used (kprobe_page_fault or kprobe->fault_handler) kprobe_fault_handler() now needs one as well. I am not sure and will probably require inputs from arch parsic folks whether it at all needs one. We dont have a stub or fallback definition for kprobe_fault_handler() when CONFIG_KPROBES is enabled just to prevent a build failure in such cases. In such a situation it might be better defining a stub symbol fallback than to try to implement one definition which the architecture previously never needed or used. AFAICS there is no generic MM callers for kprobe_fault_handler() as well which would have made it mandatory for parisc to define a real one. > >> This adds a stub kprobe_fault_handler() definition which not only prevents >> a build failure but also makes sure that kprobe_page_fault() if called will >> always return negative in absence of a sane platform specific alternative. > > I don't like introducing this complicated macro only for avoiding (not fixing) > build error. To fix that, kprobes on parisc should implement kprobe_fault_handler > correctly (and call kprobe->fault_handler). As I mentioned before parsic might not need a real one. But you are right this complicated (if perceived as such) change can be just avoided at least for the build failure problem by just defining a stub definition kprobe_fault_handler() for arch parsic when CONFIG_KPROBES is enabled. But this patch does some more and solves the kprobe_fault_handler() symbol dependency in a more generic way and forces kprobe_page_fault() to fail in absence a real arch kprobe_fault_handler(). Is not it worth solving in this way ? > > BTW, even if you need such generic stub, please use a weak function instead > of macros for every arch headers. There is a bit problem with that. The existing definitions are with different signatures and an weak function will need them to be exact same for override requiring more code changes. Hence choose to go with a macro in each header. arch/arc/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned long cause); arch/arm/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); arch/arm64/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); arch/ia64/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); arch/powerpc/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); arch/s390/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, int trapnr); arch/sh/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); arch/sparc/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, int trapnr); arch/x86/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > >> While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud >> definitions for generic kporbe_fault_handler() and kprobes_built_in() can >> just be dropped. Only on x86 it needs to be added back locally as it gets >> used in a !CONFIG_KPROBES function do_general_protection(). > > If you want to remove kprobes_built_in(), you should replace it with > IS_ENABLED(CONFIG_KPROBES), instead of this... Apart from kprobes_built_in() the intent was to remove !CONFIG_KPROBES stub for kprobe_fault_handler() as well which required making generic kprobe_page_fault() to be empty in such case.
On 07/05/2019 11:00 AM, Anshuman Khandual wrote: > Architectures like parisc enable CONFIG_KROBES without having a definition > for kprobe_fault_handler() which results in a build failure. Arch needs to > provide kprobe_fault_handler() as it is platform specific and cannot have > a generic working alternative. But in the event when platform lacks such a > definition there needs to be a fallback. > > This adds a stub kprobe_fault_handler() definition which not only prevents > a build failure but also makes sure that kprobe_page_fault() if called will > always return negative in absence of a sane platform specific alternative. > > While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud > definitions for generic kporbe_fault_handler() and kprobes_built_in() can > just be dropped. Only on x86 it needs to be added back locally as it gets > used in a !CONFIG_KPROBES function do_general_protection(). > > Cc: Vineet Gupta <vgupta@synopsys.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Fenghua Yu <fenghua.yu@intel.com> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Paul Burton <paul.burton@mips.com> > Cc: James Hogan <jhogan@kernel.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Vasily Gorbik <gor@linux.ibm.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > Cc: Rich Felker <dalias@libc.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> > Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Allison Randal <allison@lohutok.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Enrico Weigelt <info@metux.net> > Cc: Richard Fontana <rfontana@redhat.com> > Cc: Kate Stewart <kstewart@linuxfoundation.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: x86@kernel.org > Cc: linux-snps-arc@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-ia64@vger.kernel.org > Cc: linux-mips@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-s390@vger.kernel.org > Cc: linux-sh@vger.kernel.org > Cc: sparclinux@vger.kernel.org > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- Any updates or suggestions on this patch ? Currently there is a build failure on parisc architecture due to the lack of a kprobe_fault_handler() definition when CONFIG_KPROBES is enabled and this build failure needs to be fixed. This patch solves the build problem. But otherwise I am also happy to just define a stub definition for kprobe_fault_handler() on parisc arch when CONFIG_KPROBES is enabled, which will avoid the build failure. Please suggest.
Hi Anshuman, On Mon, 8 Jul 2019 09:03:13 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote: > >> Architectures like parisc enable CONFIG_KROBES without having a definition > >> for kprobe_fault_handler() which results in a build failure. > > > > Hmm, as far as I can see, kprobe_fault_handler() is closed inside each arch > > specific code. The reason why include/linux/kprobes.h defines > > dummy inline function is only for !CONFIG_KPROBES case. > > IIRC Andrew mentioned [1] that we should remove this stub from the generic kprobes > header because this is very much architecture specific. As we see in this proposed > patch, except x86 there is no other current user which actually calls this from > some where when CONFIG_KPROBES is not enabled. > > [1] https://www.spinics.net/lists/linux-mm/msg182649.html Ah, OK. I saw another branch. Also, this is a bugfix patch against commit 4dd635bce90e ("mm, kprobes: generalize and rename notify_page_fault() as kprobe_page_fault()"), please add Fixes: tag on it. In this case, we should just add a prototype of kprobe_fault_handler() in include/linux/kprobes.h, and maybe add a stub of kprobe_fault_handler() as a weak function, something like below. int __weak kprobe_fault_handler(struct pt_regs *regs, int trapnr) { /* * Each architecture which uses kprobe_page_fault() must define * a fault handler to handle page fault in kprobe correctly. */ WARN_ON_ONCE(1); return 0; } > >> Arch needs to > >> provide kprobe_fault_handler() as it is platform specific and cannot have > >> a generic working alternative. But in the event when platform lacks such a > >> definition there needs to be a fallback. > > > > Wait, indeed that each arch need to implement it, but that is for calling > > kprobe->fault_handler() as user expected. > > Hmm, why not fixing those architecture implementations? > > After the recent change which introduced a generic kprobe_page_fault() every > architecture enabling CONFIG_KPROBES must have a kprobe_fault_handler() which > was not the case earlier. As far as I can see, gcc complains it because there is no prototype of kprobe_fault_handler(). Actually no need to define empty kprobe_fault_handler() on each arch. If we have a prototype, but no actual function, gcc stops the error unless the arch depending code uses it. So actually, we don't need above __weak function. > Architectures like parisc which does enable KPROBES but > never used (kprobe_page_fault or kprobe->fault_handler) kprobe_fault_handler() now > needs one as well. (Hmm, it sounds like the kprobes porting is incomplete on parisc...) > I am not sure and will probably require inputs from arch parsic > folks whether it at all needs one. We dont have a stub or fallback definition for > kprobe_fault_handler() when CONFIG_KPROBES is enabled just to prevent a build > failure in such cases. Yeah, that is a bug, and fixed by adding a prototype, not introducing new macro. > > In such a situation it might be better defining a stub symbol fallback than to try > to implement one definition which the architecture previously never needed or used. > AFAICS there is no generic MM callers for kprobe_fault_handler() as well which would > have made it mandatory for parisc to define a real one. > > > > >> This adds a stub kprobe_fault_handler() definition which not only prevents > >> a build failure but also makes sure that kprobe_page_fault() if called will > >> always return negative in absence of a sane platform specific alternative. > > > > I don't like introducing this complicated macro only for avoiding (not fixing) > > build error. To fix that, kprobes on parisc should implement kprobe_fault_handler > > correctly (and call kprobe->fault_handler). > > As I mentioned before parsic might not need a real one. But you are right this > complicated (if perceived as such) change can be just avoided at least for the > build failure problem by just defining a stub definition kprobe_fault_handler() > for arch parsic when CONFIG_KPROBES is enabled. But this patch does some more > and solves the kprobe_fault_handler() symbol dependency in a more generic way and > forces kprobe_page_fault() to fail in absence a real arch kprobe_fault_handler(). > Is not it worth solving in this way ? > > > > > BTW, even if you need such generic stub, please use a weak function instead > > of macros for every arch headers. > > There is a bit problem with that. The existing definitions are with different > signatures and an weak function will need them to be exact same for override > requiring more code changes. Hence choose to go with a macro in each header. > > arch/arc/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned long cause); > arch/arm/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); > arch/arm64/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); > arch/ia64/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > arch/powerpc/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > arch/s390/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > arch/sh/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > arch/sparc/include/asm/kprobes.h:int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > arch/x86/include/asm/kprobes.h:extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); OK, in that case, original commit is wrong way. it should be reverted and should introduce something like below /* Returns true if arch should call kprobes_fault_handler() */ static nokprobe_inline bool is_kprobe_page_fault(struct pt_regs *regs) { if (!kprobes_built_in()) return false; if (user_mode(regs)) return false; /* * To be potentially processing a kprobe fault and to be allowed * to call kprobe_running(), we have to be non-preemptible. */ if (preemptible()) return false; if (!kprobe_running()) return false; return true; } Since it silently casts the type of trapnr, which is strongly depends on architecture. > >> While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud > >> definitions for generic kporbe_fault_handler() and kprobes_built_in() can > >> just be dropped. Only on x86 it needs to be added back locally as it gets > >> used in a !CONFIG_KPROBES function do_general_protection(). > > > > If you want to remove kprobes_built_in(), you should replace it with > > IS_ENABLED(CONFIG_KPROBES), instead of this... > > Apart from kprobes_built_in() the intent was to remove !CONFIG_KPROBES > stub for kprobe_fault_handler() as well which required making generic > kprobe_page_fault() to be empty in such case. No, I meant that "IS_ENABLED(CONFIG_KPROBES)" is generic and is equal to what kprobes_built_in() does. Thank you,
diff --git a/arch/arc/include/asm/kprobes.h b/arch/arc/include/asm/kprobes.h index 2134721dce44..ee8efe256675 100644 --- a/arch/arc/include/asm/kprobes.h +++ b/arch/arc/include/asm/kprobes.h @@ -45,6 +45,7 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; }; +#define kprobe_fault_handler kprobe_fault_handler int kprobe_fault_handler(struct pt_regs *regs, unsigned long cause); void kretprobe_trampoline(void); void trap_is_kprobe(unsigned long address, struct pt_regs *regs); diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h index 213607a1f45c..660f877b989f 100644 --- a/arch/arm/include/asm/kprobes.h +++ b/arch/arm/include/asm/kprobes.h @@ -38,6 +38,7 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; }; +#define kprobe_fault_handler kprobe_fault_handler void arch_remove_kprobe(struct kprobe *); int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); int kprobe_exceptions_notify(struct notifier_block *self, diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h index 97e511d645a2..667773f75616 100644 --- a/arch/arm64/include/asm/kprobes.h +++ b/arch/arm64/include/asm/kprobes.h @@ -42,6 +42,7 @@ struct kprobe_ctlblk { struct kprobe_step_ctx ss_ctx; }; +#define kprobe_fault_handler kprobe_fault_handler void arch_remove_kprobe(struct kprobe *); int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr); int kprobe_exceptions_notify(struct notifier_block *self, diff --git a/arch/ia64/include/asm/kprobes.h b/arch/ia64/include/asm/kprobes.h index c5cf5e4fb338..c321e8585089 100644 --- a/arch/ia64/include/asm/kprobes.h +++ b/arch/ia64/include/asm/kprobes.h @@ -106,6 +106,7 @@ struct arch_specific_insn { unsigned short slot; }; +#define kprobe_fault_handler kprobe_fault_handler extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); extern int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); diff --git a/arch/mips/include/asm/kprobes.h b/arch/mips/include/asm/kprobes.h index 68b1e5d458cf..d1efe991ea22 100644 --- a/arch/mips/include/asm/kprobes.h +++ b/arch/mips/include/asm/kprobes.h @@ -40,6 +40,7 @@ do { \ #define kretprobe_blacklist_size 0 +#define kprobe_fault_handler kprobe_fault_handler void arch_remove_kprobe(struct kprobe *p); int kprobe_fault_handler(struct pt_regs *regs, int trapnr); diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h index 66b3f2983b22..c94f375ec957 100644 --- a/arch/powerpc/include/asm/kprobes.h +++ b/arch/powerpc/include/asm/kprobes.h @@ -84,6 +84,7 @@ struct arch_optimized_insn { kprobe_opcode_t *insn; }; +#define kprobe_fault_handler kprobe_fault_handler extern int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); diff --git a/arch/s390/include/asm/kprobes.h b/arch/s390/include/asm/kprobes.h index b106aa29bf55..0ecaebb78092 100644 --- a/arch/s390/include/asm/kprobes.h +++ b/arch/s390/include/asm/kprobes.h @@ -73,6 +73,7 @@ struct kprobe_ctlblk { void arch_remove_kprobe(struct kprobe *p); void kretprobe_trampoline(void); +#define kprobe_fault_handler kprobe_fault_handler int kprobe_fault_handler(struct pt_regs *regs, int trapnr); int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); diff --git a/arch/sh/include/asm/kprobes.h b/arch/sh/include/asm/kprobes.h index 6171682f7798..637a698393c0 100644 --- a/arch/sh/include/asm/kprobes.h +++ b/arch/sh/include/asm/kprobes.h @@ -45,6 +45,7 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; }; +#define kprobe_fault_handler kprobe_fault_handler extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); extern int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); diff --git a/arch/sparc/include/asm/kprobes.h b/arch/sparc/include/asm/kprobes.h index bfcaa6326c20..9aa4d25a45a8 100644 --- a/arch/sparc/include/asm/kprobes.h +++ b/arch/sparc/include/asm/kprobes.h @@ -47,6 +47,7 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; }; +#define kprobe_fault_handler kprobe_fault_handler int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); int kprobe_fault_handler(struct pt_regs *regs, int trapnr); diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h index 5dc909d9ad81..1af2b6db13bd 100644 --- a/arch/x86/include/asm/kprobes.h +++ b/arch/x86/include/asm/kprobes.h @@ -101,11 +101,17 @@ struct kprobe_ctlblk { struct prev_kprobe prev_kprobe; }; +#define kprobe_fault_handler kprobe_fault_handler extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); extern int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val, void *data); extern int kprobe_int3_handler(struct pt_regs *regs); extern int kprobe_debug_handler(struct pt_regs *regs); +#else +static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) +{ + return 0; +} #endif /* CONFIG_KPROBES */ #endif /* _ASM_X86_KPROBES_H */ diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h index 04bdaf01112c..e106f3018804 100644 --- a/include/linux/kprobes.h +++ b/include/linux/kprobes.h @@ -182,11 +182,19 @@ DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk); /* * For #ifdef avoidance: */ -static inline int kprobes_built_in(void) + +/* + * Architectures need to override this with their own implementation + * if they care to call kprobe_page_fault(). This will just ensure + * that kprobe_page_fault() returns false when called without having + * a proper platform specific definition for kprobe_fault_handler(). + */ +#ifndef kprobe_fault_handler +static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) { - return 1; + return 0; } - +#endif #ifdef CONFIG_KRETPROBES extern void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs); @@ -375,14 +383,6 @@ void free_insn_page(void *page); #else /* !CONFIG_KPROBES: */ -static inline int kprobes_built_in(void) -{ - return 0; -} -static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr) -{ - return 0; -} static inline struct kprobe *get_kprobe(void *addr) { return NULL; @@ -458,12 +458,11 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr) } #endif +#ifdef CONFIG_KPROBES /* Returns true if kprobes handled the fault */ static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, unsigned int trap) { - if (!kprobes_built_in()) - return false; if (user_mode(regs)) return false; /* @@ -476,5 +475,12 @@ static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, return false; return kprobe_fault_handler(regs, trap); } +#else +static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, + unsigned int trap) +{ + return false; +} +#endif #endif /* _LINUX_KPROBES_H */
Architectures like parisc enable CONFIG_KROBES without having a definition for kprobe_fault_handler() which results in a build failure. Arch needs to provide kprobe_fault_handler() as it is platform specific and cannot have a generic working alternative. But in the event when platform lacks such a definition there needs to be a fallback. This adds a stub kprobe_fault_handler() definition which not only prevents a build failure but also makes sure that kprobe_page_fault() if called will always return negative in absence of a sane platform specific alternative. While here wrap kprobe_page_fault() in CONFIG_KPROBES. This enables stud definitions for generic kporbe_fault_handler() and kprobes_built_in() can just be dropped. Only on x86 it needs to be added back locally as it gets used in a !CONFIG_KPROBES function do_general_protection(). Cc: Vineet Gupta <vgupta@synopsys.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Tony Luck <tony.luck@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Paul Burton <paul.burton@mips.com> Cc: James Hogan <jhogan@kernel.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Vasily Gorbik <gor@linux.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Yoshinori Sato <ysato@users.sourceforge.jp> Cc: Rich Felker <dalias@libc.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Allison Randal <allison@lohutok.net> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Enrico Weigelt <info@metux.net> Cc: Richard Fontana <rfontana@redhat.com> Cc: Kate Stewart <kstewart@linuxfoundation.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc: x86@kernel.org Cc: linux-snps-arc@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-ia64@vger.kernel.org Cc: linux-mips@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-s390@vger.kernel.org Cc: linux-sh@vger.kernel.org Cc: sparclinux@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arc/include/asm/kprobes.h | 1 + arch/arm/include/asm/kprobes.h | 1 + arch/arm64/include/asm/kprobes.h | 1 + arch/ia64/include/asm/kprobes.h | 1 + arch/mips/include/asm/kprobes.h | 1 + arch/powerpc/include/asm/kprobes.h | 1 + arch/s390/include/asm/kprobes.h | 1 + arch/sh/include/asm/kprobes.h | 1 + arch/sparc/include/asm/kprobes.h | 1 + arch/x86/include/asm/kprobes.h | 6 ++++++ include/linux/kprobes.h | 32 ++++++++++++++++++------------ 11 files changed, 34 insertions(+), 13 deletions(-)