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

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

Thanks Michal for the comments!!

On 7/18/2022 5:20 PM, Michal Hocko wrote:
>> The above mentioned race is just one example __but the problem persists
>> in the other paths too involving page_ext->flags access(eg:
>> page_is_idle())__. Since offline waits till the last reference on the
>> page goes down i.e. any path that took the refcount on the page can make
>> the memory offline operation to wait. Eg: In the migrate_pages()
>> operation, we do take the extra refcount on the pages that are under
>> migration and then we do copy page_owner by accessing page_ext. For
>>
>> Fix those paths where offline races with page_ext access by maintaining
>> synchronization with rcu lock.
> Please be much more specific about the synchronization. How does RCU
> actually synchronize the offlining and access? Higher level description
> of all the actors would be very helpful not only for the review but also
> for future readers.

I will improve the commit message about this synchronization change
using RCU's.

> 
> Also, more specifically
> [...]
>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..5ccd3ee 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -299,8 +299,9 @@ static void __free_page_ext(unsigned long pfn)
>>  	if (!ms || !ms->page_ext)
>>  		return;
>>  	base = get_entry(ms->page_ext, pfn);
>> -	free_page_ext(base);
>>  	ms->page_ext = NULL;
>> +	synchronize_rcu();
>> +	free_page_ext(base);
>>  }
> So you are imposing the RCU grace period for each page_ext! This can get
> really expensive. Have you tried to measure the effect?
> 

I didn't really measure the effect. Let me measure it and post these in V2.

> Is there any reason why page_ext is freed during offlining rather when
> it is hotremoved?

This is something I am struggling to get the answer. IMO, this is even
wrong design where I don't have page_ext but page. Moving the freeing of
page_ext to hotremove path actually solves the problem but somehow this
idea didn't liked[1].  copying the excerpt here:

">
> 3) Change the design where the page_ext is valid as long as the struct
> page is alive.

:/ Doesn't spark joy."


@Joonsoo : We see that you did commit eefa864b701d ("mm/page_ext:
resurrect struct page extending code for debugging").  Any reason why
the page_ext is chosen to free at offline operation rather than the
remove operation of a memory block?


[1]
https://lore.kernel.org/linux-mm/8fefe59d-c893-39f4-3225-65343086c867@redhat.com/
> 
> Thanks!