Thanks Michal & Pavan,
On 7/20/2022 2:40 PM, Michal Hocko wrote:
>>>> Thanks! The most imporant part is how the exclusion is actual achieved
>>>> because that is not really clear at first sight
>>>>
>>>> CPU1 CPU2
>>>> lookup_page_ext(PageA) offlining
>>>> offline_page_ext
>>>> __free_page_ext(addrA)
>>>> get_entry(addrA)
>>>> ms->page_ext = NULL
>>>> synchronize_rcu()
>>>> free_page_ext
>>>> free_pages_exact (now addrA is unusable)
>>>>
>>>> rcu_read_lock()
>>>> entryA = get_entry(addrA)
>>>> base + page_ext_size * index # an address not invalidated by the freeing path
>>>> do_something(entryA)
>>>> rcu_read_unlock()
>>>>
>>>> CPU1 never checks ms->page_ext so it cannot bail out early when the
>>>> thing is torn down. Or maybe I am missing something. I am not familiar
>>>> with page_ext much.
>>>
>>> Thanks a lot for catching this Michal. You are correct that the proposed
>>> code from me is still racy. I Will correct this along with the proper
>>> commit message in the next version of this patch.
>>>
>> Trying to understand your discussion with Michal. What part is still racy? We
>> do check for mem_section::page_ext and bail out early from lookup_page_ext(),
>> no?
>>
>> Also to make this scheme explicit, we can annotate page_ext member with __rcu
>> and use rcu_assign_pointer() on the writer side.
Annotating with __rcu requires all the read and writes to ms->page_ext
to be under rcu_[access|assign]_pointer which is a big patch. I think
READ_ONCE and WRITE_ONCE, mentioned by Michal, below should does the job.
>>
>> 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);
>> /*
>> * The sanity checks the page allocator does upon freeing a
>> * page can reach here before the page_ext arrays are
>> * allocated when feeding a range of pages to the allocator
>> * for the first time during bootup or memory hotplug.
>> */
>> if (!section->page_ext)
>> return NULL;
>> return get_entry(section->page_ext, pfn);
>> }
> You are right. I was looking at the wrong implementation and misread
> ifdef vs. ifndef CONFIG_SPARSEMEM. My bad.
>
There is still a small race window b/n ms->page_ext setting NULL and its
access even under CONFIG_SPARSEMEM. In the above mentioned example:
CPU1 CPU2
rcu_read_lock()
lookup_page_ext(PageA): offlining
offline_page_ext
__free_page_ext(addrA)
get_entry(addrA)
if (!section->page_ext)
turns to be false.
ms->page_ext = NULL
addrA = get_entry(base=section->page_ext):
base + page_ext_size * index;
**Since base is NULL here, caller
can still do the dereference on
the invalid pointer address.**
synchronize_rcu()
free_page_ext
free_pages_exact (now )
> Memory hotplug is not supported outside of CONFIG_SPARSEMEM so the
> scheme should really work. I would use READ_ONCE for ms->page_ext and
> WRITE_ONCE on the initialization side.
Yes, I should be using the READ_ONCE() and WRITE_ONCE() here.
Thanks,
Charan
