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 29D82EB64D8 for ; Thu, 22 Jun 2023 06:40:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 335348D0002; Thu, 22 Jun 2023 02:40:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E52B8D0001; Thu, 22 Jun 2023 02:40:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1858B8D0002; Thu, 22 Jun 2023 02:40:07 -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 095598D0001 for ; Thu, 22 Jun 2023 02:40:07 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id CAEA4809E1 for ; Thu, 22 Jun 2023 06:40:06 +0000 (UTC) X-FDA: 80929434012.29.ED19FDE Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by imf09.hostedemail.com (Postfix) with ESMTP id DF412140002 for ; Thu, 22 Jun 2023 06:40:04 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=WzcupkLR; spf=pass (imf09.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687416005; 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=vv9UsVgg9fbHRyw9vFcVO/IAcQzpHnhUOw/udmG8S1I=; b=JHBqSR5PBcGF3ylUyY46DXzqKcxZQqvONs1R1c9vGMES5+q83JhM6ddPXDNrOwYZT8MP2N x7prT6LLKpHVV7XlnC+Uf/0PXRIjIBPnqRNy35oLvA/t9jRrN1uVvDw1jmVEYIiMvYhudg LcnwP9hIGe8Q9yRPtro5rAutJydd54g= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687416005; a=rsa-sha256; cv=none; b=y50JfciYpj6yXWFbwo4RHoInqlwNojPXUvRlSj3Gkj4RpakUYElXQdvb4JnT6Dwd3PrC4S zqyjuGmClM+I7rmq+Jk+Y0BQ/ziONJOSayqbJNxFai6fVrSvca0nBkis8FbOEhsKey7cO2 Zh9GInfYw330VJVCKyq6dWwxDViZmco= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=WzcupkLR; spf=pass (imf09.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-51be527628fso841666a12.3 for ; Wed, 21 Jun 2023 23:40:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1687416003; x=1690008003; 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=vv9UsVgg9fbHRyw9vFcVO/IAcQzpHnhUOw/udmG8S1I=; b=WzcupkLR0sjm51+HzOWS6BXCJPzEFSg4ccXNy7pbRGZ4tEyrK2y/rXJ+WqGdBLruV+ /9aP20WeiSLKo7Nqt5EZk4BPdEKNvwZpELwQ59U9ox6TcbtXauNlAcCXEKoiOh8g1DfX ey+kHgNXpVNrxdXLAy4QHPWVLAnqCHM2BWp36xqozuWnE8I3I/MV330ND2E/aID5To4M xR7s5FGEjvCJv0rTsrGS4A5jhQGFyjwXlkW4W0yX2eJJGaV8CjPXy2mTTVfie6Y6P8ig K+AewzaYp816VWuNo30aTqYdJucnftX0LZuQyDwRIxLk8Dplt5CNc61YSC0R785fmQpN +1nQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687416003; x=1690008003; 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=vv9UsVgg9fbHRyw9vFcVO/IAcQzpHnhUOw/udmG8S1I=; b=VSde+VjLEkJr6UGSVYxAHvqgJwPtpszpOaj0wbJiiQKzZS/4+zgplv2E6TdqLXVtxG ySYcLarDG0C99le84t7ZGkT44e1Wh7qLihE//OQiOHoVb2Vfa/qhZDBiWORjTQ2zvFv0 WY3+/zlFqpnCfShfLihgQlUV6zyS5+VTQ5OkEG+DXOw8AsG70g2+kUJU70VAYK9xbWl9 OQ5F3xCYySKy5yOTB8mVR53F4prM2PRwjVRrbIkW6wLIu+7BIQQsVcezxC+0JaF66WA5 RNCBx/+n2QHOUn30asPa1pI9QN1zdJiJfniboeUYClsdqiuE+rrhggDPP4+VgIWTjwgi 8g8w== X-Gm-Message-State: AC+VfDzFVevKfTRb5nlY1iwOzCw1R0cZpeLkiobrTjdna4rhQgU7jDQc LmPC3s2RSRCijtsMkuF0exMe8tr1ROX6d6KBlN4nrQ== X-Google-Smtp-Source: ACHHUZ4xg4cv3atPrm/9EuRzjTjrcAhKnPRpZUSLrlRWkpNRLglzg2KFmcuUWRc6Pol/RKICA8o0nof4PRjR4ktKRiQ= X-Received: by 2002:a17:906:7ce:b0:98d:4000:1bf9 with SMTP id m14-20020a17090607ce00b0098d40001bf9mr289161ejc.65.1687416003149; Wed, 21 Jun 2023 23:40:03 -0700 (PDT) MIME-Version: 1.0 References: <20230621093009.637544-1-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 21 Jun 2023 23:39:26 -0700 Message-ID: Subject: Re: [PATCH] mm: zswap: fix double invalidate with exclusive loads To: Domenico Cerasuolo 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-Stat-Signature: honni17m54jyuiwidkobu48iypaotfn1 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: DF412140002 X-Rspam-User: X-HE-Tag: 1687416004-219673 X-HE-Meta: U2FsdGVkX1/IOcfvFCiVRZlMnICalwqUMDCdqyw6uQqjcZF086codkGnOrjtuM+uIW81AHpnj99qdM/ec6D19H2Xe6gA0nVsD+3CqWGHELrIplQdzrHm3BAsqu8TuRb59j48ABvMhge54Zf1x+hM9K7Y6EeKjMv+h4exzb7oU29YuEaRGSAavvvj20Uk8da1I0GGXbYB1o6fs4Q2UEyfIs+s0lGdpqErU89z99khttA5rP1ke+eD5dtpZ0my0R7MbyX4QETLKNj7dI9QcQFc43UHIbK92BBb0BHRcmOV9DlYSNaqqonjDNmuqZmSDlTZkX2ZaLvINaMeYQHMkK8A6SplRHLQVXikWJBJe766Q+rXKmRKRT9CFVi9giPMAo6TPlIO5cRb0UaYxzX+PvzUjyM73eq+ffHv/4Ft41i4jlE2+Cr5yZSuDwtPpt3euTBbp7EdIAIG5sCfofn3vqd2ISX503Ag4u2B87BdKa23PE83nPsFQxdh+O5LqbKhEOD2szzpK0sTb5KLB4PinUDApR5f6vU32mVHRoJsJCKjRoCLks5oHLG41yTqCdTRMQhEyqdI4TCzqIirOvmPX92phlIi6sx4M0oRmKP9m291+uEwValxvRt3UA0d/S9+PleNZnXHjWicmKnDQjpAVtDugbWDkryieBrR/kU+blRS6OkzlhAAfxSm75vQuT0V7MXIPOwfuiPh5K+lXr5NcsyFiu7nBZv4OSfpnQWz/8i0W8jC6zHGIibtO7Hmyb8ri17/l3lNbEp+acCef9ISQfVi6UEKXWBx/DNTkiKPgmFh1DB/dayrHhgSodxQyuFpbgk2dqWlOygS266DurWL5MTbQWa8Hk3aSas28QX2ongkuo1FzDCHSE4Wibyn0f5K/d7D/f+aKK0SH3QS+PyqhAJpbe9AFDZh/NK7yQ6u2Zk2RaMk2gx3khpiY+SzC/+uHRn5kQz/9tmgvJpALHTwzjx tKZLKkZM O0mDMGwm8Pnae4DSfinMnUuEdHfjAfDdQn0UoVF/b5KbZV2/4q6YfUUgSWt49vKP+iDs1Y1VvGdjD6IPqTvEwdqSn8pK4p3fLIbZMgmmSlUInDBPNLcPYfiQQRHjF0NYfMPoWdsBiJpXJW9maMxPz5omUlIT9zQZZVj2WXNA6LCjYhoYIv5Ldy6cV1DOEW3h9YgUEszBiDmJVySJypqnKYcIubXPxDvD0LCvfMA5UgJ+hgopXXCliCzE1yTKt2/hYJOTOMyz+Tl6g2jlyvQ+UBYhdav5hcSbwDTNPYrYKwn/sp95qngACuOkzysCdXH2wRw03sgF7NFjoCQIcj0zHL+fUPNn2F6D5ZMSS 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:33=E2=80=AFPM Domenico Cerasuolo wrote: > > On Wed, Jun 21, 2023 at 11:23=E2=80=AFPM Yosry Ahmed wrote: > > > > On Wed, Jun 21, 2023 at 12:36=E2=80=AFPM Domenico Cerasuolo > > wrote: > > > > > > On Wed, Jun 21, 2023 at 7:26=E2=80=AFPM Yosry Ahmed wrote: > > > > > > > > On Wed, Jun 21, 2023 at 3:20=E2=80=AFAM Domenico Cerasuolo > > > > wrote: > > > > > > > > > > On Wed, Jun 21, 2023 at 11:30=E2=80=AFAM Yosry Ahmed wrote: > > > > > > > > > > > > If exclusive loads are enabled for zswap, we invalidate the ent= ry before > > > > > > returning from zswap_frontswap_load(), after dropping the local > > > > > > reference. However, the tree lock is dropped during decompressi= on after > > > > > > the local reference is acquired, so the entry could be invalida= ted > > > > > > before we drop the local ref. If this happens, the entry is fre= ed once > > > > > > we drop the local ref, and zswap_invalidate_entry() tries to in= validate > > > > > > 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 dropp= ed. > > > > > > > > > > > > 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_e= ntry() > > > > > > 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, struct zswap_entry *entry, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -static void zswap_rb_erase(struct rb_root *root, struct zswap_= entry *entry) > > > > > > +static bool zswap_rb_erase(struct rb_root *root, struct zswap_= entry *entry) > > > > > > { > > > > > > 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 r= ef and remove it > > > > > > + * from the tree. This function must be called with an additio= nal ref held, > > > > > > + * 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 p= utting > > > > > the initial ref, there's no need for an additional local one. > > > > > > > > I believe it is, but please correct me if I am wrong. Consider the > > > > following scenario: > > > > > > > > // Initially refcount is at 1 > > > > > > > > CPU#1: CPU#2: > > > > spin_lock(tree_lock) > > > > zswap_entry_get() // 2 refs > > > > spin_unlock(tree_lock) > > > > spin_lock(tree_lock) > > > > zswap_invalidate_entry(= ) // 1 ref > > > > spin_unlock(tree_lock) > > > > zswap_entry_put() // 0 refs > > > > zswap_invalidate_entry() // problem > > > > > > > > That last zswap_invalidate_entry() call in CPU#1 is problematic. Th= e > > > > entry would have already been freed. If we check that the entry is = on > > > > the tree by checking RB_EMPTY_NODE(&entry->rbnode), then we are > > > > reading already freed and potentially re-used memory. > > > > > > > > We would need to search the tree to make sure the same entry still > > > > exists in the tree (aka what zswap_reclaim_entry() currently does). > > > > This is not ideal in the fault path to have to do the lookups twice= . > > > > > > Thanks for the clarification, it is indeed needed in that case. I was= just > > > wondering if the wording of the comment is exact, in that before call= ing > > > zswap_invalidate_entry one has to ensure that the entry has not been = freed, not > > > specifically by holding an additional reference, if a lookup can serv= e the same > > > purpose. > > > > > > I am wondering if the scenario below is possible, in which case a > > lookup would not be enough. Let me try to clarify. Let's assume in > > zswap_reclaim_entry() we drop the local ref early (before we > > invalidate the entry), and rely on the lookup to ensure that the entry > > was not freed: > > > > - On CPU#1, in zswap_reclaim_entry() we release the lock during IO. > > Let's assume we drop the local ref here and rely on the lookup to make > > sure the zswap entry wasn't freed. > > - On CPU#2, invalidates the swap entry. The zswap entry is freed > > (returned to the slab allocator). > > - On CPU#2, we try to reclaim another page, and allocates the same > > swap slot (same type and offset). > > - On CPU#2, a zswap entry is allocated, and the slab allocator happens > > to hand us the same zswap_entry we just freed. > > - On CPU#1, after IO is done, we lookup the tree to make sure that the > > zswap entry was not freed. We find the same zswap entry (same address) > > at the same offset, so we assume it was not freed. > > - On CPU#1, we invalidate the zswap entry that was actually used by CPU= #2. > > > > I am not entirely sure if this is possible, perhaps locking in the > > swap layer will prevent the swap entry reuse, but it seems like > > relying on the lookup can be fragile, and we should rely on the local > > ref instead to reliably prevent freeing/reuse of the zswap entry. > > > > Please correct me if I missed something. > > I think it is, we definitely need an additional reference to pin down the= entry. > Sorry if I was being pedantic, my original doubt was only about the wordi= ng of > the comment, where it says that an additional reference must be held. I w= as > wondering if it was strictly needed, and now I see that it is :) Not at all! Questions and comments are always welcome (and encouraged), at least for me :) > > > > > > > > > > > > > > > > > Also, in zswap_reclaim_entry(), would it be possible if we call > > > > zswap_invalidate_entry() after we drop the local ref that the swap > > > > entry has been reused for a different page? I didn't look closely, = but > > > > if yes, then the slab allocator may have repurposed the zswap_entry > > > > and we may find the entry in the tree for the same offset, even tho= ugh > > > > it is referring to a different page now. This sounds practically > > > > unlikely but perhaps theoretically possible. > > > > > > I'm not sure I understood the scenario, in zswap_reclaim_entry we kee= p a local > > > reference until the end in order to avoid a free. > > > > > > Right, I was just trying to reason about what might happen if we call > > zswap_invalidate_entry() after dropping the local ref, as I mentioned > > above. > > > > > > > > > > > > > > > > > > I think it's more reliable to call zswap_invalidate_entry() on an > > > > entry that we know is valid before dropping the local ref. Especial= ly > > > > that it's easy to do today by just moving a few lines around. > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 *pool) > > > > > > * swapcache. Drop the entry from zswap - unless invali= date already > > > > > > * took it out while we had the tree->lock released for= IO. > > > > > > */ > > > > > > - if (entry =3D=3D zswap_rb_search(&tree->rbroot, swpoffs= et)) > > > > > > - 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, pgoff_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, pgoff_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 > > > > > > > > Reviewed-by: Domenico Cerasuolo Thanks!