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

[Mathieu Desnoyers]

----- On Apr 30, 2020, at 10:39 PM, rostedt rostedt@xxxxxxxxxxx wrote:

> 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.

True.

> 
>> 
>> > 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.

If it's not fastpath, then we might want to do this in pcpu_mem_zalloc()
which is used to allocate the percpu chunks:

static void *pcpu_mem_zalloc(size_t size, gfp_t gfp)
{
        if (WARN_ON_ONCE(!slab_is_available()))
                return NULL;

        if (size <= PAGE_SIZE) {
                return kzalloc(size, gfp);
        } else {
                void *p = __vmalloc(size, gfp | __GFP_ZERO, PAGE_KERNEL);
                vmalloc_sync_mappings();
                return p;
        }
}

However this would not solve the entire problem for vmap(), vmalloc() and
friends.

> 
>> 
>> 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.

Yeah, I've myself sprinkled vmalloc_sync_all() for years in LTTng to handle
these corner-cases. Note that module memory is vmalloc'd as well!

> 
> 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.

Indeed, tracing the page fault handler is really tricky exactly because
of minor faults vs vmalloc'd memory interactions.

> 
> 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.

I'm really OK with making all this more robust. I think it's however important
that we are careful not slowing down or adding extra memory overhead to
existing workloads as we do this, especially if the only motivation is
to make tracing marginally simpler.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com