Re: [PATCH] mm/kprobes: Add generic kprobe_fault_handler() fallback definition

[Masami Hiramatsu <mhiramat@kernel.org>]

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>