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 0BC2DD41C03 for ; Wed, 13 Nov 2024 06:22:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7436F6B00A7; Wed, 13 Nov 2024 01:22:33 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6D4A46B00AC; Wed, 13 Nov 2024 01:22:33 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 544376B00AD; Wed, 13 Nov 2024 01:22:33 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 2F66D6B00A7 for ; Wed, 13 Nov 2024 01:22:33 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id CFF051407D6 for ; Wed, 13 Nov 2024 06:22:32 +0000 (UTC) X-FDA: 82780076568.12.684C1D3 Received: from mail-vk1-f174.google.com (mail-vk1-f174.google.com [209.85.221.174]) by imf28.hostedemail.com (Postfix) with ESMTP id A8C72C0008 for ; Wed, 13 Nov 2024 06:21:47 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=gvJIB3x2; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.174 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=1731478777; 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=1OS1dBSTD/d7MdmWEB3ioT+o20JOJetm7ko1itzjkB8=; b=Su+qRPM6L1XL00bZVI69SiFefo3pJrQvBA5GxD9mnlaser+8W3umy5R3p9dJinPiZ1ayfC 8JRoAn9mMM5hNdJv5wSZVcUBlTm6VTEXyVl08gbdxFbK5i0b9Kg3KhH7Ge3Al70VHNiFu6 ozmdsA2nap1XrxnRF0PLEN/VVxJ0xw8= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=gvJIB3x2; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.174 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731478777; a=rsa-sha256; cv=none; b=LuAEN669E9B1VIJmQ82FlJ2b2sfXN6VZmvBZpjmC8LpRSenK7D2zIXnyqjTgBwy8QnT2aE 6pEzpjSOVDJU5ZDbO6oAsvLZmOEIbhgEIGT0tmn9VZ4RkMtiJdB5NYzL+dqKIGOyYaHJPU FOgkujbjFgTkZaAshhPDT9o9EKNjzXQ= Received: by mail-vk1-f174.google.com with SMTP id 71dfb90a1353d-513de426746so2778834e0c.3 for ; Tue, 12 Nov 2024 22:22:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1731478950; x=1732083750; 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=1OS1dBSTD/d7MdmWEB3ioT+o20JOJetm7ko1itzjkB8=; b=gvJIB3x2NPcTMWk/CkXOD4IG3yEDdl4GvQfqZe0RhIwiGzntqUstWWUDymPTu3Le7l K5pM0GYsaVIUEmF1V0wV+VcQa7Avv8LeIAV/1VHXBRscZi9Bu4W/PRGLZYKuUNSql/j3 abDE2DqTJ5YzQtg52gI9EOzAQdNCaubC/55/uINLUB0ugFiDKcMeyu0RextsvZthq52P pzMf5iho1qxabojgdaYNIMi0Tz0Kt0bBwO80heKOFdchcxaxGvi5S5IQRCs4/xqY5sy0 LII/DcOjsnPXEBh3fiAfjl3xlDxviSRuqvnEhoUl8vXMo8Wp5Hxum0NeG+YtfkNkvxdF l34g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731478950; x=1732083750; 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=1OS1dBSTD/d7MdmWEB3ioT+o20JOJetm7ko1itzjkB8=; b=AqrbaGru02fDWkygcoEWGolGM7SNadmaSz6I5bq6xcwX9On+I8KE8qPAohT8LXEVFf S8Qu6lek/Ry2hcVOCazHOAOUb2SXTPsI7H6Tw1b7Wa024XQUxpSc1Y8yWlrDc4nv5kin JtHzsI97WpOu44IIxST3W2y73WAB0sj+s3z7Rgr4SjTxs+tXDweFyEdiQnS3LyfiP/jw DZ/dCk94BF4+AfmebwLnqEjR3m5aUaSHdRbeZk5b1vblEY87LTGvM7dOPhAOv4zMGeIv o2sIPepU/o5s9ExeLOy26+XixNouZeywbeG9bqKnfAEbwl6fIY+2UTWDwlIlkDp3PQdM cyew== X-Forwarded-Encrypted: i=1; AJvYcCWu9XosVDcxcj5ozEQtrfeK4Rg0ckIc4EZKaInaTSEABaBRdYG1QQRzQFt4o7dsgUovv2OukoUF8g==@kvack.org X-Gm-Message-State: AOJu0YwyWDv5t68ZBVCJMggcxwQ2OWiDaAgdVZ0L43SBd7gvohJ8Vjib bVjJo1XEJq8zmxmbs8UxJkINeVRJjmqgjptL671peXWeRosORnxoSMv1KT1GvvbcqyppsaXymwF YEmojyg+9U9Kv6aPRY76HLaX0l/uYCjApOIS0 X-Google-Smtp-Source: AGHT+IG2yb5waITtZc4S+F4tVrhABeWZlltTyROL5h4zXO140BIbtCHIs7QWzk9+EN4YfOvSgLFIrqgr25IcNP17XHk= X-Received: by 2002:a05:6122:1821:b0:50d:57df:1522 with SMTP id 71dfb90a1353d-51401b94c0emr18733458e0c.6.1731478949905; Tue, 12 Nov 2024 22:22:29 -0800 (PST) MIME-Version: 1.0 References: <20241113052413.157039-1-kanchana.p.sridhar@intel.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 12 Nov 2024 22:21:53 -0800 Message-ID: Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress(). To: "Sridhar, Kanchana P" 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" , "Huang, Ying" , "21cnbao@gmail.com" <21cnbao@gmail.com>, "akpm@linux-foundation.org" , "Feghali, Wajdi K" , "Gopal, Vinodh" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: A8C72C0008 X-Stat-Signature: uzn8i3mm4u7pqqy9b1knq1haemn79sjq X-Rspam-User: X-HE-Tag: 1731478907-954783 X-HE-Meta: U2FsdGVkX1++thvB1NmgW+W+yhv9l1Ed48EuOQvF/jt35YWi59P13qYcNQgWqx6vJqC1d97P68sI4bFZlnOaNkgnk61Ml+/+ZDgXvuKYkr/brhT4pz02afLi/csHlF9mLNHDY2kuwilpd9N7RPJOO7s+Cra0C8mS3GhVlRNboUHDaEICnsbVkYzjBiOOJrKFA0fFmLeCyc8pCZgyTenoltkF7xlYPt98HrW9I6pLtyBGfpOWeb5lvKgrQKjGnwS4lbe1NgIqIBYrfTTLDbKrmpD692LkcI3iqnEdlXXck6m16eY0b0tMnRoyTv2m5Rh9jC7mpnXWUEQcDrDrGPME1SjyVnh6Hu6FYERccZtYMDYDzS/mJ1Rm2Num+EALWc4nGVVHb/Rm65NHAgb3gvjI1Q0qH+zyG/2rwRwyN6Z+e5kmOoM3rA0UIJIwef/J4YDA1YyfY3+IZyTeO3xbA7Sqd3PjJE/i99zG1RN8qUpAwDgUsjggWseieKU/b6CEa8UOlahT9SpJrq6zMD3c07L4RJTRVA55AbOX4bN/toNEIZB0eMP+zTZfbQbvSqExAZfhCIMuvU5I+Hf4WgVgOr7/8ySTEopejnTqMjs1gDRgH3io4X/O6ZK5qxJWG3v9jJiZ7oMau9a2aJcoP5Nwpk5vHlcwqe9dPxZtbg9Qkz/ZjvfAHzxhpGnXKkmQ0PFGifZLyt5joMfPidW2AWpnPA4y/XNsWMZ2xvGyMVUhVCsn7PJcaqdPWkqfDqhoJUFvGulqgEEv0j7EvKPruJMSVcN9fQiP9v60YXZg+HEf5v+v0mz3HIHOFzM8ps38hjtZpZ3G2/66YFN+FrXxykjU4+PHzKAEyqiCNg7/sVEjdnvaCdMnqyVwAXb68hpEQ9nI8VeJSKw3rxhTsFTAi3MWtweCiRUhUx67bgBpW6j/e8OLoHyIuM6RQgbxNZxxg8K4MYl2GfwUjGKLXJPSetuaHH0 8hxA5QUj Bd3ejJOR3CgGtdsjkpdE/lpqwNaZ+EChMmvU1OedQyI7YM0G9gEiB1+hhzsLwgBIowaZ5+jJDxAwtZ6HS9RoLaZkB3KIoYbsVXXDQOEWnUpdaa+5F97chMST0YMwag0hL4o4MW4KuM65GAyxrsTj/ramgAjJcbYCgyAie1YWMI35nJD81sqcu4KfaBf2LpuX0DV+CUGURpw7/mP5d5TyKkklv3ys7lhhAfIHFGrNliMDt9hJEjk4jSne4O0qWCqySoue6/gOOyw/aDiw65MXOYzKqZHVwJRsaR6u3+b8bBXaQls77narvUhFhnZuS5ahYv0AWXOPvfE9JXg7UIWKL3kL7N/eQwV6C0d8NqPR045sqP4oWTzBPhHrhg/aoMxmA61iD2a0g0Coa8PfWyu8g77LkZgNOqXHML/ajk/tTuxWrfC+00mHVqg8XKK4YK+fSOqO/KS3TrxVyKasTri2OAbGkpmUwDD1o1+s+Hj+UJOUI//vIXGA0l8IJfD5Wm28KsqRLC5Ds1BP+vnuOyp++4gR28aDuWWeQZnVa91DrcyxDWRxMA61mL6P+aE7kqVhACkt3O1mFNMU66f1WTaY0qs2bJcg24JiLk/7MsaPONqpVelU= 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:59=E2=80=AFPM Sridhar, Kanchana P wrote: > > > > -----Original Message----- > > From: Yosry Ahmed > > Sent: Tuesday, November 12, 2024 9:35 PM > > To: Sridhar, Kanchana P > > 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; Huang, Ying > > ; 21cnbao@gmail.com; akpm@linux-foundation.org; > > Feghali, Wajdi K ; Gopal, Vinodh > > > > Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in > > zswap_decompress(). > > > > 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 integr= ity of > > > "src", which is set earlier under the acomp_ctx mutex lock. This pose= s 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 an= d > > > 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. > > Thanks Yosry. That's exactly what I meant, but I think the wording got > confusing. The problem I was trying to fix is the acomp_ctx->buffer > value changing after the lock is released. This could happen as a result = of any > other compress or decompress that acquires the lock. I will simplify and > clarify accordingly. > > > > > > > > > Even though an actual memory leak was not observed, this fix seems li= ke 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 *entry, struct folio *folio) > > > acomp_request_set_params(acomp_ctx->req, &input, &output, ent= ry- > > >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. > > If we move the mutex_unlock to happen after the conditional and unmap, > shouldn't that be sufficient under all conditions? With the fix, "src" ca= n > take on only 2 values in this procedure: the mapped handle or > acomp_ctx->buffer. The only ambiguity would be in the (unlikely?) case > if the mapped zpool handle happens to be equal to acomp_ctx->buffer. Yes, that's the scenario I mean. Specifically, in zswap_decompress(), we do not use 'acomp_ctx->buffer' so 'src' is equal to the mapped handle. But, 'acomp_ctx->buffer' happens to be equal to the same handle from a previous operation as we don't clear it. In this case, the 'src !=3D acomp_ctx->buffer' check will be false, even though it should be true. This will result in an extra zpool_unmap_handle() call. I didn't look closely, but this seems like it can have a worse effect than leaking memory (e.g. there will be an extra __kunmap_atomic() call in zsmalloc, and we may end up corrupting a random handle). > > Please let me know if I am missing anything. > > > > > 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. > > Sure, this could be done, but not sure if it is required. Please let me k= now > if we still need the boolean variable in addition to moving the mutex_unl= ock(). If we use a boolean, there is no need to move mutex_unlock(). The boolean will be a local variable on the stack that doesn't need protection. > > Thanks, > Kanchana > > > > > > + > > > + mutex_unlock(&acomp_ctx->mutex); > > > } > > > > > > /********************************* > > > > > > base-commit: 0e5bdedb39ded767bff4c6184225578595cee98c > > > -- > > > 2.27.0 > > >