Message-ID: <20250210191235.GA2256827@perftesting>
Date: Mon, 10 Feb 2025 14:12:35 -0500
From: Josef Bacik <josef@...icpanda.com>
To: Joanne Koong <joannelkoong@...il.com>
Cc: Vlastimil Babka <vbabka@...e.cz>, Matthew Wilcox <willy@...radead.org>,
	Miklos Szeredi <miklos@...redi.hu>,
	Christian Heusel <christian@...sel.eu>,
	Miklos Szeredi <mszeredi@...hat.com>, regressions@...ts.linux.dev,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-mm <linux-mm@...ck.org>,
	Mantas Mikulėnas <grawity@...il.com>
Subject: Re: [REGRESSION][BISECTED] Crash with Bad page state for
 FUSE/Flatpak related applications since v6.13

On Mon, Feb 10, 2025 at 10:13:51AM -0800, Joanne Koong wrote:
> On Mon, Feb 10, 2025 at 12:27 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> >
> > On 2/8/25 16:46, Joanne Koong wrote:
> > > On Sat, Feb 8, 2025 at 2:11 AM Matthew Wilcox <willy@...radead.org> wrote:
> > >>
> > >> On Fri, Feb 07, 2025 at 04:22:56PM -0800, Joanne Koong wrote:
> > >> > > Thanks, Josef. I guess we can at least try to confirm we're on the right track.
> > >> > > Can anyone affected see if this (only compile tested) patch fixes the issue?
> > >> > > Created on top of 6.13.1.
> > >> >
> > >> > This fixes the crash for me on 6.14.0-rc1. I ran the repro using
> > >> > Mantas's instructions for Obfuscate. I was able to trigger the crash
> > >> > on a clean build and then with this patch, I'm not seeing the crash
> > >> > anymore.
> > >>
> > >> Since this patch fixes the bug, we're looking for one call to folio_put()
> > >> too many.  Is it possibly in fuse_try_move_page()?  In particular, this
> > >> one:
> > >>
> > >>         /* Drop ref for ap->pages[] array */
> > >>         folio_put(oldfolio);
> > >>
> > >> I don't know fuse very well.  Maybe this isn't it.
> > >
> > > Yeah, this looks it to me. We don't grab a folio reference for the
> > > ap->pages[] array for readahead and it tracks with Mantas's
> > > fuse_dev_splice_write() dmesg. this patch fixed the crash for me when
> > > I tested it yesterday:
> > >
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index 7d92a5479998..172cab8e2caf 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 = 0; i < ap->num_folios; i++)
> > > +       for (i = 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,7 @@ static void fuse_readahead(struct readahead_control *rac)
> > >
> > >                 while (ap->num_folios < cur_pages) {
> > >                         folio = readahead_folio(rac);
> > > +                       folio_get(folio);
> >
> > This is almost the same as my patch, but balances the folio_put() in
> > readahead_folio() with another folio_get(), while mine uses
> > __readahead_folio() that does not do folio_put() in the first place.
> >
> > But I think neither patch proves the extraneous folio_put() comes from
> > fuse_try_move_page().
> >
> > >                         ap->folios[ap->num_folios] = folio;
> > >                         ap->descs[ap->num_folios].length = folio_size(folio);
> > >                         ap->num_folios++;
> > >
> > >
> > > I reran it just now with a printk by that ref drop in
> > > fuse_try_move_page() and I'm indeed seeing that path get hit.
> >
> > It might get hit, but is it hit in the readahead paths? One way to test
> > would be to instead of yours above or mine change, to stop doing the
> > foio_put() in fuse_try_move_page(). But maybe it's called also from other
> > contexts that do expect it, and will leak memory otherwise.
> 
> When I tested it a few days ago, I printk-ed the address of the folio
> and it matched in fuse_readahead() and try_move_page(). I think that
> proves that the extra folio_put() came from fuse_try_move_page()
> through the readahead path.

This patch should fix the problem, let me know if it's stil happening

>From 964c798ee9e8f2e8e2c37cfd060c76a772cc45b7 Mon Sep 17 00:00:00 2001
Message-ID: <964c798ee9e8f2e8e2c37cfd060c76a772cc45b7.1739214698.git.josef@...icpanda.com>
From: Josef Bacik <josef@...icpanda.com>
Date: Mon, 10 Feb 2025 14:06:40 -0500
Subject: [PATCH] fuse: drop extra put of folio when using pipe splice

In 3eab9d7bc2f4 ("fuse: convert readahead to use folios"), I converted
us 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 I failed to catch the case for splice pipe
responses where we will remove the old folio and splice in the new
folio.  Here we assumed that there is a reference held on the folio for
ap->folios, which is no longer the case.

To fix this, simply drop the extra put to keep us consistent with the
non-splice variation.  This will fix the UAF bug that was reported.

Link: https://lore.kernel.org/linux-fsdevel/2f681f48-00f5-4e09-8431-2b3dbfaa881e@heusel.eu/
Fixes: 3eab9d7bc2f4 ("fuse: convert readahead to use folios")
Signed-off-by: Josef Bacik <josef@...icpanda.com>
---
 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_copy_state *cs, struct page **pagep)
 	}
 
 	folio_unlock(oldfolio);
-	/* Drop ref for ap->pages[] array */
-	folio_put(oldfolio);
 	cs->len = 0;
 
 	err = 0;
-- 
2.43.0

Powered by blists - more mailing lists