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;
Message-ID: <be4bb3d5-2658-752b-826c-f2dc1359e92d@quicinc.com>
Date:   Thu, 18 Aug 2022 19:31:05 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH V3] mm: fix use-after free of page_ext after race with
 memory-offline
Content-Language: en-US
To:     Michal Hocko <mhocko@suse.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>
References: <1660056403-20894-1-git-send-email-quic_charante@quicinc.com>
 <Yvpg6odyDsXrjw5i@dhcp22.suse.cz>
 <286e47e7-3d63-133c-aa6c-05100b557d42@quicinc.com>
 <YvvCpW0ep9N8CbDr@dhcp22.suse.cz>
From:   Charan Teja Kalla <quic_charante@quicinc.com>
In-Reply-To: <YvvCpW0ep9N8CbDr@dhcp22.suse.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Michal,

On 8/16/2022 9:45 PM, Michal Hocko 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?
I don't think that !page_ext check is really required as
__set_page_owner() is called means page_ext should have been
initialized.  Will raise a separate change for this suggestion. For now
V4 is raised with the earlier suggestion of dropping the initial
page_ext.
https://lore.kernel.org/all/1660830600-9068-1-git-send-email-quic_charante@quicinc.com/.

Thanks,
Charan