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

[Charan Teja Kalla <quic_charante@quicinc.com>]

Thanks David for the inputs!!

On 7/27/2022 10:59 PM, David Hildenbrand wrote:
>> Fix those paths where offline races with page_ext access by maintaining
>> synchronization with rcu lock and is achieved in 3 steps:
>> 1) Invalidate all the page_ext's of the sections of a memory block by
>> storing a flag in the LSB of mem_section->page_ext.
>>
>> 2) Wait till all the existing readers to finish working with the
>> ->page_ext's with synchronize_rcu(). Any parallel process that starts
>> after this call will not get page_ext, through lookup_page_ext(), for
>> the block parallel offline operation is being performed.
>>
>> 3) Now safely free all sections ->page_ext's of the block on which
>> offline operation is being performed.
>>
>> Thanks to David Hildenbrand for his views/suggestions on the initial
>> discussion[1] and Pavan kondeti for various inputs on this patch.
>>
>> FAQ's:
>> Q) Should page_ext_[get|put]() needs to be used for every page_ext
>> access?
>> A) NO, the synchronization is really not needed in all the paths of
>> accessing page_ext. One case is where extra refcount is taken on a
>> page for which memory block, this pages falls into, offline operation is
>> being performed. This extra refcount makes the offline operation not to
>> succeed hence the freeing of page_ext.  Another case is where the page
>> is already being freed and we do reset its page_owner.
>>
>> Some examples where the rcu_lock is not taken while accessing the
>> page_ext are:
>> 1) In migration (where we also migrate the page_owner information), we
>> take the extra refcount on the source and destination pages and then
>> start the migration. This extra refcount makes the test_pages_isolated()
>> function to fail thus retry the offline operation.
>>
>> 2) In free_pages_prepare(), we do reset the page_owner(through page_ext)
>> which again doesn't need the protection to access because the page is
>> already freeing (through only one path).
>>
>> So, users need not to use page_ext_[get|put]() when they are sure that
>> extra refcount is taken on a page preventing the offline operation.
>>
>> Q) Why can't the page_ext is freed in the hot_remove path, where memmap
>> is also freed ?
>>
>> A) As per David's answers, there are many reasons and a few are:
>> 1) Discussions had happened in the past to eventually also use rcu
>> protection for handling pfn_to_online_page(). So doing it cleanly here
>> is certainly an improvement.
>>
>> 2) It's not good 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.
>>
>> 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.
>>
>> [1] https://lore.kernel.org/linux-mm/59edde13-4167-8550-86f0-11fc67882107@quicinc.com/
>>
> I guess if we care about the synchronize_rcu() we could go crazy with
> temporary allocations for data-to-free + call_rcu().

IMO, single synchronize_rcu() call overhead shouldn't be cared
especially if the memory offline operation it self is expected to
complete in seconds. On the Snapdragon system, I can see the lowest it
can complete in 3-4secs for a complete memory block of size 512M. And
agree that this time depends on lot of other factors too but wanted to
raise a point that it is really not a path where tiny optimizations
should be strictly considered. __Please help in correcting me If I am
really downplaying the scenario here__.

But then I moved to single synchronize_rcu() just to avoid any visible
effects that can cause by multiple synchronize_rcu() for a single memory
block with lot of sections.

Having said that, I am open to go for call_rcu() and infact it will be a
much simple change where I can do the freeing of page_ext in the
__free_page_ext() itself which is called for every section there by
avoid the extra tracking flag PAGE_EXT_INVALID.
      ...........
        WRITE_ONCE(ms->page_ext, NULL);
	call_rcu(rcu_head, fun); // Free in fun()
       .............

Or your opinion is to use call_rcu () only once in place of
synchronize_rcu() after invalidating all the page_ext's of memory block?

Thanks,
Charan