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 262DDEB64D7 for ; Wed, 21 Jun 2023 10:20:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 80C428D0003; Wed, 21 Jun 2023 06:20:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7BD518D0001; Wed, 21 Jun 2023 06:20:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 683E18D0003; Wed, 21 Jun 2023 06:20:30 -0400 (EDT) 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 557C58D0001 for ; Wed, 21 Jun 2023 06:20:30 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 1C85C1A0905 for ; Wed, 21 Jun 2023 10:20:30 +0000 (UTC) X-FDA: 80926360620.23.CF6EE7D Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by imf05.hostedemail.com (Postfix) with ESMTP id 4F5E2100015 for ; Wed, 21 Jun 2023 10:20:28 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="VF/f0gKy"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.216.43 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=1687342828; 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=OcQaMP8s+AxLjoBlCFXTD9bsymZvwNu0P0OUlN7TZjI=; b=dHR3povBjFU0kmRyXwaLcl0UUw8t37qFQEVLRnylFdZmQTG0qIVYzgN5yEVfDNqu+juPPc 6alZZ5/02W42c5/8SdhcS40CMkRDUr/nBBF+DfEnPVHv0ykT/CY5a4njXTWofgugG9Q0Yf cEZ/FU4Q+x5PyBR5Z2TlVJ+GnYQgXVk= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="VF/f0gKy"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.216.43 as permitted sender) smtp.mailfrom=cerasuolodomenico@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687342828; a=rsa-sha256; cv=none; b=MygvQKqeOd8gb/iYRQudClOElBRNJv1Fkct9Fc1sJFC/gLEOktDZIqDe/pgMtzBzzcHIPT OIs7RkHqIRM8NFJvdPRdM0oLE4nETf6GVUx1RDR9AhhDoc0wRAzJNzMZ7eZwHFSpz6+2ly l4Yy7slmEtbG5/I+OXQ5+411HcPNjQs= Received: by mail-pj1-f43.google.com with SMTP id 98e67ed59e1d1-25e92536fb6so2742259a91.1 for ; Wed, 21 Jun 2023 03:20:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687342827; x=1689934827; 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=OcQaMP8s+AxLjoBlCFXTD9bsymZvwNu0P0OUlN7TZjI=; b=VF/f0gKy6Cfk+X4TK5U08DAgZE3/xdFeSomUIfc9CwoE/cEl+FT9Va0B24fqYMBcE+ g0cS78O6pjUi7FYI76L7hWriU87noXmwP6CyU9ZtlkGPdSz7qDYC3+s6XfwX2yfmZ/Zg Dx7y+M6I8tTe/sPZeyV8ZitG0vPDTvHqEgX1DncAREBAGhsFGZEc3JyYyBCK2uAukLAc WYkwsH9yYH+jkeqsKIA8Vda8VhHpRZLoUlfuSKKP5Do2DXhTCW6uzXK0ArCrMhCL7fCk BewWxWV/Ed1l2UlFbWui71WfgWCrQ9mIcDn4mbmP6o4nXhL2+bT1yfKnUGuSvR93sbrB 0k4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687342827; x=1689934827; 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=OcQaMP8s+AxLjoBlCFXTD9bsymZvwNu0P0OUlN7TZjI=; b=LU/+lTpwpZ50VOAfiZ24YA3h/TRhLcJdmlxE9RPrPyMIBqsXxfAO896OcdpIE8Hz+P MyooMDREvDu0a39EGXXpOsNZWMsmVDoNU4qZmACiCl+kwZAX1Z6gEFFXdtTbM69ADPCK gqab064TB5NscvMrLCbZda7DeSyCAEMWD7QujLDMWmG7qG7T2ECZTn2+j0HyIBstKR9F eeRcTlfmGOp8f8lI6sg4hBZLcAtCdK6kX1RcszMtDPmrXaVeuFZ+dShmts6P7jZdBAWk CHYX9valEruE2VuF01Hupjg7OxgLP0VG5igmFSruTZFEeceR/EEDwLaWbQ7rv6cAhcp4 W2/A== X-Gm-Message-State: AC+VfDy5wNEEJ3JSXyxdo+UQKpRbm2arCjbNvPAX1I9pxyTN++evY7Et lDaCRt6GTTvFzvdXuSfjC6lYrSp/ei0CRZXbsD8= X-Google-Smtp-Source: ACHHUZ5Qeg6rt461AK5fIaW73RYdd2qgVi2eyCOOkTm9Vl9NrU2nWM8G+wwqheNzZkJfb8UkCA5IIHINX1hGHAqb8p8= X-Received: by 2002:a17:90a:7641:b0:25b:ca75:8f44 with SMTP id s1-20020a17090a764100b0025bca758f44mr9072244pjl.4.1687342826817; Wed, 21 Jun 2023 03:20:26 -0700 (PDT) MIME-Version: 1.0 References: <20230621093009.637544-1-yosryahmed@google.com> In-Reply-To: <20230621093009.637544-1-yosryahmed@google.com> From: Domenico Cerasuolo Date: Wed, 21 Jun 2023 12:20:15 +0200 Message-ID: Subject: Re: [PATCH] mm: zswap: fix double invalidate with exclusive loads To: Yosry Ahmed Cc: Andrew Morton , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Konrad Rzeszutek Wilk , Seth Jennings , Dan Streetman , Vitaly Wool , Johannes Weiner , Nhat Pham , Yu Zhao , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 4F5E2100015 X-Stat-Signature: jbxdqg4cfqpd5zqqj7ngnaqrejftk363 X-Rspam-User: X-HE-Tag: 1687342828-126176 X-HE-Meta: U2FsdGVkX186dsfk9Ck0Qqqj67ckliMtmau5zoqt989GDvnF87rpShDPPj4Qlvt10ZIkbo3Z5vLe7TlTSoaFOUm6HW/4es3i6movI408xRl9ous+rgSU9o3NT2suufDAcWSulsW1vB8idN2XunZHbm9Z6rDAa02u2n5ZZtCzqqDWJqP9mtM0+pDRVlpU9YIYMOU+LLmY3naSyztJWYLOPEyTAM95YgxcA8bxeAuG/0p9CgUxjdGZFVumnZ+3UMuCpMhBJ0TZvSj8Pw+PTkC9gUkJyQKvtcIGxpG99DhsjjMM6+C04qamRSH6PkL4KaJ06uZpEQJ/NBZUjWxzJwuk3+IX0gUycuckPg0EiII+X2CIcj2oHJ+gqiW4WgKZ/Q2sTnbGAzEiN04DETxYyfphXAcZBrSKaGg3fz4vO7THHyP2ei3G3zKABtVHalWwZYs8mIqrHf0oE/DqJD2BcOn1pLEmn89jCHYJ5ebUTA/prWHTy8S0JGb7wPqPbIcMtS5JLot6+AGjEGmRKt4aA3BrsQDU/O8Ho8ZBz7nPhnCDIpkIFdNYmUkoBNaOLTbLm/rM452iwgGogTxa4z5ZuVGdj/x0M2OEV110vSgBjYwd0PF9wAEsj/FQ9siryKuybU2h5OmS/tpOnwXBhQ7sB6KTahX776G9FcsMTwzq/fJrRTObB0XEyPWvkFU+CDrgEPwmvSh+aj24TLq3Hk/io4zp8X6Gx86Oy/Bex31FSTtQW+obrHX7f3VIR1y33C0dY0YDWF8FmiJoVdD38nLuGR/HaqoD9VJdsbNMh+mP55IGcADwMyE3nzjYbBuJmDvEHg7CGmewIxs7bOusTWMTkHfMgAekclLePOT496jvZC5YQG/8RJqk3NmaWMdrGKETqXX8XwuFFqHqoNatY+vfJxj5ScXER92Oe07B4JOe5TICNmRrFO6U0xO0nJn46sdbPmY3Ep3B/C1M8xbZpIJx9sT UvJkw9cF kvvIdeXizJfkoOc9C/EG0T6VCPAhMBO60bxhCznnVFYuUyhX8AlnX+ldDncr39UZtWiUqNS8xwO5GcTo8jcsCvVlZyct+32SAsnMRMOVPsPH5Mqys/REx5JbLEuFUGn4VPojLn5j+u0XsgvAfFw+3AZXYYoXCoGLp5Ct+T3x6Ecuv9DFNlFZDYB16gggul0xdfK8AqaFJj3dDF72gdIaAR2gfxM2yhLlnGOYcHDf6GCvcsErXSTq9BlEl+xEOk3B7cn5zC/rC/eils3BofKxexvIz5G6NJrfytjqn0ssUgcJ2fQmVlGa8al/xi8pE+dJX/xa32v0qI7rEGiSPQnord3F3yf8H8949DB6nLSVQBaZ3SKOZ8leDnBdNBQ== 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 Wed, Jun 21, 2023 at 11:30=E2=80=AFAM Yosry Ahmed wrote: > > If exclusive loads are enabled for zswap, we invalidate the entry before > returning from zswap_frontswap_load(), after dropping the local > reference. However, the tree lock is dropped during decompression after > the local reference is acquired, so the entry could be invalidated > before we drop the local ref. If this happens, the entry is freed once > we drop the local ref, and zswap_invalidate_entry() tries to invalidate > an already freed entry. > > Fix this by: > (a) Making sure zswap_invalidate_entry() is always called with a local > ref held, to avoid being called on a freed entry. > (b) Making sure zswap_invalidate_entry() only drops the ref if the entry > was actually on the rbtree. Otherwise, another invalidation could > have already happened, and the initial ref is already dropped. > > With these changes, there is no need to check that there is no need to > make sure the entry still exists in the tree in zswap_reclaim_entry() > before invalidating it, as zswap_reclaim_entry() will make this check > internally. > > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads") > Reported-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > Signed-off-by: Yosry Ahmed > --- > mm/zswap.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 87b204233115..62195f72bf56 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -355,12 +355,14 @@ static int zswap_rb_insert(struct rb_root *root, st= ruct zswap_entry *entry, > return 0; > } > > -static void zswap_rb_erase(struct rb_root *root, struct zswap_entry *ent= ry) > +static bool zswap_rb_erase(struct rb_root *root, struct zswap_entry *ent= ry) > { > if (!RB_EMPTY_NODE(&entry->rbnode)) { > rb_erase(&entry->rbnode, root); > RB_CLEAR_NODE(&entry->rbnode); > + return true; > } > + return false; > } > > /* > @@ -599,14 +601,16 @@ static struct zswap_pool *zswap_pool_find_get(char = *type, char *compressor) > return NULL; > } > > +/* > + * If the entry is still valid in the tree, drop the initial ref and rem= ove it > + * from the tree. This function must be called with an additional ref he= ld, > + * otherwise it may race with another invalidation freeing the entry. > + */ On re-reading this comment there's one thing I'm not sure I get, do we really need to hold an additional local ref to call this? As far as I understood, once we check that the entry was in the tree before putting the initial ref, there's no need for an additional local one. > static void zswap_invalidate_entry(struct zswap_tree *tree, > struct zswap_entry *entry) > { > - /* remove from rbtree */ > - zswap_rb_erase(&tree->rbroot, entry); > - > - /* drop the initial reference from entry creation */ > - zswap_entry_put(tree, entry); > + if (zswap_rb_erase(&tree->rbroot, entry)) > + zswap_entry_put(tree, entry); > } > > static int zswap_reclaim_entry(struct zswap_pool *pool) > @@ -659,8 +663,7 @@ static int zswap_reclaim_entry(struct zswap_pool *poo= l) > * swapcache. Drop the entry from zswap - unless invalidate alrea= dy > * took it out while we had the tree->lock released for IO. > */ > - if (entry =3D=3D zswap_rb_search(&tree->rbroot, swpoffset)) > - zswap_invalidate_entry(tree, entry); > + zswap_invalidate_entry(tree, entry); > > put_unlock: > /* Drop local reference */ > @@ -1466,7 +1469,6 @@ static int zswap_frontswap_load(unsigned type, pgof= f_t offset, > count_objcg_event(entry->objcg, ZSWPIN); > freeentry: > spin_lock(&tree->lock); > - zswap_entry_put(tree, entry); > if (!ret && zswap_exclusive_loads_enabled) { > zswap_invalidate_entry(tree, entry); > *exclusive =3D true; > @@ -1475,6 +1477,7 @@ static int zswap_frontswap_load(unsigned type, pgof= f_t offset, > list_move(&entry->lru, &entry->pool->lru); > spin_unlock(&entry->pool->lru_lock); > } > + zswap_entry_put(tree, entry); > spin_unlock(&tree->lock); > > return ret; > -- > 2.41.0.162.gfafddb0af9-goog >