On Wed 20-07-22 16:13:19, Charan Teja Kalla wrote:
> 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.**
only if the value is re-fetched. Not likely but definitely better to
have it covered. That is why I was suggesting READ_ONCE/WRITE_ONCE for
this iperation.
>
> 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.
yes.
--
Michal Hocko
SUSE Labs
