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

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

Thanks Michal for the reviews!!

On 7/28/2022 8:07 PM, Michal Hocko wrote:
>> 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.
> This is just subtlety and something that can get misunderstood over
> time. Moreover there is no documentation explaining the difference.
> What is the reason to have these two different APIs in the first place.
> RCU read side is almost zero cost. So what is the point?
Currently not all the places where page_ext is being used is put under
the rcu_lock. I just used rcu lock in the places where it is possible to
have the use-after-free of page_ext. You recommend to use rcu lock while
using with page_ext in all the places?

My only point here is since there may be a non-atomic context exist
across page_ext_get/put() and If users are sure that this page's
page_ext will not be freed by parallel offline operation, they need not
get the rcu lock.

I agree that this can be misunderstood over time, let me check if I can
use page_ext_get/put in all the places.

>> @@ -57,6 +60,11 @@ static inline void page_ext_init(void)
>>  
>>  struct page_ext *lookup_page_ext(const struct page *page);
>>  
>> +static inline bool page_ext_invalid(struct page_ext *page_ext)
>> +{
>> +	return !page_ext || (((unsigned long)page_ext & PAGE_EXT_INVALID) == 1);
>> +}
>> +
> No real reason to expose this into a header file. Nothing but page_ext.c
> should know and care about this.
Agree. Will move it accordingly.

> 
>> +static inline struct page_ext *page_ext_get(struct page *page)
>> +{
>> +	struct page_ext *page_ext;
>> +
>> +	rcu_read_lock();
>> +	page_ext = lookup_page_ext(page);
>> +	if (!page_ext) {
>> +		rcu_read_unlock();
>> +		return NULL;
>> +	}
>> +
>> +	return page_ext;
> If you make this an extern you can actually hide lookup_page_ext and
> prevent from future bugs where people are using non serialized API
> without realizing that.

This design looks good. Let me check the feasibility in its implementation.

>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..404a2eb 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -211,15 +211,17 @@ struct page_ext *lookup_page_ext(const struct page *page)
>>  {
>>  	unsigned long pfn = page_to_pfn(page);
>>  	struct mem_section *section = __pfn_to_section(pfn);
>> +	struct page_ext *page_ext = READ_ONCE(section->page_ext);
>> +
> 	WARN_ON_ONCE(!rcu_read_lock_held());

Again this requires page_ext usage should be under the rcu lock always
by the user.

> 
>>  static void *__meminit alloc_page_ext(size_t size, int nid)
>> @@ -298,9 +300,26 @@ static void __free_page_ext(unsigned long pfn)
>>  	ms = __pfn_to_section(pfn);
>>  	if (!ms || !ms->page_ext)
>>  		return;
>> -	base = get_entry(ms->page_ext, pfn);
>> +
>> +	base = READ_ONCE(ms->page_ext);
>> +	if (page_ext_invalid(base))
>> +		base = (void *)base - PAGE_EXT_INVALID;
> All page_ext accesses should use the same fetched pointer including the
> ms->page_ext check. Also page_ext_invalid _must_ be true here otherwise
> something bad is going on so I would go with
> 	if (WARN_ON_ONCE(!page_ext_invalid(base)))
> 		return;
> 	base = (void *)base - PAGE_EXT_INVALID;

The roll back operation in the online_page_ext(), where we free the
allocated page_ext's, will not have the PAGE_EXT_INVALID flag thus
WARN() may not work here. no?
> 

Thanks,
Charan