Re: [BUGFIX][PATCH -rc/-mm] prevent kprobes from catching spurious page faults

[Ingo Molnar <mingo@elte.hu>]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
> > * Ingo Molnar (mingo@elte.hu) wrote:
> > > 
> > > * Masami Hiramatsu <mhiramat@redhat.com> wrote:
> > > 
> > > > -	if (notify_page_fault(regs))
> > > > -		return;
> > > >  	if (unlikely(kmmio_fault(regs, address)))
> > > >  		return;
> > > > 
> > > > @@ -634,6 +632,9 @@ void __kprobes do_page_fault(struct pt_r
> > > >  		if (spurious_fault(address, error_code))
> > > >  			return;
> > > > 
> > > > +		/* kprobes don't want to hook the spurious faults. */
> > > > +		if (notify_page_fault(regs))
> > > > +			return;
> > > >  		/*
> > > >  		 * Don't take the mm semaphore here. If we fixup a prefetch
> > > >  		 * fault we could otherwise deadlock.
> > > > @@ -641,6 +642,9 @@ void __kprobes do_page_fault(struct pt_r
> > > >  		goto bad_area_nosemaphore;
> > > >  	}
> > > > 
> > > > +	/* kprobes don't want to hook the spurious faults. */
> > > > +	if (notify_page_fault(regs))
> > > > +		return;
> > > 
> > > I dont know - this spreads that callback to two places now. Any
> > > reason why kprobes cannot call spurious_fault(), if there's a
> > > probe active?
> > > 
> > > Also, moving that would remove the planned cleanup of merging these
> > > two into one call:
> > > 
> > >  	if (notify_page_fault(regs))
> > >  		return;
> > >   	if (unlikely(kmmio_fault(regs, address)))
> > >   		return;
> > > 
> > > We should reduce the probing cross section, not increase it,
> > > especially in such a critical codepath as the pagefault handler.
> > > 
> > > Btw., why cannot kprobes install a dynamic probe to the fault
> > > handler itself? That way the default path would have no such
> > > callbacks and checks at all.
> > > 
> > 
> > Or we could simply merge my 2 LTTng page fault handler tracepoints per
> > architecture and be done with it ?
> > 
> > I'd need to clean up the patchset a little bit to fold a few patches,
> > but that would be straightforward enough.
> 
> yes, that would be an option too - it depends on the details of how it looks 
> like and what kind of complexity it hides.

Linus just merged the fix so the urgency of the matter has become lower :)

	Ingo