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 E807EC4828D for ; Wed, 7 Feb 2024 11:36:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 798846B0071; Wed, 7 Feb 2024 06:36:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 720F16B0072; Wed, 7 Feb 2024 06:36:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5C1D66B0074; Wed, 7 Feb 2024 06:36:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 450AF6B0071 for ; Wed, 7 Feb 2024 06:36:23 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 17C3B1208E5 for ; Wed, 7 Feb 2024 11:36:23 +0000 (UTC) X-FDA: 81764804646.11.2EB6C48 Received: from out-182.mta1.migadu.com (out-182.mta1.migadu.com [95.215.58.182]) by imf20.hostedemail.com (Postfix) with ESMTP id 301221C000D for ; Wed, 7 Feb 2024 11:36:20 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=RyHYWm9R; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf20.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707305781; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=BwYC5Iw0QVlv46eQ3xV5uqeU2c0WlPeQhqg/YeYM5W8=; b=hU+fWhGTYuErexd0Eg8znpOGK4SzGg0d813BoGZa9BK/Uwn+IxssTzl1fiutlCaXfdJRml vJ0NMDs9qxJ9QqdwmLpiLPAkI3Ruy+Jh/a8GWWUyKukCE00ArdutP3SVicVwtTT+MAm6ux pB0G1Ph6h99tmzhAk7pyiR5NVM+m4Zs= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=RyHYWm9R; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf20.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707305781; a=rsa-sha256; cv=none; b=RddB5g4V8EGWlTyBRwyWOcUlBo/0dRvSpV6ZMWP5NCxsKN95pzyLE4hpQCeilVDi8xrsqj maLQSrhJ+Q7McH98+E8+TifMdokXDIN83tBzsJsgGYjhfWVkXzEMhe4VDzxNxcEA+CGZnz mdw4sSa6nLuXKDi1Idx9Y4GYOPc9/ow= Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1707305779; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=BwYC5Iw0QVlv46eQ3xV5uqeU2c0WlPeQhqg/YeYM5W8=; b=RyHYWm9Rq12CQe3E+U+/nAMYv2rW2Ysz9pKaVvq2bqBISZiYgUBMn6/YkIO3lAOiNcTHAY CHJLgcu93mBOQmQ2YgtzDyYAFZIuIxVyhDTKXQ+0blnsdZBDD+nUPkB/Yxeu9gglxuJMqN +68AeJPM3kWV0mcSfzaRwsdH7IOg+VU= Date: Wed, 7 Feb 2024 19:36:06 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v3] mm/zswap: invalidate old entry when store fail or !zswap_enabled Content-Language: en-US To: hannes@cmpxchg.org, yosryahmed@google.com, nphamcs@gmail.com, akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Chengming Zhou , stable@vger.kernel.org References: <20240207033857.3820921-1-chengming.zhou@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <20240207033857.3820921-1-chengming.zhou@linux.dev> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Stat-Signature: 5furggjenwt3a18ythr3g5qz1gm8t3ti X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 301221C000D X-HE-Tag: 1707305780-677050 X-HE-Meta: U2FsdGVkX1/TX7ASX3bClnk5dW0Y2Y59Ja8gh6/GGD1i/1FhdCDYIs7/9ABguKNR8uvRIRVDT+NnfAAenm+0z6Gzbovhq3js6cE44HE/7JZ75/PVsYIL8Ki9qKHBQCvxJvAYfNmLfFOjigf3qrydn8r6jafrHrA/4Bnot8+vxgPobhk+ZB4H4qVKfF99IRYy8GC+1oa9vjV/ZDvkvDSzvpU0NOcp0rO58tUMJIEwKvmicpEtgY+DFPN9beKqSIwpgyC4/93UOuQxrs1APxauO9R1ELO7gzLOxXJIn6ojJOS+Pvr2TvhvCV/62O6klDDLWmb6q2+SgnFRVDyuJVfo4l4Cef3cJ8XVAKfBIPhDjPVL9Yf5lGtLmMRRHj47w7jx0R/9hFav13xDKMSr5Zh1E6NLmPS/xV+T25p25G2kzt4ZoprFJhTcmi5NVosm7WQNaCFzWRw2oMx1uZlsSiGp1uiMnbP/NmVyP5pq+TL4QSjlizVmK+ime3Qy6ZkRVzcuHxtYbBXRFLU0m0tyk5STuRVtvskF1wjtTbcAGAfONZCfHHBMww4FWK6Lm41ynZE4iUxAZBDumGWoW1JCHHpvnkz/NpFYMWKPLW/0VI+IINK+RBNusoTtV9mXCw6WV0xu7su8abS7a3fyCw257vuQIMi02mcM1w/Fbh6bcuFQvCr5bQ3ePSIznKU0UKAHsMho06p6sKAjq6b2fGVKBDYaD7vVLK8ApMgFqwrv0KS9jlIwhDuAAKo3jjdr490UGwl/LqJbAnWlRyqK6dVlF5EiRr8gwYHWm3KmcatzVCN+TYDQUR+BXz/U7E0NE+qXY6yg0g3LziqJYX28fWT3rQ4uWMXoPDF2qbeHxADB1e0EtKQ1uT6UL8qus2Vs14rtXxgsJyfpUHngGbo= 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: On 2024/2/7 11:38, chengming.zhou@linux.dev wrote: > From: Chengming Zhou > > We may encounter duplicate entry in the zswap_store(): > > 1. swap slot that freed to per-cpu swap cache, doesn't invalidate > the zswap entry, then got reused. This has been fixed. > > 2. !exclusive load mode, swapin folio will leave its zswap entry > on the tree, then swapout again. This has been removed. > > 3. one folio can be dirtied again after zswap_store(), so need to > zswap_store() again. This should be handled correctly. > > So we must invalidate the old duplicate entry before insert the > new one, which actually doesn't have to be done at the beginning > of zswap_store(). And this is a normal situation, we shouldn't > WARN_ON(1) in this case, so delete it. (The WARN_ON(1) seems want > to detect swap entry UAF problem? But not very necessary here.) > > The good point is that we don't need to lock tree twice in the > store success path. > > Note we still need to invalidate the old duplicate entry in the > store failure path, otherwise the new data in swapfile could be > overwrite by the old data in zswap pool when lru writeback. > > We have to do this even when !zswap_enabled since zswap can be > disabled anytime. If the folio store success before, then got > dirtied again but zswap disabled, we won't invalidate the old > duplicate entry in the zswap_store(). So later lru writeback > may overwrite the new data in swapfile. > > Fixes: 42c06a0e8ebe ("mm: kill frontswap") > Cc: > Acked-by: Johannes Weiner > Acked-by: Yosry Ahmed > Signed-off-by: Chengming Zhou > --- > v3: > - Fix a few grammatical problems in comments, per Yosry. > > v2: > - Change the duplicate entry invalidation loop to if, since we hold > the lock, we won't find it once we invalidate it, per Yosry. > - Add Fixes tag. > --- > mm/zswap.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index cd67f7f6b302..d9d8947d6761 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1518,18 +1518,8 @@ bool zswap_store(struct folio *folio) > return false; > > if (!zswap_enabled) > - return false; > + goto check_old; > > - /* > - * If this is a duplicate, it must be removed before attempting to store > - * it, otherwise, if the store fails the old page won't be removed from > - * the tree, and it might be written back overriding the new data. > - */ > - spin_lock(&tree->lock); > - entry = zswap_rb_search(&tree->rbroot, offset); > - if (entry) > - zswap_invalidate_entry(tree, entry); > - spin_unlock(&tree->lock); > objcg = get_obj_cgroup_from_folio(folio); > if (objcg && !obj_cgroup_may_zswap(objcg)) { > memcg = get_mem_cgroup_from_objcg(objcg); > @@ -1608,14 +1598,12 @@ bool zswap_store(struct folio *folio) > /* map */ > spin_lock(&tree->lock); > /* > - * A duplicate entry should have been removed at the beginning of this > - * function. Since the swap entry should be pinned, if a duplicate is > - * found again here it means that something went wrong in the swap > - * cache. > + * The folio may have been dirtied again, invalidate the > + * possibly stale entry before inserting the new entry. > */ > - while (zswap_rb_insert(&tree->rbroot, entry, &dupentry) == -EEXIST) { > - WARN_ON(1); > + if (zswap_rb_insert(&tree->rbroot, entry, &dupentry) == -EEXIST) { > zswap_invalidate_entry(tree, dupentry); > + VM_WARN_ON(zswap_rb_insert(&tree->rbroot, entry, &dupentry)); Oh, I just realized this is empty if !CONFIG_DEBUG_VM, will post v4. Thanks. > } > if (entry->length) { > INIT_LIST_HEAD(&entry->lru); > @@ -1638,6 +1626,17 @@ bool zswap_store(struct folio *folio) > reject: > if (objcg) > obj_cgroup_put(objcg); > +check_old: > + /* > + * If the zswap store fails or zswap is disabled, we must invalidate the > + * possibly stale entry which was previously stored at this offset. > + * Otherwise, writeback could overwrite the new data in the swapfile. > + */ > + spin_lock(&tree->lock); > + entry = zswap_rb_search(&tree->rbroot, offset); > + if (entry) > + zswap_invalidate_entry(tree, entry); > + spin_unlock(&tree->lock); > return false; > > shrink: