Re: [RFC][PATCH] x86/mm: Sync all vmalloc mappings before text_poke()

[Steven Rostedt]

On Thu, 30 Apr 2020 22:26:55 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:

> ----- On Apr 30, 2020, at 9:13 PM, rostedt rostedt@xxxxxxxxxxx wrote:
> 
> > [ Joerg, sending again this time not just to you. (hit reply to sender
> >  and not reply to all). Feel free to resend what you wrote before to this ]
> > 
> > On Thu, 30 Apr 2020 21:14:34 +0200
> > Joerg Roedel <jroedel@xxxxxxx> wrote:
> >   
> >> And alloc_percpu() calls down into pcpu_alloc(), which allocates new
> >> percpu chunks using vmalloc() on x86. And there we are again in the
> >> vmalloc area.  
> > 
> > So after a vmalloc() is made, should the page tables be synced?  
> 
> Why should it ? Usually, the page fault handler is able to resolve the
> resulting minor page faults lazily.
> 
> > 
> > This is a rather subtle bug, and I don't think it should be the caller of
> > percpu_alloc() that needs to call vmalloc_sync_mappings().  
> 
> Who said tracing was easy ? ;-)

But anyone can hook to a tracepoint, and then if they hook to one that is
in the page fault handler, and they use vmalloc, they can lock up the
machine.

> 
> > What's your suggestion for a fix?  
> 
> I know the question is not addressed to me, but here are my 2 cents:
> 
> It's subtle because ftrace is tracing the page fault handler through
> tracepoints. It would not make sense to slow down all vmalloc or
> percpu_alloc() just because tracing recurses when tracing page faults.

What's so damn special about alloc_percpu()? It's definitely not a fast
path. And it's not used often.

> 
> I think the right approach to solve this is to call vmalloc_sync_mappings()
> before any vmalloc'd memory ends up being observable by instrumentation.
> This can be achieved by adding a vmalloc_sync_mappings call on tracepoint
> registration like I proposed in my patchset a few week ago:
> 
> https://lore.kernel.org/r/20200409193543.18115-2-mathieu.desnoyers@xxxxxxxxxxxx
> 
> The tracers just have to make sure they perform their vmalloc'd memory
> allocation before registering the tracepoint which can touch it, else they
> need to issue vmalloc_sync_mappings() on their own before making the
> newly allocated memory observable by instrumentation.
> 
> This approach is not new: register_die_notifier() does exactly that today.
> 

I'll give the answer I gave to Joerg when he replied to my accidental
private (not public) email:

Or even my original patch would be better than having the generic tracing
code understanding the intrinsic properties of vmalloc() and
alloc_percpu() on x86_64. I really don't think it is wise to have:

	foo = alloc_percpu();

	/*
	 * Because of some magic with the way alloc_percpu() works on
	 * x86_64, we need to synchronize the pgd of all the tables,
	 * otherwise the trace events that happen in x86_64 page fault
	 * handlers can't cope with accessing the chance that a
	 * alloc_percpu()'d memory might be touched in the page fault trace
	 * event. Oh, and we need to audit all alloc_percpu() and vmalloc()
	 * calls in tracing, because something might get triggered within a
	 * page fault trace event!
	 */
	vmalloc_sync_mappings();

That would be exactly what I add as a comment if it were to be added in the
generic tracing code.

And we would need to audit any percpu alloc'd code in all tracing, or
anything that might git hooked into something that hooks to the page fault
trace point.

Since this worked for a decade without this, I'm strongly against adding it
in the generic code due to some issues with a single architecture.

-- Steve