Hi Anshuman, On Mon, 8 Jul 2019 09:03:13 +0530 Anshuman Khandual <anshuman.khandual@xxxxxxx> 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, -- Masami Hiramatsu <mhiramat@xxxxxxxxxx>
