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 34559C54E49 for ; Wed, 6 Mar 2024 02:41:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6160C6B0083; Tue, 5 Mar 2024 21:41:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5C6166B0085; Tue, 5 Mar 2024 21:41:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 48D136B0088; Tue, 5 Mar 2024 21:41:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 3A0CD6B0083 for ; Tue, 5 Mar 2024 21:41:18 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id D84C8140BE0 for ; Wed, 6 Mar 2024 02:41:17 +0000 (UTC) X-FDA: 81865062594.18.A38450E Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by imf02.hostedemail.com (Postfix) with ESMTP id CA7F280002 for ; Wed, 6 Mar 2024 02:41:15 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=UjDBfmY0; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf02.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709692876; 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=9b+t0q2L1C6peeCZCLsWZvU/SjqX2SpibQfmW/GkLsM=; b=LNHohiW5Je10WvF7lOxNwZVWFW9zzf6RUWy3gS6Z+ErYlTHdFcgL0LsVQrMBs11nSofmmZ 0Z4sJuJI7nY1SAB0WiCzy7PmROa8UGyO2h3Pptr/Pm+6tt4V0laMj5U3UVjIR00gFHHavW bc3XTK8m/kq8fuBnVI7pvcNK0NHWJvE= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=UjDBfmY0; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf02.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709692876; a=rsa-sha256; cv=none; b=WCB7IF/y4FMKJ2PD9Wn/WBPQSL84Q6pgLof+1iJIdKNyLiDBnTxfQcK9o6YXPIAD6eEdCy UBL/ns71kG+Lta3ANVNgXLme40b6ErXFPf90NC4aJyebRiP4nrU4+TM0gf22pDj295GTZw Q97id32Pc5U/8SOCCUuj55/STQ9pDZo= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1709692876; x=1741228876; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=foxpAO3zNvBtAsxiTI5xgxbfm6Fwoz5RnfFjuTvze4g=; b=UjDBfmY0CXKDs0f90jt0UHSnKepwNLLrzFuq4uVQMyvQDiSOuUgUPr43 5LKPAilHJlPNavls26o3I+Z+jYH9d4EAay303QgPiO+Y+5q4CFDZ851wp zmBdIn1IhFsA6uUmItIBxgw6NrQ8h2tkHUZeJHfJK68akijfcSU7OusHk l2BAXNxb9mc7/2M8I75q/r+QY8TTFRfeqaqFvNYbTdfIJ7TOcAwec3/DP 8s2K9Pp9MRKeNi7j3hPXWe5rZ4tDrW7j0VnBrafw6gSJeYAVk4R2hpVyG vYZL8vm2Ds8gvxl9d18s/FHihwy0hl6aQCQLv8xE+O16jXkqnHMeZxENh A==; X-IronPort-AV: E=McAfee;i="6600,9927,11004"; a="4397677" X-IronPort-AV: E=Sophos;i="6.06,207,1705392000"; d="scan'208";a="4397677" Received: from fmviesa003.fm.intel.com ([10.60.135.143]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Mar 2024 18:41:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,207,1705392000"; d="scan'208";a="14156593" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmviesa003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Mar 2024 18:41:12 -0800 From: "Huang, Ying" To: David Hildenbrand Cc: Ryan Roberts , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff() In-Reply-To: <017414bc-78cd-4aa1-9edf-6ce947b9e4e4@redhat.com> (David Hildenbrand's message of "Tue, 5 Mar 2024 23:05:11 +0100") References: <20240305151349.3781428-1-ryan.roberts@arm.com> <8989df79-84f5-488c-bd74-c11d2241eff1@arm.com> <017414bc-78cd-4aa1-9edf-6ce947b9e4e4@redhat.com> Date: Wed, 06 Mar 2024 10:39:18 +0800 Message-ID: <87a5nc84o9.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspam-User: X-Stat-Signature: 1mw8mn1q4sbgx3z6nfbii9ojz8oof419 X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: CA7F280002 X-HE-Tag: 1709692875-204261 X-HE-Meta: U2FsdGVkX1/Do4LmfmEenCCmV4t5MQLq+v1UOHesEdJt53ibmXV9PfrB17xy33CiOW7QTOsXQpcwK5zgXymRJK1WWfF3PRethF9y6MvWq27ybfw2Z7JMb6NWbqocEHrBBcR5S9qTjRYVBMvX/fZwsrYABHwTkPTWmP2szQ+PooOnqIwedin3En0eX5uAJwYz3FZ+3wmhLjVx6EXqkrLtf2u2Hkxb8MSbxcei69UcJCOAIAjebx5VI7Mtr/sR9GKgGL7nJdrTRh2JjcmGLyoQyQDtm+ZysW0wPKKDF9kPtXOky1vOuBb1NT9O6Lr0IPQ32o6hlo1B7iiPLoObzt7JvjeEwWQt7aa7Dgj9CIvVT99A9h11ZZuo/TzuwQoPYasK1ABAPKG8T1n9s7o78EbKfaAMf95Jg8ntEA5Z4a+ufzn8b8kADE1WJEZrH3s8IKLDITjUhiHSLnexpVq1Uxnl8oVZrNpTfPAAU98Dp13XAoJkq2dC3OaWrI2A6u7bkVQuE8KipH2CoGtpgYWVNa66KqicZSRnU+NupMlGmQ+ffovHLjy1wvLxb8iaJsoPm7Ja9e2TvY0Q/TL4Q0dyEcuk5/Fw4aqGRjd165nYajOq+of+t0ilYW6vOaB847FHYV95mQRuGgBZ2dTSpwXtujqL2CrmX4gwzVRzaCwqLCAH0z0sdRJyDFFYQejWfxdVDZmoY5SoSYr465cToLcD8m4o5ksnaIjPT7KVmaurxaX+7lwQriWmiOHzGJWZFTvwnqYvN2O2ypO+oQN+8H7BXq92wos6oEQtRUqh5/KxSzMak2mtF431wY57E7rn8dI3AIbG7gjzXO0gthGN8p/5qGqw0PbPusgF0oYxj3nl7XkXmPG6W9H4u4tTJGe9LgB+ThzKqKvUsjFHRKs= 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: List-Subscribe: List-Unsubscribe: David Hildenbrand writes: > On 05.03.24 17:33, Ryan Roberts wrote: >> On 05/03/2024 15:50, David Hildenbrand wrote: >>> On 05.03.24 16:13, Ryan Roberts wrote: >>>> There was previously a theoretical window where swapoff() could run and >>>> teardown a swap_info_struct while a call to free_swap_and_cache() was >>>> running in another thread. This could cause, amongst other bad >>>> possibilities, swap_page_trans_huge_swapped() (called by >>>> free_swap_and_cache()) to access the freed memory for swap_map. >>>> >>>> This is a theoretical problem and I haven't been able to provoke it from >>>> a test case. But there has been agreement based on code review that this >>>> is possible (see link below). >>>> >>>> Fix it by using get_swap_device()/put_swap_device(), which will stall >>>> swapoff(). There was an extra check in _swap_info_get() to confirm that >>>> the swap entry was valid. This wasn't present in get_swap_device() so >>>> I've added it. I couldn't find any existing get_swap_device() call sites >>>> where this extra check would cause any false alarms. >>>> >>>> Details of how to provoke one possible issue (thanks to David Hilenbrand >>>> for deriving this): >>> >>> Almost >>> >>> "s/Hilenbrand/Hildenbrand/" :) >> >> Ahh sorry... I even explicitly checked it against your name on emails... fat >> fingers... > > No need to be sorry. Even your average German person would get it wrong, > because there are other (more common) variants :) > > [...] > >>>> >>> >>> LGTM >>> >>> Are you planning on sending a doc extension for get_swap_device()? >> >> I saw your comment: >> >> We should likely update the documentation of get_swap_device(), that after >> decrementing the refcount, the SI might become stale and should not be touched >> without a prior get_swap_device(). >> >> But when I went to make the changes, I saw the documentation already said: >> >> ...we need to enclose all swap related functions with get_swap_device() and >> put_swap_device()... Notice that swapoff ... can still happen before the >> percpu_ref_tryget_live() in get_swap_device() or after the percpu_ref_put() in >> put_swap_device()... The caller must be prepared for that. >> >> I thought that already covered it? I'm sure as usual, I've misunderstood your >> point. Happy to respin if you have something in mind? > > No need to respin, we could clarify on top, if we decide it makes sense. > > I was thinking about something like this, making it clearer that the PTL > discussion above does not express the corner case we discovered: > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 2b3a2d85e350b..646a436581eee 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1232,6 +1232,11 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *p, > * with get_swap_device() and put_swap_device(), unless the swap > * functions call get/put_swap_device() by themselves. > * > + * Note that when only holding the PTL, swapoff might succeed immediately > + * after freeing a swap entry. Therefore, immediately after > + * __swap_entry_free(), the swap info might become stale and should not > + * be touched without a prior get_swap_device(). > + * > * Check whether swap entry is valid in the swap device. If so, > * return pointer to swap_info_struct, and keep the swap entry valid > * via preventing the swap device from being swapoff, until LGTM. Thanks! -- Best Regards, Huang, Ying