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 B6034D637CD for ; Wed, 13 Nov 2024 22:02:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 485606B008C; Wed, 13 Nov 2024 17:02:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4356B6B0093; Wed, 13 Nov 2024 17:02:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2FC996B0095; Wed, 13 Nov 2024 17:02:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 12F3B6B008C for ; Wed, 13 Nov 2024 17:02:41 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 985ED40CBB for ; Wed, 13 Nov 2024 22:02:40 +0000 (UTC) X-FDA: 82782445494.21.3FE183F Received: from mail-qv1-f44.google.com (mail-qv1-f44.google.com [209.85.219.44]) by imf30.hostedemail.com (Postfix) with ESMTP id EC7558002B for ; Wed, 13 Nov 2024 22:01:16 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=P5rYpyz4; spf=pass (imf30.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.44 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=1731535215; 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=8Tue0NA7Gyr/4e4kASUwpcagXEvSkFrkfPrAWsPw47E=; b=cmxLD9Dk46dUqgQVCgPbgrTTCXjpmfM9b/QNPcX4/ylwHwnKKK1FPmZA9Mkp021nmXMMBl iqdEEK600HF/h5zAOiTImnYg5ppU3C37vMSuY5ZRD/HtW6w1hmKgmHIFMHdeBBScYNnWLW XddcmzGSIQ1CoPd7LQFruuTtseI821g= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731535215; a=rsa-sha256; cv=none; b=psQuE0hKRKAbDYx8mXNyJisscGhA72+dG2td/aSHch/fAasryulGN3tuZhzbc0nD3WVRBu QwxKtIXLpUiAGaJsQHknqyss1bXPASaMuob2S+qR2ujexPzSK2vJJLLkoAjbUWRF4ahJWd Xg+gMyEzktJNTmlamyy04fDajyKpco8= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=P5rYpyz4; spf=pass (imf30.hostedemail.com: domain of yosryahmed@google.com designates 209.85.219.44 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-qv1-f44.google.com with SMTP id 6a1803df08f44-6d396a6f6aaso53256626d6.2 for ; Wed, 13 Nov 2024 14:02:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1731535358; x=1732140158; 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=8Tue0NA7Gyr/4e4kASUwpcagXEvSkFrkfPrAWsPw47E=; b=P5rYpyz4APl1ztGrBfZ/7x/ODWq2QBd0Gz5O+WeOLwWpkoDkHxHcC45OZjiorZqTdN 4ae5IaPQhnrHaqvfUUpPA+DU+WeJj5qNDs0ku/WskYSLh/0H9pAnNQKgtZqngngxBcWB d4Ciu/eLvLfo0d5UCfKE1HAAtrmrXZVTsa1/UtynoUFnVgeOmGtg0NaaqOr03a4SpaKF c7291HnlOJVzGF3xSJhfUZ0pqPApAVbirSVT/F+QedfbTc73its83pjbtNaNRxlbdrE5 2XlXXjlvMWHOAiqnzJYTY7ST+NuCvYfUxFQLPO7VRzKf22G71rtkqhZHKCwW4IIj/+xK cL6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731535358; x=1732140158; 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=8Tue0NA7Gyr/4e4kASUwpcagXEvSkFrkfPrAWsPw47E=; b=my+lmFuNTJroU9b7PtuGMje+kj27/40ykAVavFtaO9KNvBBcAbLsWdsqYe7xpMavgx /ljVmMoBF4ZHFEfrtDWfMkqkYt0+IGfKOGsB6FocPpmYn/pQUutfIwr1aMCu5qB9wB2C Sr6YAg/pEm+Lc+lsIuo3maIjBADYZro67lmIep3RqFeBNrGodlUM+bbiwNmXamCHvgk/ uPSNhh/cRrSY0QLCxIY/mFBbu7UUVH9APxxnTvjgsRUL1FLmAABQ518q42sdaj51eJS7 Jv6D1+Gjdx69UNpCWPBYdly/hoJpPm+OUH2UIVKG/pLxoXVdL6y83/kIUi0HoxeIiIJZ Ll/Q== X-Forwarded-Encrypted: i=1; AJvYcCUie9fntMMTho4OHrDKfFm8QPoy+n2ixr1Iiex3ght0PtjcyafqjHucSWh1uUP4BTM1OuMf0gk4Ig==@kvack.org X-Gm-Message-State: AOJu0YzyCe5+0suQ8vNBF+SGw5y4PYJ/Q19oSRbWCgoOTzTBl56E0akS Qn5QKtjO5WSLAAGOVzStododZa18IdPr+X/rSpnqHGerIT9ay4YoqVrgaioZE9t07IRIb6vln7d EABUiCEX4ux0umYOM0QpeVzIUBVutZPEOA3CP X-Google-Smtp-Source: AGHT+IF5BDgKpU5OEcDYKDM2Wg1pubTLvM+jzGp12TT20Yq80Ru0soRPNTVi2XWqR8cM920PBmqkYGuhXujI83V7vZ4= X-Received: by 2002:a05:6214:5c07:b0:6d3:4849:1b8a with SMTP id 6a1803df08f44-6d39e14cdb2mr314905816d6.21.1731535356166; Wed, 13 Nov 2024 14:02:36 -0800 (PST) MIME-Version: 1.0 References: <20241113052413.157039-1-kanchana.p.sridhar@intel.com> <20241113213007.GB1564047@cmpxchg.org> In-Reply-To: <20241113213007.GB1564047@cmpxchg.org> From: Yosry Ahmed Date: Wed, 13 Nov 2024 14:01:59 -0800 Message-ID: Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress(). To: Johannes Weiner Cc: "Sridhar, Kanchana P" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.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-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: EC7558002B X-Stat-Signature: rdys1efz5o6ffpfr43mhhxsjce4ja1fz X-HE-Tag: 1731535276-495370 X-HE-Meta: U2FsdGVkX1/AUj3C3JZ/LUG2eFcmdAZ0YXNdBxHdwjHs76oYQZYS8V2R7EEUD4tyZJuBa5ZgIqhVrVf+AK58vHMFy0L/573WjF0Rnh7+vhLpYfXwPmgCicbeUGGhPR5PWmfTPFZFRspTrP/Eph8McXHsCT+2g+sa9nsRWFmKxToL/H/jjSXCqiBOw7nEJCJAJku/xYSpeKub8hCwMx6pW+G/G79q+ScGOh9i2hF+7z0CVH77Iokqy4JbivXGrYstYgqKxH/mj1XwdS+ffu02K3lGOO/sKPm0mWeSyYeNl8YenxgVl+xb/Gv1j0XGnAEDLl0edz0LpUbFIbTXdGzR225ZqchPBvhHxqeKSBifc6/Ol6KtQxItX0hI7AGJSF9CycyqouQmf+PBn49RBItJ8AqjK/kUNqFc3v2c46AKziVun9sTDv0ReXqtOWNAmO3bDJySRfVA0wPmYZ8Sg2DUP44NwDrAvhlYnk3hyF5tjU8o2ml29HUfRn7MLmjkQtuZeJN6/sIITvI2IoNL0oiekYO6d0qBGiBXHye/vtEFUqP0waIzewwZSQEnrbDlO/oPinsO4kiYxmp08ot9PKUEWFjvnKuISCWJOzEA93tsKAnfU0luflILFSj5JgNoddvD3Yy0oE3d49boYb+vZuSN0Yto4npCn+w+uhjIDOtrFReZybwUFi5Oy/cZ8wKFCBnk+18w3uQdtoJ7xLQuapWpvMZGlINhXgJ+3eYi5p+IZHOm+CrxIBLUHDoX4MzJDYyj0bCyEqvtd3bMZREne6EB/7mbQQ4jHUDUstdwfLI0Nl4dgT9Csu8A539nbXyBf3ZSDwA1GDqg7VZiAdkeXLFa2svbX1+pbvI/jajIsvpJyrcuArl3rnjX0ZEbrwDlLWrwOB9/Dj0uXGIU5R0DTdo4sc+fIMHws5TyYvbXQRNdcvJN3yjW99vM4usFMpXQzfXgVyTXkLMfyCw7BjkLFaX q6yVnGZQ M4/mKJ7j9gXOSQVOeXVlruRq7baJk2tcAPAlIz+bCaIpQuYTHKJCIvjYNuIb6aLWCBSa1z2bx+mambYTkyluEGoHb8Baq/gYluDxNLaAw+H7meNY3Ez0VIP/m6uLQiFmtEqWOhwyjFsWzWMbkfvZLij8rN9Eku7NAWMaCYJuGrEfUYFfB+IviDWbtz657KaWbnvmELCSYg6sYqn5lPlv8x9jOF7Tpg1hM6MxaK9R6zzCH88JT/NCjbqQOFw6/95CA/YM5wdOguK/ioK0t3OSG4JvQkMLj0tYdudOO/gt8G3gE8gVJj1mpwFeL7bt0RUkXnKKquAt0vHi44A3XEchuTXUZKw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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 Wed, Nov 13, 2024 at 1:30=E2=80=AFPM Johannes Weiner wrote: > > On Wed, Nov 13, 2024 at 07:12:18PM +0000, Sridhar, Kanchana P wrote: > > I am still thinking moving the mutex_unlock() could help, or at least h= ave > > no downside. The acomp_ctx is per-cpu and it's mutex_lock/unlock > > safeguards the interaction between the decompress operation, the > > sg_*() API calls inside zswap_decompress() and the shared zpool. > > > > If we release the per-cpu acomp_ctx's mutex lock before the > > zpool_unmap_handle(), is it possible that another cpu could acquire > > it's acomp_ctx's lock and map the same zpool handle (that the earlier > > cpu has yet to unmap or is concurrently unmapping) for a write? > > If this could happen, would it result in undefined state for both > > these zpool ops on different cpu's? > > The code is fine as is. > > Like you said, acomp_ctx->buffer (the pointer) doesn't change. It > points to whatever was kmalloced in zswap_cpu_comp_prepare(). The > handle points to backend memory. Neither of those addresses can change > under us. There is no confusing them, and they cannot coincide. > > The mutex guards the *memory* behind the buffer, so that we don't have > multiple (de)compressors stepping on each others' toes. But it's fine > to drop the mutex once we're done working with the memory. We don't > need the mutex to check whether src holds the acomp buffer address. I have to admit that I confused myself with this alleged bug more than I like to admit :) I initially thought acomp_ctx->buffer can be changed, then when I realized it cannot be changed I did not tie that back to the 'fix' not being needed at all. I need more coffee. > > That being said, I do think there is a UAF bug in CPU hotplugging. > > There is an acomp_ctx for each cpu, but note that this is best effort > parallelism, not a guarantee that we always have the context of the > local CPU. Look closely: we pick the "local" CPU with preemption > enabled, then contend for the mutex. This may well put us to sleep and > get us migrated, so we could be using the context of a CPU we are no > longer running on. This is fine because we hold the mutex - if that > other CPU tries to use the acomp_ctx, it'll wait for us. > > However, if we get migrated and vacate the CPU whose context we have > locked, the CPU might get offlined and zswap_cpu_comp_dead() can free > the context underneath us. I think we need to refcount the acomp_ctx.