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 7B74BCE7A81 for ; Mon, 25 Sep 2023 08:36:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 13C048D001A; Mon, 25 Sep 2023 04:36:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0ED898D0001; Mon, 25 Sep 2023 04:36:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EF6DC8D001A; Mon, 25 Sep 2023 04:36:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E04F88D0001 for ; Mon, 25 Sep 2023 04:36:23 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B0A3A160666 for ; Mon, 25 Sep 2023 08:36:23 +0000 (UTC) X-FDA: 81274463046.10.0CDC6BB Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by imf28.hostedemail.com (Postfix) with ESMTP id D77F7C000E for ; Mon, 25 Sep 2023 08:36:20 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BJ0xDO8m; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.221.48 as permitted sender) smtp.mailfrom=cerasuolodomenico@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695630980; 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=aeqAQlatwdDujYhPSrHB+qQb56NZMAb9DgwinqxqIEs=; b=djDO41Kpfjy/TuUaWqonmt6QpneRzU+R3sH3fAhDTh7r25EqCHq9fj71lCst8UZXOTA/3P 2MZUa5Fcm6I9qYj6M63g/F6IPOBC9ufUZQJQrI18BWtnBwoec2QxTQJd/ViMLwMGu6a7h1 ULKlL96Nf9pdNYKxSakyb604IQGX4CA= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BJ0xDO8m; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.221.48 as permitted sender) smtp.mailfrom=cerasuolodomenico@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695630980; a=rsa-sha256; cv=none; b=7RZ3OZZPa85MpClfwSfbqmTPEuNn0ziAymQrYdNV6ngQT6/6z3XRkHqQqmJFBtE1x6/2BL FYgNqrY4yHedLpK7fiA2hVAJvz/R/QSBDTcX7NPHu02Yn970U+XSF3FoLFT0wdVdCvgoC3 B9FwyLrsM7Sy9Sh+Tf1S+vWB6x/he3o= Received: by mail-wr1-f48.google.com with SMTP id ffacd0b85a97d-3231df68584so2080224f8f.1 for ; Mon, 25 Sep 2023 01:36:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695630979; x=1696235779; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=aeqAQlatwdDujYhPSrHB+qQb56NZMAb9DgwinqxqIEs=; b=BJ0xDO8mMl8woHr81omeuuF2ENN7J2dnfDoXS2dN0laz/ktTXZyFt50Yhp0q+DMXVk jZp8TBDdTzHAbTcc7m65Lw3u6WwzrCg6tErT/r1+CEotdIEpRs8PZVnDANW2JlJC6H19 Xq1X6xM65DESC34Qyi0dSS6OCE3ogEcPsE6IMCbFpmv5XyaiUBJKN0ljNqW0YqSLfSAe cfhW8M5pToYx6G2lFdhkuM+NMaDe3g9YTpKjMZRvn++VmLNAusoWJwHITaLNmuezvjN8 jJi6s9wF71+Lw22nM9P6LRcdGBK8I/Hxlk/31LJ2UNK7TPNs/KhG6oAykMeU5latplXM JOrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695630979; x=1696235779; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=aeqAQlatwdDujYhPSrHB+qQb56NZMAb9DgwinqxqIEs=; b=vlW+IdSM1JFu9hnM4MWuK6K9BYbBoo35dBcSDl91fnxzmq6YWVEIq+uAjTNqz7D480 0kAUH50WVvvH37n4LH86Fq5ul+Nc2pZYSlbELVPhZ+hdfXhba6cWNY1b9nzPctxZF9Tv ZP7WWWvuVilU/R21h7gmq3I8osgOaPdbXOo+w2Nv/xea5Ezyw/3UxLFDGDmAF/ak5f4k sQq4ugXwC6GjKs05he38Cdq3tuQKuZnAuF0UdaQdxqPwxErK7bUakdr6wiVCGa1uc+bQ MdIF+I6KuFX3CBYBq4CzhwJPuyNOtDq5q2dIdLAhDULZD+HujPe87gDWpAOjWAyMIwdJ WL2g== X-Gm-Message-State: AOJu0YwvyTtqdQoLnabldlCg0OZE9/BCnlBOTIwG3cOIbVMKWak+Baa8 AlBu93Gv3my7xjrmw9Ui6mfIvD4YGfAOowqh9kI= X-Google-Smtp-Source: AGHT+IGPt5Irg12cqRt6/lQ24N6F1G7U2LNqzKgwK5lhjGj0AbzqmGGQUuBEcXRDN6KwCV91ET3d6rijl2xGviDexbw= X-Received: by 2002:a5d:6682:0:b0:31f:9838:dfc4 with SMTP id l2-20020a5d6682000000b0031f9838dfc4mr5220361wru.33.1695630978949; Mon, 25 Sep 2023 01:36:18 -0700 (PDT) MIME-Version: 1.0 References: <20230922172211.1704917-1-cerasuolodomenico@gmail.com> <20230922174225.GF124289@cmpxchg.org> In-Reply-To: <20230922174225.GF124289@cmpxchg.org> From: Domenico Cerasuolo Date: Mon, 25 Sep 2023 10:36:06 +0200 Message-ID: Subject: Re: [PATCH] mm: zswap: fix potential memory corruption on duplicate store To: Johannes Weiner Cc: sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, akpm@linux-foundation.org, nphamcs@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: D77F7C000E X-Stat-Signature: jbmpk9cc1gjcrwd9dqiqypijp6u9xfnp X-HE-Tag: 1695630980-181552 X-HE-Meta: U2FsdGVkX1+18rV8hW38F+Bi+1YD8C3KONNarGFSmw6oq/R6ETuCNQoQ2SDHeM89Q1kW9zvbWQASxG6cq54diYli6Lde30gsvinCQfgCLZ6t2UDqFcPwsU/dLgNt3zn1+TkleZo3aj0FDDfmpl/8Ecni3I5njaVCz1ImPcGKW20tNnL3iyqBSgh0l1QgIuJQ5TJVxmxfP9kRDLOX/XFP6QacoZCEUnQJqlV4Qj7ZVmQXmKwSRCuVC7wdIaEI7G031CXecB0OdcQl1dnL24f3EHvEXqZDMsv8N+ytyMUcJhFiqn5I0ZPry1dQMKl2AeTHNoxbQXigCY0I8B2ZQa++Ux5D0qga3VV9X6aMEaERz29kj9kDdY7tJIvUDDV/QZY9+CKptm8k9t2jcRi7dnpnMrRMvJMuGsNI9kjO66lKu5gQahFpvycCyuh5gfrSn7URXB7CZw9SrnPPR8dfu02GmPKthWVAaXjfSEd7GuquzgJAIn8nPWDcTGGm967jXJBlQ/tQydDcdsKRhLJaj33uYDoAl1czApdGl0qTU9ilvW4434XUb4g6ox5xzAX/qh7x5CzfnBeJdy5UnJskIjb0zbOVHKVWtaqEfLpXQYwOVMqa13rVF1Sk0St9E9RfVr989q/3CoY7sOjIB+v/cO4MqZMm2Of+delGMowEksoOcgqc1PyJa3Xuf8f9NcmRBZMNnz0sQvu7SIC8+5HcroNwZmAl2h2uixjPIBWUwzkabEd1hawpu8vAQlqI7UYOkj2mywYrArJC3RGrvyXD/RY007YQ1BqSvMrzecEsfEaodmRy+lQC+FnpcaS60aMY/QilKVX7XxrEjZ+eVyE5hWP9n9G/Jigraix1KfdM4EArFOtv/qiOL/PPzj+jsEWXUUBzXLAWub05nuhg3D7N8fmqC2s7+wROL9I3VCwC6L2OqLL9jUeJ9XmRX1DtKvep7YS0J9eXJm6EUHTLw/mxC6+ xHGFoGTa dn1+iIznjb6+CE4YEXJRe34w1ryKQ737Mz2GLESd6YVwxZNmtiiM3KVYIPwdHL9+BBzhN6qB8JhYdzzwaiLgOUFiQB1SuX4LrFPsSgjvXIKPXToWs6NWHwEdEECa5UzWgIQOk5C5cM48pOSjZE87d6x2uUC+oBECmwR2EN9BCSLHZ195U+3BrzY4zC9nhmroKBNnlUnwd9WHBBRU/nOVCLdOlkhbmxL6CCi7C/5NblKyVBj6ooNug+GaKvo2HYgQqe0KhqU3R6GRvsqH3OMsMEOGt9EK8ldRQmSSxH/pIDH5bqI5v/6QeBuLbjEoyvy7winq1D31W4FNTjFGHYUsWrDnqnPjnEgM1hthmFbrrruiSJ8s8EKzQFAOrxffmRbEvnAKd2UTBfQ9Dd+bV81ctqJAn3csVq8P38GIYpVEGwn94wfTrMYpZo2G53A== 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 Fri, Sep 22, 2023 at 7:42=E2=80=AFPM Johannes Weiner wrote: > > On Fri, Sep 22, 2023 at 07:22:11PM +0200, Domenico Cerasuolo wrote: > > While stress-testing zswap a memory corruption was happening when writi= ng > > back pages. __frontswap_store used to check for duplicate entries befor= e > > attempting to store a page in zswap, this was because if the store fail= s > > the old entry isn't removed from the tree. This change removes duplicat= e > > entries in zswap_store before the actual attempt. > > > > Based on commit ce9ecca0238b ("Linux 6.6-rc2") > > > > Fixes: 42c06a0e8ebe ("mm: kill frontswap") > > Signed-off-by: Domenico Cerasuolo > > Acked-by: Johannes Weiner > > > @@ -1218,6 +1218,19 @@ bool zswap_store(struct folio *folio) > > if (!zswap_enabled || !tree) > > return false; > > > > + /* > > + * 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); > > + dupentry =3D zswap_rb_search(&tree->rbroot, offset); > > + if (dupentry) { > > + zswap_duplicate_entry++; > > + zswap_invalidate_entry(tree, dupentry); > > + } > > + spin_unlock(&tree->lock); > > Do we still need the dupe handling at the end of the function then? > > The dupe store happens because a page that's already in swapcache has > changed and we're trying to swap_writepage() it again with new data. > > But the page is locked at this point, pinning the swap entry. So even > after the tree lock is dropped I don't see how *another* store to the > tree at this offset could occur while we're compressing. My reasoning here was that frontswap used to invalidate a dupe right before calling store(), so I thought that the check at the end of zswap_store() wa= s actually needed. Since we acquire the tree lock anyway to add the new entry to the LRU, woul= dn't checking the result of zswap_rb_insert be a very cheap sanity check? We cou= ld treat it as an error and fail the store(), or just add a warning and still invalidate the dupe?