Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Jul 2022 13:13:19 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Charan Teja Kalla <quic_charante@quicinc.com>
Cc:     Pavan Kondeti <quic_pkondeti@quicinc.com>,
        akpm@linux-foundation.org, pasha.tatashin@soleen.com,
        sjpark@amazon.de, sieberf@amazon.com, shakeelb@google.com,
        dhowells@redhat.com, willy@infradead.org, vbabka@suse.cz,
        david@redhat.com, minchan@kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org,
        "iamjoonsoo.kim@lge.com" <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH] mm: fix use-after free of page_ext after race with
 memory-offline
Message-ID: <YtfjT5gs/sa7uzrt@dhcp22.suse.cz>
References: <1657810063-28938-1-git-send-email-quic_charante@quicinc.com>
 <YtVJBQ/ZOt22o8+B@dhcp22.suse.cz>
 <fca23df7-37b0-f32d-ece3-58317dfad210@quicinc.com>
 <YtV0PSMAGG46Pq0K@dhcp22.suse.cz>
 <aaeec83d-bdf8-280c-b943-ad510f1d8db2@quicinc.com>
 <20220720082112.GA14437@hu-pkondeti-hyd.qualcomm.com>
 <YtfGeUUoi9cq3g0A@dhcp22.suse.cz>
 <f7e1efa4-43da-22e0-b748-d0855ecc7456@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f7e1efa4-43da-22e0-b748-d0855ecc7456@quicinc.com>
Precedence: bulk

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