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 C05FDC4828F for ; Wed, 7 Feb 2024 05:42:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EFE5A6B0071; Wed, 7 Feb 2024 00:42:15 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E87E66B0072; Wed, 7 Feb 2024 00:42:15 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D281B6B0074; Wed, 7 Feb 2024 00:42:15 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id BBB026B0071 for ; Wed, 7 Feb 2024 00:42:15 -0500 (EST) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 3D663C0B4A for ; Wed, 7 Feb 2024 05:42:15 +0000 (UTC) X-FDA: 81763912230.27.30DF969 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf03.hostedemail.com (Postfix) with ESMTP id 821CE20010 for ; Wed, 7 Feb 2024 05:42:12 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=OLzDuAyD; spf=pass (imf03.hostedemail.com: domain of chrisl@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707284533; 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=Q6PvVrwtykFAedRLTrpVRFlK15B4Ux3U2JcFpT+sCZ0=; b=1iNy/GO4iy+RL1V5BDUuA2CA+t5NQ8edafXIYN40W4xUfbwcXKs0n4+JWpZwgKiChBjysu 6tWaKhNLhmUWOQLdb9ot2V9O5r7sDGx0cJuf0VB/hXLB40kewFugS6LYbln5qPdXvDvi7v hZ9Gq0G/i1TFpIgToMV4o0GKkZB+kjg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707284533; a=rsa-sha256; cv=none; b=pRs48tmrsNwvqe1Dqqo93H90xCQvIF1RNcxa0WUbASrInKwSQfQeBk2UEn3+nvZ4kXmJzP m/KW1nEtlYgDHDcDrY5zMZeIqzcj+LZL4fZCEOP/BS8cFgFQlkozqkyklYqVarsGy516YZ TYzeZ+kH2YeVwnIkn4PLeKwVYADmuus= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=OLzDuAyD; spf=pass (imf03.hostedemail.com: domain of chrisl@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=chrisl@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 78E14CE182B for ; Wed, 7 Feb 2024 05:42:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 47A12C433B1 for ; Wed, 7 Feb 2024 05:42:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1707284526; bh=FgH8tnv5yRc3gbEHjgfT7POB2ce4RuHzox/ix/Fdxw4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=OLzDuAyDkJtvxaKy19srq2BFEWqRtctRUIw3lwOurfqnpSXTekjY8dCPrPAsi4uqe eMkQGMm9ymCeupZTu9jdQWRgnYHxHeRe2V/Z/QR0oms0OPD1fhagjdbc7ZiR0UbvNK GWLLyqeRwWJD1IPEREejUm9PxH+2CNskoaTuJAbc5xjQi3EcqhtdiZV37/M3iwuWei Q6Ux4KD/+GTrT8IiSv5lGkuw0Y93e8NvwItRux6I4DE7FkujU33v5jLfoz6SBtgXo/ 7UgSV1Pgyr+NFbdwPd4egKZxNIgkPvNrmbCGvXOb+0PGSHpt9zSr5b4WvTIajX7K/w bodZCfVJN3XGQ== Received: by mail-io1-f50.google.com with SMTP id ca18e2360f4ac-7c3dda2a76eso14731039f.1 for ; Tue, 06 Feb 2024 21:42:06 -0800 (PST) X-Gm-Message-State: AOJu0YzWKIxZgHKE3jOVfCVgcZsZMN/mXwocnPuV/3JQp897ryKvk4or 0PBz3hOs0+PUJ7dgJ+Y/D/8Ik0VGCGsxu9Czz7zaSIXVlolHcmda47+h+TAKZg1DCM3ctiNJTP+ cY0sm1gvjDJA9h745WukmGlRVI64Cu4ClPh6G X-Google-Smtp-Source: AGHT+IGDXoXz7jBkmllm+YN/sz23Nu5IvF4AVg3js2wUAhQmGGQojd44IpYSEBJF+vwMJHRgA+5ET+KyIosZ+mdbTG0= X-Received: by 2002:a92:d407:0:b0:363:d9eb:c2de with SMTP id q7-20020a92d407000000b00363d9ebc2demr1383300ilm.6.1707284525573; Tue, 06 Feb 2024 21:42:05 -0800 (PST) MIME-Version: 1.0 References: <20240207033857.3820921-1-chengming.zhou@linux.dev> In-Reply-To: <20240207033857.3820921-1-chengming.zhou@linux.dev> From: Chris Li Date: Tue, 6 Feb 2024 21:41:53 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3] mm/zswap: invalidate old entry when store fail or !zswap_enabled To: chengming.zhou@linux.dev Cc: hannes@cmpxchg.org, yosryahmed@google.com, nphamcs@gmail.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Chengming Zhou , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: t553mxzuob7easg6puwpn7j4jiqtkxz3 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 821CE20010 X-Rspam-User: X-HE-Tag: 1707284532-152053 X-HE-Meta: U2FsdGVkX1/uXPpzE7XCbbp3xh6uBkPeiCFZEwBs8b8jcXCZLKaxvWPf37XqSH6Rx20dPoZqjGDuV4eiRqzUHZnpTczpwnubNqC7EvT//c4aqJCpI8b8gSqKUVlKkSx5CBHIbmOu9h1UEWWM6HmTRJ7Yh5dC6P5p6nyuHF3MES232oQdojCaYD7MKqWc6YoD6+G21WI4xr6BAfOMSBj0s+dHgxoi5aQ6dvSDI+6eQwa4zJ2fK6iFwP2z3qXfmfO6++mMYYhEwQoZJCOvb6ny9Pyj2fmV3q8Rnls1u6b9xq61n3y1IkF2W0Tj222C8cXLyM5aOfHyt+6trOSQY05cmbObUMJPHsnaG03sfuSrLn43sHah04OvnPEssA/rpkYvbbwmYF6cvoANOTGK6QLatHopViDOdHLfKfeOKNZz7l1Tj11+SojZaLhpNnN6GHpb9cQd0TDh65d6sC8s4stqGGPa9oiL0gWAJZNGDaOEuNL3lgoAquluU8y9chhUfJmC+6AK8nQVIZxr5X/I57FjO8O2CdeN64e2wjc8WHpugGveOLL6ntRyZaIl6NKRon9vpRsEdx7w4KS2PmrTincTFWDcE0wnCMjFUbKYWvX/RwKvBGXYinWaJv8LCKEpaj9B3/cju/lo3bPz6fZCL35I8648xhnQrchXmM/6pHOjkrS3qz7str9ykt5S37ewv8P206bplbEwEUIgqR4VoWqEKYlkQEPBX1zZAUMrN3nVdf6s2MSChFRcCfeZtECiRkcIDh8LNLW5zU5Nt/ZKqEIulV6/cIaVgkMTAxjjCPWuWWd05l0Wjwz5qjsS+ct7B1mzy8LT8vUTdhqNM5wEpLw29pG9cwm2tVwJQi/LuFtwQdQmIs9LLVWEJ1B6FbITXVvsTdHTR68b7M/Fe6bjfwMibSUaYum+LOUti7QvxG9Txsglru5letNQQAQbJAXPrAPyORDvnwsfUPjX3/f4PO+ FX+wJwMT CkmEIO0HLGTSmBVG3aBihpT5gMlZpuoS6HivrCH50f6uxRBPUtweMfF4XTTsTg15KWpHXX+zkf/hJPSx3XqpRNPYyAn92NEakhpbPE1xXOlcwcuB+IEQ4fsTCOpEVXMXLn6EzbZHqjaEyN+Hejn0UPwggsa3Qjcl+7618D3ZbScKetCfrPsrgwH0dc3WdvZ9dul2oAJlhqSlHRTaFLKJbHWV6pTIZvY62WEwbFiE6DHG4HEFwbOlY3WTDVdSRz5ImR97OUxXUHJfrPTL3BtlHnEKUyC9GEKSXxTO1ilECoaTUhf+tnbRIaGTbX76VOKZoKG06NE5Op1fjfA/jQBnooKRumQn9tZRnZdl44rr94TSdEu5DFkWK/JoeijVdTfGk+90GJX3TkjwdH3qx35EYhqhIGetET7EZTUKlJ2346TWiKyTORmZXNq4fmafjz2eytQN8F2FIB8sFamMZSLTpq6lYmtT7vEJux0FOKDm7hnzklTcgOX7DNEWFHIeS1E1YjFT490mTOjNUXYFmI0GDZdFvzQ== 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 Tue, Feb 6, 2024 at 7:39=E2=80=AFPM 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. Thanks, I have been wondering about the cause of that for a while. > > 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 t= o store > - * it, otherwise, if the store fails the old page won't be remove= d from > - * the tree, and it might be written back overriding the new data= . > - */ > - spin_lock(&tree->lock); > - entry =3D zswap_rb_search(&tree->rbroot, offset); > - if (entry) > - zswap_invalidate_entry(tree, entry); > - spin_unlock(&tree->lock); > objcg =3D get_obj_cgroup_from_folio(folio); > if (objcg && !obj_cgroup_may_zswap(objcg)) { > memcg =3D 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 duplicat= e is > - * found again here it means that something went wrong in the swa= p > - * 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) =3D=3D -E= EXIST) { > - WARN_ON(1); > + if (zswap_rb_insert(&tree->rbroot, entry, &dupentry) =3D=3D -EEXI= ST) { > zswap_invalidate_entry(tree, dupentry); > + VM_WARN_ON(zswap_rb_insert(&tree->rbroot, entry, &dupentr= y)); It seems there is only one path called zswap_rb_insert() and there is no loop to repeat the insert any more. Can we have the zswap_rb_insert() install the entry and return the dupentry? We can still just call zswap_invalidate_entry() on the duplicate. The mapping of the dupentry has been removed when zswap_rb_insert() returns. That will save a repeat lookup on the duplicate case. After this change, the zswap_rb_insert() will map to the xarray xa_store() pretty nicely. Chris > } > 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 invalid= ate the > + * possibly stale entry which was previously stored at this offse= t. > + * Otherwise, writeback could overwrite the new data in the swapf= ile. > + */ > + spin_lock(&tree->lock); > + entry =3D zswap_rb_search(&tree->rbroot, offset); > + if (entry) > + zswap_invalidate_entry(tree, entry); > + spin_unlock(&tree->lock); > return false; > > shrink: > -- > 2.40.1 > >