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:   Tue, 16 Aug 2022 18:15:33 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Charan Teja Kalla <quic_charante@quicinc.com>
Cc:     akpm@linux-foundation.org, david@redhat.com,
        pasha.tatashin@soleen.com, sieberf@amazon.com, shakeelb@google.com,
        sjpark@amazon.de, dhowells@redhat.com, willy@infradead.org,
        quic_pkondeti@quicinc.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH V3] mm: fix use-after free of page_ext after race with
 memory-offline
Message-ID: <YvvCpW0ep9N8CbDr@dhcp22.suse.cz>
References: <1660056403-20894-1-git-send-email-quic_charante@quicinc.com>
 <Yvpg6odyDsXrjw5i@dhcp22.suse.cz>
 <286e47e7-3d63-133c-aa6c-05100b557d42@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <286e47e7-3d63-133c-aa6c-05100b557d42@quicinc.com>
Precedence: bulk

On Tue 16-08-22 15:04:01, Charan Teja Kalla wrote:
[...]
> >> @@ -183,19 +184,26 @@ static inline void __set_page_owner_handle(struct page_ext *page_ext,
> >>  noinline void __set_page_owner(struct page *page, unsigned short order,
> >>  					gfp_t gfp_mask)
> >>  {
> >> -	struct page_ext *page_ext = lookup_page_ext(page);
> >> +	struct page_ext *page_ext = page_ext_get(page);
> >>  	depot_stack_handle_t handle;
> >>  
> >>  	if (unlikely(!page_ext))
> >>  		return;
> > Either add a comment like this
> > 	/* save_stack can sleep in general so we have to page_ext_put */
> 
> 
> Vlastimil suggested to go for save stack first since !page_ext is mostly
> unlikely.  Snip from his comments:
> Why not simply do the save_stack() first and then page_ext_get() just
> once? It should be really rare that it's NULL, so I don't think we save
> much by avoiding an unnecessary save_stack(), while the overhead of
> doing two get/put instead of one will affect every call.

right see below
> 
> https://lore.kernel.org/all/f5fd4942-b03e-1d1c-213b-9cd5283ced91@suse.cz/
> >> +	page_ext_put();
> >>  
> >>  	handle = save_stack(gfp_mask);
> > or just drop the initial page_ext_get altogether. This function is
> > called only when page_ext is supposed to be initialized and !page_ext
> > case above should be very unlikely. Or is there any reason to keep this?

^^^^^
-- 
Michal Hocko
SUSE Labs