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 98A86D41C00 for ; Wed, 13 Nov 2024 05:35:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D7C6A8D0005; Wed, 13 Nov 2024 00:35:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D2C278D0003; Wed, 13 Nov 2024 00:35:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BF4BE8D0005; Wed, 13 Nov 2024 00:35:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 9ED888D0003 for ; Wed, 13 Nov 2024 00:35:25 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 0851C140822 for ; Wed, 13 Nov 2024 05:35:25 +0000 (UTC) X-FDA: 82779957624.21.863F7F3 Received: from mail-qv1-f50.google.com (mail-qv1-f50.google.com [209.85.219.50]) by imf25.hostedemail.com (Postfix) with ESMTP id 74376A000B for ; Wed, 13 Nov 2024 05:34:52 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=y68qjJ3d; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.50 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731475993; a=rsa-sha256; cv=none; b=5lcTohGqsDuHNDKLYCFr2JRzji3f7YcKRqvCqOheL/lcKpG3sIutww2J9Dj2bBmSKMdakk gEKnhpkJYwJR6+QsjxQCIkBEZPqld9Ym21aIPUholdnuOEyynHGEXOuqaJQp5PukUm62Hc 4zmIvCGbXyVr1CFzDUh0pT22ZyuEpFI= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=y68qjJ3d; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.50 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731475993; 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=XyPT0/YbDuXqIj/AzCTZh1WZxLR8mJzfYf8jckGMcP8=; b=BdiUJ5D58TXKKBAbO5fpdL1PK/VVBRMqZmrDXHguzLJhZ2PwbmIVIXmoUOjb1jjGc1a/T8 0c8Z7jogcyTeErx+hrCMSeiDwb8offZa8qXGVrl17X1M6YVRTUVQCBoO/S2ybKVt1yyF8V JMMRQNAbBUTHGBQq4ywwLRdEImA47lE= Received: by mail-qv1-f50.google.com with SMTP id 6a1803df08f44-6d396a6f6aaso47600006d6.2 for ; Tue, 12 Nov 2024 21:35:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1731476122; x=1732080922; 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=XyPT0/YbDuXqIj/AzCTZh1WZxLR8mJzfYf8jckGMcP8=; b=y68qjJ3dfy/xXCsCPSUV+SlDB6EI+FIqE3+5AdGg01kKuMwoQcACeI2UxFmptuIHcc IdJA3bDkqgFnwzRMTyqJyFZa7tvoS1jiKsfG4F2yT62NncMJ+5CMj1LR5E32XETmtG3a NIqWMTwnZV483GDpslbNxRZzW4CX40Re3j8MAakp06sr0n+2CIbM16uBFytCzNO5C/Ul Ca+bZsoqkHtiDaXrqKOyumMIXTM542SzJbfHX//6nZTeBX7Ed5HktGFhN0L1McccTyvv 5yj4Qjo1gie+Xcelgnjnw72sc+6ZNHaBIH8SeWHaqBnH+/ld1H29y3etvs0OZ5HN7DHh MWFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731476122; x=1732080922; 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=XyPT0/YbDuXqIj/AzCTZh1WZxLR8mJzfYf8jckGMcP8=; b=PVKR++32y0qvo9170R0xwBGjBgCsZ1EM7WrI5NkI2K0mvPwc6QOLWqatfmhVmmKRjA v51d1IPtmoA3BJ/Ir8SKb3brcfNTwLGAtUySVY5qMxXX5lLdf9iGvTr0u9f9WllpMaR0 fcWCabOnNwirLMnLRS2/5DVPPO2pE3E4yBdslVU281YpRhY5h1ajItPFBsB7IVfXp93w nsL0npiuNBhhGLrU9rF5G17vMnXKc1vxlFGhgr2KlhGTGfGACwi+3YPJ4LKicvyChk/F GX0R3e6HwDz+rIa0L8NzmLZoq2SicFYo5BeF73oTnKClTx6tanRjOkUtxnw1IOyios4L +ZdQ== X-Forwarded-Encrypted: i=1; AJvYcCXIvIRZeXJYrHkmwRcuGAPmWddEzuFVvJbow6gB4UDK5CU34yPvBRSdUc3l4OA1SPD5+4yAzr/OOg==@kvack.org X-Gm-Message-State: AOJu0YyQVygFHm1noenw/bwlrvYypP7MbOAscZne+6M1kUWShAXf6ICw MAvfbabboRhyrc14+a378S4od5ftfj1rvMSOnc/h7i7McpXdL4TXtuyxlxITJof3j/zdghNRgnx 6OU+5r+j6zStXLviBj5sOB3EhqlMPCAkWJzdw X-Google-Smtp-Source: AGHT+IGhDDWZiVRqU950S4Da0ArH/jG0Mi1RbYzsgL032N+wWEohldfNeDLZLRcsR9GRL+0QGbjgoCmvn4JroJ1AVBo= X-Received: by 2002:a05:6214:4291:b0:6d1:87be:96d9 with SMTP id 6a1803df08f44-6d39e1bae22mr261120866d6.46.1731476121984; Tue, 12 Nov 2024 21:35:21 -0800 (PST) MIME-Version: 1.0 References: <20241113052413.157039-1-kanchana.p.sridhar@intel.com> In-Reply-To: <20241113052413.157039-1-kanchana.p.sridhar@intel.com> From: Yosry Ahmed Date: Tue, 12 Nov 2024 21:34:45 -0800 Message-ID: Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress(). To: Kanchana P Sridhar Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, hannes@cmpxchg.org, nphamcs@gmail.com, chengming.zhou@linux.dev, usamaarif642@gmail.com, ryan.roberts@arm.com, ying.huang@intel.com, 21cnbao@gmail.com, akpm@linux-foundation.org, wajdi.k.feghali@intel.com, vinodh.gopal@intel.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 74376A000B X-Stat-Signature: tz559319jiqpt15izgjtk6d5e54xkzn7 X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1731476092-864215 X-HE-Meta: U2FsdGVkX1/IIIfB1KUS/IF1rTUTngmecZek44T/8DDy15NhG3V/YS3W3wHNnvFX6EJfkLBUEEtSyjwSi3aUzd0XI5iBQjG9rJL6YZHXwgBJZwhmE1dnz+v7JFvyU7VFV3ZtiE0l4p9jEa9bylSV5pHq6+7OUrTgLRKzw16CD9Pa6U72ydEw9fzk2iKdrd/eVQUwAGBbvEgys/shdzYkS8aAssycjiTvOibXdbGFEMSatWKeuo84HjmvEpb2C5edSNlBX7ZNFqWuca/IotmdqQ70YLU/OdfjYlcwM67HtvFXZ32o26Kzj1Vol6vn+RuZtFFDdQMtD9paSB/5aQ0kTvzEos5qdlly8xkmcxs2YbqrIy2WYxeCYj3B2036xsBGdl/5xLWpv7x6FaTM1N1oP3tXTcFaCBz/AIKcFXKb2YDAEbH+nAyRwE6JdrHrZnekcyujcqBaKU76p/W2R3+SiFUE5miCLlCTRUl1REL1Y33TA0X94Bo9g8HPzDcYCzft/cMxyuTeG/3cD688n/wEIBmCqbb2LwskP9+8Coanuip/DsYVOiKVvPEhdy/LUkZ3fWU2fNc1zdjHlr+fbvQ1jhsOhYp5TkFHFXhXeR2esOv4mMZ5R1xRazs4Eckh49qYJ0Vg9VQ58zyuh7JNq/FaOw/rHQuJeJcxhjXvFTGXyqy7iTrKu9w2OSSMiOR7vuUg96WmxltOgkG9vaZ5nLq4TAxUQGUq2quBwlyvGbk4Wc1cObBRgncQuG6K791QxL3yMTvKN4yxk2lM+Gxupw5GGSmrja5kMrID2L8cNrtqSNdDEZAJHUJyNavnjIuKosop+ai0Y/XqrbOF33KKTnTIUzFNNUc1ybWOXtW2+0kdMaxzQE5OnViGeq+WsMoZIALBqnm8iWE/WBVJY9BX76IBel/tM+dHjkIhCR6/mXz+2QR1Ymqgws9UCaQvAv59cX2e3yF7TETWaMe5KE0kR7d 4FfQnHzA mfSupqTVi6/Wk2c2iSL4MHLggXG/VJ3YvXrh4/XqoSvjpTLmVl0YrW4ygCftOgPkm5P0P7U4n7KMPgGNKmpwosFc7eH3i2HsUZdNMB85H4aR2uqUOir2tiwU35rg4bmghvROFJl5OmwUk56KUJLmXG6g0TCHJ7g51MC5b27vTe5SXxmMoDM07RIIuhoQcDtn0yQZiCJrNp+mGMYOVSGv1zQ2ykiYf1cVD8op2Y2OdBL35fbXPkUomUVf47NYydkGDt8V8f9RWzsMIkMY= 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, Nov 12, 2024 at 9:24=E2=80=AFPM Kanchana P Sridhar wrote: > > This is a hotfix for a potential zpool memory leak that could result in > the existing zswap_decompress(): > > mutex_unlock(&acomp_ctx->mutex); > > if (src !=3D acomp_ctx->buffer) > zpool_unmap_handle(zpool, entry->handle); > > Releasing the lock before the conditional does not protect the integrity = of > "src", which is set earlier under the acomp_ctx mutex lock. This poses a > risk for the conditional behaving as intended, and consequently not > unmapping the zpool handle, which could cause a zswap zpool memory leak. > > This patch moves the mutex_unlock() to occur after the conditional and > subsequent zpool_unmap_handle(). This ensures that the value of "src" > obtained earlier, with the mutex locked, does not change. The commit log is too complicated and incorrect. It is talking about the stability of 'src', but that's a local variable on the stack anyway. It doesn't need protection. The problem is 'acomp_ctx->buffer' being reused and changed after the mutex is released. Leading to the check not being reliable. Please simplify this. > > Even though an actual memory leak was not observed, this fix seems like a > cleaner implementation. > > Signed-off-by: Kanchana P Sridhar > Fixes: 9c500835f279 ("mm: zswap: fix kernel BUG in sg_init_one") > --- > mm/zswap.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index f6316b66fb23..58810fa8ff23 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -986,10 +986,11 @@ static void zswap_decompress(struct zswap_entry *en= try, struct folio *folio) > acomp_request_set_params(acomp_ctx->req, &input, &output, entry->= length, PAGE_SIZE); > BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &= acomp_ctx->wait)); > BUG_ON(acomp_ctx->req->dlen !=3D PAGE_SIZE); > - mutex_unlock(&acomp_ctx->mutex); > > if (src !=3D acomp_ctx->buffer) > zpool_unmap_handle(zpool, entry->handle); Actually now that I think more about it, I think this check isn't entirely safe, even under the lock. Is it possible that 'acomp_ctx->buffer' just happens to be equal to 'src' from a previous decompression at the same handle? In this case, we will also mistakenly skip the unmap. It would be more reliable to set a boolean variable if we copy to acomp_ctx->buffer and do the unmap, and check that flag here to check if the unmap was done or not. > + > + mutex_unlock(&acomp_ctx->mutex); > } > > /********************************* > > base-commit: 0e5bdedb39ded767bff4c6184225578595cee98c > -- > 2.27.0 >