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 2759AC0219B for ; Tue, 11 Feb 2025 21:21:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8D5446B0088; Tue, 11 Feb 2025 16:21:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 885BA280001; Tue, 11 Feb 2025 16:21:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6FF816B0093; Tue, 11 Feb 2025 16:21:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 525346B0088 for ; Tue, 11 Feb 2025 16:21:46 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id EFE6680B51 for ; Tue, 11 Feb 2025 21:21:45 +0000 (UTC) X-FDA: 83108935770.16.51D3CA2 Received: from mail-qt1-f177.google.com (mail-qt1-f177.google.com [209.85.160.177]) by imf16.hostedemail.com (Postfix) with ESMTP id 13CE3180003 for ; Tue, 11 Feb 2025 21:21:43 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gB5sV2JJ; spf=pass (imf16.hostedemail.com: domain of joannelkoong@gmail.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=joannelkoong@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1739308904; a=rsa-sha256; cv=none; b=bZTdjAe/aGsdSMDvEfbn8Lexd4WiDPWKGO8d76oi47jXUX73CC7yVCZc51FXHc5ULcAioQ yPQd32HAL1SJ05zBJ9rjHCNr0RCLXUf8uoCgtl1Zpq0ePpUb+opWvMGXLfJLn9wqCR5vjF T/XejXPr14ussxWewZkRbOb7C46IjI0= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gB5sV2JJ; spf=pass (imf16.hostedemail.com: domain of joannelkoong@gmail.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=joannelkoong@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1739308904; 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=fSbVqMiTkl0LXiSTIcddvXxCMvvitAPamcbDbPBQmiQ=; b=1uU/dstW9718+KpcJt9H6bgSxgOLWyee6f9l7StcMwBnw2xtj3fjXC0oIgDv7JyZoYDdgn sGS7G+eYZlPCNyD9zbF1CECfEBEXp0v3Id1C3kq0zU9h7jm3bfrRfH1NrhfJJrutSQNPjt HojqByMF7IVXs8XjkZnGD2xjwUbVINM= Received: by mail-qt1-f177.google.com with SMTP id d75a77b69052e-46fa734a0b8so54302821cf.1 for ; Tue, 11 Feb 2025 13:21:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1739308903; x=1739913703; 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=fSbVqMiTkl0LXiSTIcddvXxCMvvitAPamcbDbPBQmiQ=; b=gB5sV2JJI0rCch5jEaHYgvGrYTSepK2tuYyNUsFwUdgWHEcCcOlU0Vuf8DJgDAhIZi AV608Yu0jp5Ej1Gj9S3kNWsTdNAa3zKQGDhi+LSuVSe3mB1wDtrNU150Y/fu8ttkr5M7 gBIDesoUDpFZ570L6p4Bc59jey7dTUaD5C/WhHcgQkWV7a6PC4o/n4GNcRPUSJnduoSi XjLSPP/A2LDYVvBVD6uPBHHT5FD+s0CCdPWcEP34Y33+iiqjJkweg4+POqW8V3hSurBm mC/G0c0nv+ZRti7ThT4+kSeEros7ky8A5IKX3rA5d2wguFCFGxnpLdvjoc0igpseRKD8 nzbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739308903; x=1739913703; 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=fSbVqMiTkl0LXiSTIcddvXxCMvvitAPamcbDbPBQmiQ=; b=iRbuqO361zB7jDOefDB/zlBYP2iUDFK6dGwIwnNi6cDH2OHj3vlR7ZWfkJPABfTqLa oJ3ji2iL1jwWxTaWSxVYCaUIqEm1ZNNfSbmIEPXaxwaUS/80n4B197PqkyVD9pl+BsaB kW7TVKQ//lH/TVj80RnB0+oVdNAW5CVoZCE5CzO91/knisyf5IATC5kXih+qOjxSzL+b vDidj9H2UD0aQekK5d5XiT52pBYkWev18oZa9WEcYRWrOWFiRa561rZtmsJc8JDRXQoo gZM1iq4nKRDz3fe7FIQ/ZGrxQ3V9N/9VZbHhGnMMTqeazkdUf3+xBDGYjp3dzRzaN+Xj KevA== X-Forwarded-Encrypted: i=1; AJvYcCXXbkGrY1VuMpp8sm31zEuXg545kQRBeckKBlBpZ/HtG40LogrMkImyWPcgJ+J0qR4XXX+LXrq0YA==@kvack.org X-Gm-Message-State: AOJu0Yx4FLnx99w54ZJXGdUa9DIoEoVoMY/Cx37AzEFAkh9NuA9+myK1 F7OjG2K4cC0qayipRISJ6BmCpdzZhQVS8jp1NRrCc2X0JevfbRHsC/iXVOvHLhyFHOixSvMeiIj D6k2azUQ00SI1C/QbAawieW9pIYU= X-Gm-Gg: ASbGncvX67Arli2YhSZ+/6uDhJK9OeSmQsiGospoAcXG608OMaEkcM/XW6T7QGxDbUK rrKCVGYo1N7h4XSOwB3Lc5wYDvvK++OJQTDTeDPvj8ITZWzOas06CqvnV/deF0PuSEOPpXnyjBQ == X-Google-Smtp-Source: AGHT+IFKtQ8UVZLdbtYyP9WyYfW9qhBvBgtzQYc9asKQ0u4aMfE1y2wI3lrrdXfr8aSY2alE3XEyecLsRiaNx7vXddo= X-Received: by 2002:ac8:7fd4:0:b0:467:7a27:f3bb with SMTP id d75a77b69052e-471b07141bamr5850091cf.49.1739308903079; Tue, 11 Feb 2025 13:21:43 -0800 (PST) MIME-Version: 1.0 References: <9cd88643-daa8-4379-be0a-bd31de277658@suse.cz> <20250207172917.GA2072771@perftesting> <8f7333f2-1ba9-4df4-bc54-44fd768b3d5b@suse.cz> <81298bd1-e630-4940-ae5b-7882576b6bf4@suse.cz> <20250210191235.GA2256827@perftesting> <8a99f6bf3f0b5cb909f11539fb3b0ef0d65b3a73.camel@kernel.org> <85f1b4ca-cdc7-48d0-a985-4185eff1b49a@suse.cz> In-Reply-To: <85f1b4ca-cdc7-48d0-a985-4185eff1b49a@suse.cz> From: Joanne Koong Date: Tue, 11 Feb 2025 13:21:31 -0800 X-Gm-Features: AWEUYZkpMikWRVzu_vH7tBi2VNxxuVFAUWBL_42-xI-v2Um0L-YoYF3TFagCMm0 Message-ID: Subject: Re: [REGRESSION][BISECTED] Crash with Bad page state for FUSE/Flatpak related applications since v6.13 To: Vlastimil Babka Cc: Jeff Layton , Matthew Wilcox , Josef Bacik , Miklos Szeredi , Christian Heusel , Miklos Szeredi , regressions@lists.linux.dev, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm , =?UTF-8?Q?Mantas_Mikul=C4=97nas?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: hu8esmdjsbjpgifca7wexogdnitw47wj X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 13CE3180003 X-Rspam-User: X-HE-Tag: 1739308903-797795 X-HE-Meta: U2FsdGVkX18m1IFFoky+9KXG5lruR0eJx0Zh044AqQpZtOwVjpuCs6LHTDS6LA8gpyUJOZ7OcUBGeoJLg+9Rii6cpRd2YPSiW13N4MEkhAZcJMxsYQwdFMrmohSJP4ESDy6q6EjfJXor+zHtjNBYkLExn5IRq1UOLJXfV3mGME8YdAWY1sNlXWxH8GjHuqo+k4wSGH2sRrUY56Aiin2sXNL/1Hk2UT3C9MDXqtBQOCIH6BD7vLyFYK5nrSTDFw3r4zEk0H+ihzg0C4eEewqID3cSrKm1YvwzZiu84Ih8OtKphAbW3lMW0lwe6+KYglFgqSJFowc0YXPxvWKPtsxfpWECdEMQHL9z9Pmq0g1jWAxYQvmBgQEXU9z7LsEblPOxrn6XNEyHGmE1rD5eTPbCi3mjnWcnzVpx3rZnySFAEj48/VR1r0aAgETEnvp9daashktNpCGDPH8BCVuxOsaW2XZ5vEb00gFFtSgVBMgDW1Lh5v/FQF1JuLIc+j3k7ypYc1s5t1qnfykvQze3i+zXK679yT6uFQGHIY8vBO/VadZORvxPChDtL0c6G5D3UL/jp2OcpBpGJ+t03GBzZ8KI2hJPOrUJKn8FOLTD0kTHk07sC8yYRpbNOt/4QuqQbJCn8gAhrfvJnl0PrNsHW8POhIBJuyLYWOclhAtp8Zgw9OsVvPENHp3mXWNsErRBCFhTFAe/K0X0RGf+maepygZ0n+jL137etg6bWOVUq2cPZty6XhkoII901q62LJ2rxrYGsMfepKhLGj0mHX7f8VmdTYniMv4COZ8/EDwfTiFxYrPYfyoymeIoUEmWUUY9L/RT238d+vr04ohYk0ONT3rmeKrZ4nU8v+bJtL/6xbE69NX83jDuTkzXSaMQiPwi9knVp1Kt+NKMpIKbDoZwcFJYobebh5m3XEMGdZNeBrE1PUi+vP9KxfdJt28EWVy+C6jpKSsiTvEejDgnX/9Td5c sjuYZTAG r1udKGFsi9hfkNAxMikPHlTJ2LqeT7vfWzGh1NhjjeQNoG9Y7sASSRK+4Ypwuck9V3kkfLPIe8nYu5xNu4a9YGLNNqzguOVuRTQWFqVCN/Lz7wJD0W8HIhsWRroaeHtmZjz+8s6rB2Yai9Cro670YO1lmQoc3/oDHJZzm577pNc4peu+0onQ0wAU1+lzLwD2XNAZNO1jdgAT5svXbsm4QjdGKUPjJbuZokp2l/d9gk7Sa80Atde5cGTPhKgozkUWXIDDQvTRx6c4ta8i4xYVJM9CaqA47OHVHkGJV+iZdX6E/n+68O5qeKOREdzFFF6NX5EOueVlplZQTA3ZTRfFOYe6ZNVhivz83sJjOg3xraGZzjaO5fFydH2dKkr319kYetTtrXLwrpGmeC0oEth+8YSBdgywoKxXk97fGj5GD4N3PBVXst7osERhu59TwoLGR54vUltRGoF3NV5a4670cfvln0/DKeJoucIvK17gDHQ4y1OdfwytRRCKHRIEJrb9V6T+qLiw8u+fWKZwv681wc6XWkg== 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, Feb 11, 2025 at 1:01=E2=80=AFPM Vlastimil Babka wr= ote: > > On 2/11/25 20:23, Joanne Koong wrote: > > On Tue, Feb 11, 2025 at 6:01=E2=80=AFAM Jeff Layton wrote: > >> > >> On Mon, 2025-02-10 at 17:38 -0500, Jeff Layton wrote: > >> > On Mon, 2025-02-10 at 20:36 +0000, Matthew Wilcox wrote: > >> > > On Mon, Feb 10, 2025 at 02:12:35PM -0500, Josef Bacik wrote: > >> > > > From: Josef Bacik > >> > > > Date: Mon, 10 Feb 2025 14:06:40 -0500 > >> > > > Subject: [PATCH] fuse: drop extra put of folio when using pipe s= plice > >> > > > > >> > > > In 3eab9d7bc2f4 ("fuse: convert readahead to use folios"), I con= verted > >> > > > us to using the new folio readahead code, which drops the refere= nce on > >> > > > the folio once it is locked, using an inferred reference on the = folio. > >> > > > Previously we held a reference on the folio for the entire durat= ion of > >> > > > the readpages call. > >> > > > > >> > > > This is fine, however I failed to catch the case for splice pipe > >> > > > responses where we will remove the old folio and splice in the n= ew > >> > > > folio. Here we assumed that there is a reference held on the fo= lio for > >> > > > ap->folios, which is no longer the case. > >> > > > > >> > > > To fix this, simply drop the extra put to keep us consistent wit= h the > >> > > > non-splice variation. This will fix the UAF bug that was report= ed. > >> > > > > >> > > > Link: https://lore.kernel.org/linux-fsdevel/2f681f48-00f5-4e09-8= 431-2b3dbfaa881e@heusel.eu/ > >> > > > Fixes: 3eab9d7bc2f4 ("fuse: convert readahead to use folios") > >> > > > Signed-off-by: Josef Bacik > >> > > > --- > >> > > > fs/fuse/dev.c | 2 -- > >> > > > 1 file changed, 2 deletions(-) > >> > > > > >> > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > >> > > > index 5b5f789b37eb..5bd6e2e184c0 100644 > >> > > > --- a/fs/fuse/dev.c > >> > > > +++ b/fs/fuse/dev.c > >> > > > @@ -918,8 +918,6 @@ static int fuse_try_move_page(struct fuse_co= py_state *cs, struct page **pagep) > >> > > > } > >> > > > > >> > > > folio_unlock(oldfolio); > >> > > > - /* Drop ref for ap->pages[] array */ > >> > > > - folio_put(oldfolio); > >> > > > cs->len =3D 0; > >> > > > >> > > But aren't we now leaking a reference to newfolio? ie shouldn't > >> > > we also: > >> > > > >> > > - folio_get(newfolio); > >> > > > >> > > a few lines earlier? > >> > > > >> > > >> > > >> > I think that ref was leaking without Josef's patch, but your propose= d > >> > fix seems correct to me. There is: > >> > > >> > - 1 reference stolen from the pipe_buffer > >> > - 1 reference taken for the pagecache in replace_page_cache_folio() > >> > - the folio_get(newfolio) just after that > >> > > >> > The pagecache ref doesn't count here, and we only need the reference > >> > that was stolen from the pipe_buffer to replace the one in pagep. > >> > >> Actually, no. I'm wrong here. A little after the folio_get(newfolio) > >> call, we do: > >> > >> /* > >> * Release while we have extra ref on stolen page. Otherwise > >> * anon_pipe_buf_release() might think the page can be reused. > >> */ > >> pipe_buf_release(cs->pipe, buf); > >> > >> ...so that accounts for the extra reference. I think the newfolio > >> refcounting is correct as-is. > > > > I think we do need to remove the folio_get(newfolio); here or we are > > leaking the reference. > > > > new_folio =3D page_folio(buf->page) # ref is 1 > > replace_page_cache_folio() # ref is 2 > > folio_get() # ref is 3 > > pipe_buf_release() # ref is 2 > > > > One ref belongs to the page cache and will get dropped by that, but > > the other ref is unaccounted for (since the original patch removed > > "folio_put()" from fuse_readpages_end()). > > > > I still think acquiring an explicit reference on the folio before we > > add it to ap->folio and then dropping it when we're completely done > > with it in fuse_readpages_end() is the best solution, as that imo > > makes the refcounting / lifetimes the most explicit / clear. For > > example, in try_move_pages(), if we get rid of that "folio_get()" > > call, the page cache is the holder of the remaining reference on it, > > and we rely on the earlier "folio_clear_uptodate(newfolio);" line in > > try_move_pages() to guarantee that the newfolio isn't freed out from > > under us if memory gets tight and it's evicted from the page cache. > > > > imo, a patch like this makes the refcounting the most clear: > > > > From 923fa98b97cf6dfba3bb486833179c349d566d64 Mon Sep 17 00:00:00 2001 > > From: Joanne Koong > > Date: Tue, 11 Feb 2025 10:59:40 -0800 > > Subject: [PATCH] fuse: acquire explicit folio refcount for readahead > > > > In 3eab9d7bc2f4 ("fuse: convert readahead to use folios"), the logic > > was converted to using the new folio readahead code, which drops the > > reference on the folio once it is locked, using an inferred reference > > on the folio. Previously we held a reference on the folio for the > > entire duration of the readpages call. > > > > This is fine, however for the case for splice pipe responses where we > > will remove the old folio and splice in the new folio (see > > fuse_try_move_page()), we assume that there is a reference held on the > > folio for ap->folios, which is no longer the case. > > > > To fix this and make the refcounting explicit, acquire a refcount on th= e > > folio before we add it to ap->folios[] and drop it when we are done wit= h > > the folio in fuse_readpages_end(). This will fix the UAF bug that was > > reported. > > > > Link: https://lore.kernel.org/linux-fsdevel/2f681f48-00f5-4e09-8431-2b3= dbfaa881e@heusel.eu/ > > Fixes: 3eab9d7bc2f4 ("fuse: convert readahead to use folios") > > Can we add some tags? > > Reported-by: Christian Heusel > Closes: https://lore.kernel.org/all/2f681f48-00f5-4e09-8431-2b3dbfaa881e@= heusel.eu/ > Closes: https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-= /issues/110 > Reported-by: Mantas Mikul=C4=97nas > Closes: https://lore.kernel.org/all/34feb867-09e2-46e4-aa31-d9660a806d1a@= gmail.com/ > Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=3D1236660 > Cc: > Ok, I'll add these tags in and formally submit this patch to Miklos's tree. > > Signed-off-by: Joanne Koong > > --- > > fs/fuse/file.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index 7d92a5479998..6fa535c73d93 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -955,8 +955,10 @@ static void fuse_readpages_end(struct fuse_mount > > *fm, struct fuse_args *args, > > fuse_invalidate_atime(inode); > > } > > > > - for (i =3D 0; i < ap->num_folios; i++) > > + for (i =3D 0; i < ap->num_folios; i++) { > > folio_end_read(ap->folios[i], !err); > > + folio_put(ap->folios[i]); > > + } > > if (ia->ff) > > fuse_file_put(ia->ff, false); > > > > @@ -1049,6 +1051,12 @@ static void fuse_readahead(struct readahead_cont= rol *rac) > > > > while (ap->num_folios < cur_pages) { > > folio =3D readahead_folio(rac); > > + /* > > + * Acquire an explicit reference on the folio i= n case > > + * it's replaced in the page cache in the splic= e case > > + * (see fuse_try_move_page()). > > + */ > > + folio_get(folio); > > It would be more efficient to use __readahead_folio() instead of doing a = folio_get() > to counter a folio_put() in readahead_folio(). An adjusted comment can ex= plain why > we use __readahead_folio(). imo, the explicit get makes the code the most readable, but I also don't feel strongly enough about it to insist. I'll make this change in the patch. > > > ap->folios[ap->num_folios] =3D folio; > > ap->descs[ap->num_folios].length =3D folio_size= (folio); > > ap->num_folios++; > > -- > > 2.43.5 > > > >> -- > >> Jeff Layton >