lkml.org 
[lkml]   [2022]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] mm: fix use-after free of page_ext after race with memory-offline
From
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!

\
 
 \ /
  Last update: 2022-07-18 15:59    [W:0.154 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site