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 6C02AD6C287 for ; Tue, 19 Nov 2024 19:27:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CDE006B0083; Tue, 19 Nov 2024 14:27:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C8CDD6B0088; Tue, 19 Nov 2024 14:27:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B2D336B0089; Tue, 19 Nov 2024 14:27:39 -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 94D5A6B0083 for ; Tue, 19 Nov 2024 14:27:39 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 5FF1FA07DE for ; Tue, 19 Nov 2024 19:27:39 +0000 (UTC) X-FDA: 82803826224.09.A91BCBA Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf09.hostedemail.com (Postfix) with ESMTP id 956C7140022 for ; Tue, 19 Nov 2024 19:27:02 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="X49/94PL"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf09.hostedemail.com: domain of yosryahmed@google.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1732044273; a=rsa-sha256; cv=none; b=rpHFdWGNjeA9s7AYCfbCgLSx2MW1ocYGAMEbMpqwtHWa8IQMTD5R52lJn6gqpYl/BvZapJ NX9905uBO9Bpc508WbJEGgXRfITtriyLEBWvIjRMCpm4yG6lJNSPh4xSL9VIKN3RyPunA9 WUt8xQi3CbPj3yNjVtr0jM3nKalhcW4= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="X49/94PL"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf09.hostedemail.com: domain of yosryahmed@google.com designates 209.85.160.182 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=1732044273; 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=LdnOOKDy1qaKBP09hAweLnoPW9+Venfz7d431iNqwy8=; b=ZLmYyoehZEVmMjcZH+LgNSX7uXXV37CRvCfXexTrnajYENdFurP8HCJZNC9PEcjMaD5Q5Y ln+v/W7lzpzqCnWOZoq+fooH8LQZMBYk6tIMJncAk32OM4mrSNIBcxaXIss7e9lzbJJx6k k5ptyS4Y0HO+7l/qebuDsfv0ATuAhJk= Received: by mail-qt1-f182.google.com with SMTP id d75a77b69052e-460b04e4b1cso8614431cf.2 for ; Tue, 19 Nov 2024 11:27:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1732044456; x=1732649256; 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=LdnOOKDy1qaKBP09hAweLnoPW9+Venfz7d431iNqwy8=; b=X49/94PLJI8nYBxuFwAdnbqJJCLnHxo4EBVxsy7eDNeKrrINXTcLciNtRJA6mQ5Sfm opqELZB+/PwA/ih5DDMDQw6vis5q/4zHRCrA35lvRZCncJc65qeY+AUBgE8WPiAl/cyz YW0P9Il6IX9jZkbyltr3tvu36krJsTtLIiyAcFszeZLl1v3/jGLBvQNMCweK5uXMwdMc x/5gN5vUWoFkWEQXGpX4POOC31bDjbG5XMdVMhnh+Y+rsdqcgX9awIPUu4RKgCRoVfXi Rrhd3p08MwiJ/v/8u3JTG7RqQUC89Jwnc+Xtk00zAs5Ong2VDjME8mRo4LXPpYAea5fg p82g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732044456; x=1732649256; 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=LdnOOKDy1qaKBP09hAweLnoPW9+Venfz7d431iNqwy8=; b=HiIgOcsae/tZ9Fihf0U4jaZjLkX7cimoNmYLUrmpbEb020EOTe7o0nZww9MmzdYTOe TxLV9szpKkMnTCVM6JyjxygiFD1FWksyQzvbd+wy91i/tIBHO8PMUa/qi6Nj8DbV0kFE Of7GVX6WC5nJEF5PmHBoTDdLWTn/INmsPZfNID9vXNqzB+IZuN0f7z0faHYHY08+HJ4s YHWlmnNrqmnOhw/FC5H/Cw3MGjZfV3fP7UQFeCYYSmPictXeLL3tDTuQddQ8WY8ofls5 5qoJeZVNOJWk9zRMntTrdQvNSvQSB8q62klgg0XPNr7rMGn+weTeSwGwLWqMcNw/iMy7 Ug4Q== X-Forwarded-Encrypted: i=1; AJvYcCXDyDfTNDBibNySZduWajzr+aSaUFzjIuCNthUCOOdqOAcOhdLyKAlUxU8i23WAVjf9ICblPJismw==@kvack.org X-Gm-Message-State: AOJu0YyfGszP3Wwfxn816NE/pM/vIHTiw053GYNoxGTqNzpJsRckaHBz p8/6ZqvEPWEhqw5g6KWZC1nO3wu9CFqav0VygF0yoVGA7627+9/aiPDyxMc/GDUEIRi3aFtKrfX RDWA2HSZvFFy6SkskQFcAj5KSDv8pyP3Rsktm X-Google-Smtp-Source: AGHT+IGbLoHVCdG3Fs/zVrFHCKzYptyKu9BqZFl8R7qNn8kIYXx6uLXr9bkoyg0tX9IseZbkx0ZPv/qo8CBsUgri4sc= X-Received: by 2002:a05:6214:518a:b0:6d4:1f86:b1e1 with SMTP id 6a1803df08f44-6d4377fff93mr2010626d6.19.1732044456407; Tue, 19 Nov 2024 11:27:36 -0800 (PST) MIME-Version: 1.0 References: <20241113052413.157039-1-kanchana.p.sridhar@intel.com> <20241113213007.GB1564047@cmpxchg.org> <20241114051149.GC1564047@cmpxchg.org> <9a807484-6693-4e2a-a087-97bbc5ee4ed9@linux.dev> In-Reply-To: From: Yosry Ahmed Date: Tue, 19 Nov 2024 11:27:00 -0800 Message-ID: Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress(). To: "Sridhar, Kanchana P" Cc: Chengming Zhou , Johannes Weiner , Nhat Pham , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "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-Stat-Signature: yfjmaawpckkffeurkmtrjeqmfgjeex4b X-Rspamd-Queue-Id: 956C7140022 X-Rspamd-Server: rspam08 X-Rspam-User: X-HE-Tag: 1732044422-816482 X-HE-Meta: U2FsdGVkX1+DCt75KaWXGbexk6MafRyO/uvBi405MRaqiiMpzzNZCtttoYkgM44ST5fn8D2PpG+uosHm0x8MkZrHwNOaXghRry+cEm52rUqT0CZbZXxwe1KLpc2aYIs+LMBMV7/AmLNQC6Gk8xPnwFPiSPFbX08Xjz+r5E9D0kZeOC336K+itARefQYXDHY4p0zH5gS+G90IvDZljco6e2Z/tN1c5j+umGyTruKITvu+X30RoOoeTd00/UgjNohcxU14un4JCYyY1DzFgYifINN88i/B9/io5ZkCNEtItzAR4lfDG5gnwtdqBCuHNc8WJxG/yki5rAoho9nh9uyoxz9e+03Hc6hJf1X9YQwQgpyB/iM3YEGrWltQhggaIhBTTuB1yxZYssZ5GSuIy4zFGi67ntQnKCJzFwXto2rfkvqJhrxvbfUyZ0AWXI/2m9PPfh3ZxsPdPy7D1V27rmwMvYSr6Rs3NeW+hILk6zqG59UEiHMqXjR4KMg/xNG8/vF5ogF0gaZHi2g5rwLbRkT1wVVQ/8trcTlmdDk2ZiEkM+2EsN491IW1vYJhMbMWDM/6RUn/cjJnRaLdaTABjh5EIoXOErI3Vjzpk8sIsc0bII1A90slmie+BjTJrRfxewYX1h8YwPP/AGd7ogua+68wUjj+RRpW/7oMKN2KO/Fu/hsrmEj1O7XOfLjt5dfZ7Rd/NQjM6axSdqOL8CRbBIrQLBgvGQ79ffjde33D4Ai6aH5wJNgPQ9VuP1DuCF3+0DpMHUF75Yp9Tio/Eye0skNce+zAO3DMYX0GcNlbBD5TYf1ezieXtM8iab10Z9/4QtHjcLgzA2J612J6wFbLy5CvmNQbbF81qX2cOVqdsYg3oS47s00Tut62AO+WWHlm7cA7v3/7IqGBBEZJ3uALFdzBp8kVN14Rs7kT7f0ZsCSKDQdznwJQK+WDWfatf8K4hAHSJsDYfpYqNax5ZVSxmAO 7gQh7czP fBoo8NqJqKSgtGHDXJ3+ZoSq6SHHbFemMoOah6G8p2FviPPk4GzMzQHeAdZHwrn7uO/2MCw+pu/JWn1JgUwl/IVF03xzoJIvBHMPi8gCZqrv20dBSOTcrNM1sH7TtROvvnYuMqMrWgUCFWup6uuJVbet+w7CIKNMbTLskfFMUYcj38WqEGAABY1DtobHb9wqs5+t1HKa2fZu6ROWgu3wWoVQsE5WKakvHuBfMld76myqGhTw8k67gMfF0q2k9vlb6Px+MRUKyUkDGYbq9UaVonSO7oBtEe73JHTEE25tvI9GWy9AMY4KRhD4vrPo4+dRCVo43a4HcN2NWcCo96ixdFHVOLBylV+wEsz2KGs7NiM35CdvsbiUDqKhArgkgT0zi+J+ViNH2OXs/Dh22zgs+jWoxQDtNgMtFaOfsggyj54W1pZ3r+X2PAmtApB1DIfMfbccmgnSOPiNm01KofJL4dk+mpW7DH+YKdGlyc2Fxe2z8a6NbBLo5eo6pQX4eDjDXA6oKc1Wgavq5/PdP+1DNvkflBWTYqgcTNsqeak4COOUYHlG2z8zzAMZBT3sj7cJiQwWA27imAROcHPcuiH0ImwuPVxSTNqNNI56dLXSfOvP88sT5vZBUWXm1zhr6MB95n5q91x9YxKSTdhcMCprNDLxVd7xsYaPyoalkzih408di21M= 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 19, 2024 at 11:22=E2=80=AFAM Sridhar, Kanchana P wrote: > > > > -----Original Message----- > > From: Yosry Ahmed > > Sent: Friday, November 15, 2024 1:49 PM > > To: Sridhar, Kanchana P > > Cc: Chengming Zhou ; Johannes Weiner > > ; Nhat Pham ; linux- > > kernel@vger.kernel.org; linux-mm@kvack.org; 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 Fri, Nov 15, 2024 at 1:14=E2=80=AFPM Sridhar, Kanchana P > > wrote: > > > > > > Hi Chengming, > > > > > > > -----Original Message----- > > > > From: Chengming Zhou > > > > Sent: Wednesday, November 13, 2024 11:24 PM > > > > To: Sridhar, Kanchana P ; Johannes > > Weiner > > > > > > > > Cc: Nhat Pham ; Yosry Ahmed > > > > ; linux-kernel@vger.kernel.org; linux- > > > > mm@kvack.org; 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(). > > > > > > > > Hello, > > > > > > > > On 2024/11/14 14:37, Sridhar, Kanchana P wrote: > > > > > > > > > >> -----Original Message----- > > > > >> From: Johannes Weiner > > > > >> Sent: Wednesday, November 13, 2024 9:12 PM > > > > >> To: Sridhar, Kanchana P > > > > >> Cc: Nhat Pham ; Yosry Ahmed > > > > >> ; linux-kernel@vger.kernel.org; linux- > > > > >> mm@kvack.org; 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 i= n > > > > >> zswap_decompress(). > > > > >> > > > > >> On Thu, Nov 14, 2024 at 01:56:16AM +0000, Sridhar, Kanchana P > > wrote: > > > > >>> So my question was, can we prevent the migration to a different= cpu > > > > >>> by relinquishing the mutex lock after this conditional > > > > >> > > > > >> Holding the mutex doesn't prevent preemption/migration. > > > > > > > > > > Sure, however, is this also applicable to holding the mutex of a = per-cpu > > > > > structure obtained via raw_cpu_ptr()? > > > > > > > > Yes, unless you use migration_disable() or cpus_read_lock() to prot= ect > > > > this section. > > > > > > Ok. > > > > > > > > > > > > > > > > > Would holding the mutex prevent the acomp_ctx of the cpu prior to > > > > > the migration (in the UAF scenario you described) from being dele= ted? > > > > > > > > No, cpu offline can kick in anytime to free the acomp_ctx->buffer. > > > > > > > > > > > > > > If holding the per-cpu acomp_ctx's mutex isn't sufficient to prev= ent the > > > > > UAF, I agree, we might need a way to prevent the acomp_ctx from b= eing > > > > > deleted, e.g. with refcounts as you've suggested, or to not use t= he > > > > > > > > Right, refcount solution from Johannes is very good IMHO. > > > > > > > > > acomp_ctx at all for the check, instead use a boolean. > > > > > > > > But this is not enough to just avoid using acomp_ctx for the check, > > > > the usage of acomp_ctx inside the mutex is also UAF, since cpu offl= ine > > > > can kick in anytime to free the acomp_ctx->buffer. > > > > > > I see. How would the refcounts work? Would this add latency to zswap > > > ops? In low memory situations, could the cpu offlining code over-ride > > > the refcounts? > > > > I think what Johannes meant is that the zswap compress/decompress > > paths grab a ref on the acomp_ctx before using it, and the CPU > > offlining code only drops the initial ref, and does not free the > > buffer directly. The buffer is only freed when the ref drops to zero. > > > > I am not familiar with CPU hotplug, would it be simpler if we have a > > wrapper like get_acomp_ctx() that disables migration or calls > > cpus_read_lock() before grabbing the per-CPU acomp_ctx? A similar > > wrapper, put_acompt_ctx() will be used after we are done using the > > acomp_ctx. > > Would it be sufficient to add a check for mutex_is_locked() in > zswap_cpu_comp_dead() and if this returns true, to exit without deleting > the acomp? I don't think this works. First of all, it's racy. It's possible the mutex gets locked after we check mutex_is_locked() but before we delete the acomp_ctx. Also, if we find that the mutex is locked, then we do nothing and essentially leak the memory. Second, and probably more important, this only checks if anyone is currently holding the mutex. What about tasks that may be sleeping waiting for the mutex to be unlocked? The mutex will be deleted from under them as well. > If this is an acceptable solution, it would also require us > to move the mutex_unlock() to occur after the "if (src !=3D acomp_ctx->bu= ffer)" > in zswap_decompress(). This would ensure all existing zswap code that's > within the mutex_lock()-mutex_unlock() will work correctly without > worrying about the acomp_ctx being deleted by cpu offlining. > > Not sure if this would be a comprehensive solution, or if it would have > unintended consequences to the cpu offlining code. Would appreciate > comments. > > Thanks, > Kanchana