Re: [PATCH] mm: fix use-after free of page_ext after race with memory-offline

[David Hildenbrand]

> 
>> 2) I really dislike having to scatter section online checks all over the
>> place in page ext code. Once there is a difference between active vs.
>> stale page ext data things get a bit messy and error prone. This is
>> already ugly enough in our generic memmap handling code IMHO.
> 
> They should represent a free page in any case so even they are stall
> they shouldn't be really dangerous, right?
Good question. The use-after-free tells me that there could at least be
something accessing page_ext data after offlining right now. As long as
it's only unsynchronized read access, we should be fine.

> 
>> 3) Having on-demand allocations, such as KASAN or page ext from the
>> memory online notifier is at least currently cleaner, because we don't
>> have to handle each and every subsystem that hooks into that during the
>> core memory hotadd/remove phase, which primarily only setups the
>> vmemmap, direct map and memory block devices.
> 
> Cannot this hook into __add_pages which is the real implementation of
> the arch independent way to allocate vmemmap. Or at the sparsemem level
> because we do not (and very likely won't) support memory hotplug on
> any other memory model.

As __add_pages() is also called from mm/memremap.c where we don't want
that metadata, we'd have to special-case (would need a new parameter I
guess).

> 
>> Personally, I think what we have in this patch is quite nice and clean.
>> But I won't object if it can be similarly done in a clean way from
>> hot(un)plug code.
> 
> Well, if the scheme can be done without synchronize_rcu for each section
> which can backfire and if the scheme doesn't add too much complexity to
> achieve that then sure I won't object. I just do not get why page_ext
> should have a different allocation lifetime expectancy than a real page.
> Quite confusing if you ask me.

In contrast to memmap, people actually test for zero pointers here.

If you ask me the memmap access is ugly enough and I don't really enjoy
other metadata following that pattern of "stale and suddenly removed".
Here seems to be an easy way to do it in a clean way.

But yes, if the synchronize_rcu turns out problematic, we'd either have
to optimize or move the allcoation/free phase.

-- 
Thanks,

David / dhildenb