From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A9C50C433EF for ; Mon, 18 Jul 2022 14:55:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0D7266B0072; Mon, 18 Jul 2022 10:55:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 087448E0001; Mon, 18 Jul 2022 10:55:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E92A66B0074; Mon, 18 Jul 2022 10:55:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id D668D6B0072 for ; Mon, 18 Jul 2022 10:55:00 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 9DCDE80C3F for ; Mon, 18 Jul 2022 14:55:00 +0000 (UTC) X-FDA: 79700517960.28.84197E2 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf11.hostedemail.com (Postfix) with ESMTP id 1B0E44008D for ; Mon, 18 Jul 2022 14:54:58 +0000 (UTC) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id DD65E1F86C; Mon, 18 Jul 2022 14:54:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1658156097; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=GYqqskOUH/7Rs8C9Uf77Hro67UXC5uPhE9Py/w8lGFI=; b=TPEaSqJEeqXFATLlnkEi6eI/xEGEBvo4pkt0vmg/kjJ9eUnVtrZUV+y24U+0bMd7OrXSfM Xl+QhPqw+3JvvOD4hlXRy64U54Ym2NSwt9EThaVksk+zwc6VO3+Ep+OiE0hWCKsPgfQHbc SKTIGGpOTiiMhw7V/icDZYqT7cpq15M= Received: from suse.cz (unknown [10.100.201.86]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id D04DF2C141; Mon, 18 Jul 2022 14:54:56 +0000 (UTC) Date: Mon, 18 Jul 2022 16:54:53 +0200 From: Michal Hocko To: Charan Teja Kalla Cc: 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" Subject: Re: [PATCH] mm: fix use-after free of page_ext after race with memory-offline Message-ID: References: <1657810063-28938-1-git-send-email-quic_charante@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1658156099; a=rsa-sha256; cv=none; b=Mp6JCVbYL+IgLf0Z1nsMphDyS+iGq/jqGb1MscuPzBQUTmmHZeIE94BSA6BAApUYtl7VdX uPjlFcRE1zJo0gMOL3uOYAb7ZminvTI67MaKDOMW+uCZ8cUiyeOMYdevOsHsvQQmrd2NuE AOQmAa4W+5/QZioWg/KT1J9YZj/mW7o= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=TPEaSqJE; dmarc=pass (policy=quarantine) header.from=suse.com; spf=pass (imf11.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=mhocko@suse.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1658156099; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GYqqskOUH/7Rs8C9Uf77Hro67UXC5uPhE9Py/w8lGFI=; b=l11ozydUg2mo9LHuu/kQMuJvJdjv1ega/jn+sZddaaupFgCF16yLe8463JAJNsgWxvhCEK NT9YEzTJJSREg6p8ihJEjFPj2a5Hd6hCguxybI2lshGzB7GcYR7OhueTlXZOqEj9bZ6/Xd 8NeMfaxaKDpz2zodAqYr0i7sxuCZ5sE= X-Rspamd-Queue-Id: 1B0E44008D X-Rspam-User: Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=TPEaSqJE; dmarc=pass (policy=quarantine) header.from=suse.com; spf=pass (imf11.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=mhocko@suse.com X-Rspamd-Server: rspam08 X-Stat-Signature: bisxxcrkf96nr9dbo8ohgupnuyftew6g X-HE-Tag: 1658156098-246814 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon 18-07-22 19:28:13, Charan Teja Kalla wrote: > Thanks Michal for the comments!! > > On 7/18/2022 5:20 PM, Michal Hocko wrote: > >> The above mentioned race is just one example __but the problem persists > >> in the other paths too involving page_ext->flags access(eg: > >> page_is_idle())__. Since offline waits till the last reference on the > >> page goes down i.e. any path that took the refcount on the page can make > >> the memory offline operation to wait. Eg: In the migrate_pages() > >> operation, we do take the extra refcount on the pages that are under > >> migration and then we do copy page_owner by accessing page_ext. For > >> > >> Fix those paths where offline races with page_ext access by maintaining > >> synchronization with rcu lock. > > Please be much more specific about the synchronization. How does RCU > > actually synchronize the offlining and access? Higher level description > > of all the actors would be very helpful not only for the review but also > > for future readers. > > I will improve the commit message about this synchronization change > using RCU's. 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. > > Also, more specifically > > [...] > >> diff --git a/mm/page_ext.c b/mm/page_ext.c > >> index 3dc715d..5ccd3ee 100644 > >> --- a/mm/page_ext.c > >> +++ b/mm/page_ext.c > >> @@ -299,8 +299,9 @@ static void __free_page_ext(unsigned long pfn) > >> if (!ms || !ms->page_ext) > >> return; > >> base = get_entry(ms->page_ext, pfn); > >> - free_page_ext(base); > >> ms->page_ext = NULL; > >> + synchronize_rcu(); > >> + free_page_ext(base); > >> } > > So you are imposing the RCU grace period for each page_ext! This can get > > really expensive. Have you tried to measure the effect? I was wrong here! This is for each memory section which is not as terrible as every single page_ext. This can be still quite a lot memory sections in a single memory block (e.g. on ppc memory sections are ridiculously small). > I didn't really measure the effect. Let me measure it and post these in V2. I think it would be much more optimal to split the operation into 2 phases. Invalidate all the page_ext metadata then synchronize_rcu and only then free them all. I am not very familiar with page_ext so I am not sure this is easy to be done. Maybe page_ext = NULL can be done in the first stage. > > Is there any reason why page_ext is freed during offlining rather when > > it is hotremoved? > > This is something I am struggling to get the answer. IMO, this is even > wrong design where I don't have page_ext but page. Moving the freeing of > page_ext to hotremove path actually solves the problem but somehow this > idea didn't liked[1]. copying the excerpt here: yes, it certainly adds subtlety to the page_ext thingy. I do agree that even situation around struct page is not all that great wrt synchronization. We have pfn_to_online_page which even when racy doesn't give you a garbage because hotremove happens very rarely or so long after offlining that the race window is essentially impractically too long for any potential damage. We would have to change a lot to make it work "properly". I am not optimistic this is actually feasible. > > 3) Change the design where the page_ext is valid as long as the struct > > page is alive. > > :/ Doesn't spark joy." I would be wondering why. It should only take to move the callback to happen at hotremove. So it shouldn't be very involved of a change. I can imagine somebody would be relying on releasing resources when offlining memory but is that really the case? -- Michal Hocko SUSE Labs