From: Miklos Szeredi <[email protected]>
commit 712a951025c0667ff00b25afc360f74e639dfabe upstream.
It is possible to trigger a crash by splicing anon pipe bufs to the fuse
device.
The reason for this is that anon_pipe_buf_release() will reuse buf->page if
the refcount is 1, but that page might have already been stolen and its
flags modified (e.g. PG_lru added).
This happens in the unlikely case of fuse_dev_splice_write() getting around
to calling pipe_buf_release() after a page has been stolen, added to the
page cache and removed from the page cache.
Fix by calling pipe_buf_release() right after the page was inserted into
the page cache. In this case the page has an elevated refcount so any
release function will know that the page isn't reusable.
Reported-by: Frank Dinoff <[email protected]>
Link: https://lore.kernel.org/r/CAAmZXrsGg2xsP1CK+cbuEMumtrqdvD-NKnWzhNcvn71RV3c1yw@mail.gmail.com/
Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
Cc: <[email protected]> # v2.6.35
Signed-off-by: Miklos Szeredi <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
fs/fuse/dev.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -847,6 +847,12 @@ static int fuse_try_move_page(struct fus
replace_page_cache_page(oldpage, newpage);
+ /*
+ * 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);
+
get_page(newpage);
if (!(buf->flags & PIPE_BUF_FLAG_LRU))
@@ -2031,8 +2037,12 @@ static ssize_t fuse_dev_splice_write(str
pipe_lock(pipe);
out_free:
- for (idx = 0; idx < nbuf; idx++)
- pipe_buf_release(pipe, &bufs[idx]);
+ for (idx = 0; idx < nbuf; idx++) {
+ struct pipe_buffer *buf = &bufs[idx];
+
+ if (buf->ops)
+ pipe_buf_release(pipe, buf);
+ }
pipe_unlock(pipe);
kvfree(bufs);
On Mon, Nov 15, 2021 at 7:04 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> From: Miklos Szeredi <[email protected]>
>
> commit 712a951025c0667ff00b25afc360f74e639dfabe upstream.
>
> It is possible to trigger a crash by splicing anon pipe bufs to the fuse
> device.
>
> The reason for this is that anon_pipe_buf_release() will reuse buf->page if
> the refcount is 1, but that page might have already been stolen and its
> flags modified (e.g. PG_lru added).
>
> This happens in the unlikely case of fuse_dev_splice_write() getting around
> to calling pipe_buf_release() after a page has been stolen, added to the
> page cache and removed from the page cache.
>
> Fix by calling pipe_buf_release() right after the page was inserted into
> the page cache. In this case the page has an elevated refcount so any
> release function will know that the page isn't reusable.
>
> Reported-by: Frank Dinoff <[email protected]>
> Link: https://lore.kernel.org/r/CAAmZXrsGg2xsP1CK+cbuEMumtrqdvD-NKnWzhNcvn71RV3c1yw@mail.gmail.com/
> Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
> Cc: <[email protected]> # v2.6.35
> Signed-off-by: Miklos Szeredi <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
It appears this patch causes a rather serious regression in flatpacks
using portals to access files. Reverting this patch restores expected
behavior. I have asked users in the Fedora bug to test with 5.16-rc2
to see if we are just missing a dependent patch in stable, or if this
is broken there as well, but no response yet.:
https://bugzilla.redhat.com/show_bug.cgi?id=2025285
https://github.com/flatpak/flatpak/issues/4595
Justin
Justin
On Tue, Nov 23, 2021 at 7:29 PM Justin Forbes <[email protected]> wrote:
>
> On Mon, Nov 15, 2021 at 7:04 PM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > From: Miklos Szeredi <[email protected]>
> >
> > commit 712a951025c0667ff00b25afc360f74e639dfabe upstream.
> >
> > It is possible to trigger a crash by splicing anon pipe bufs to the fuse
> > device.
> >
> > The reason for this is that anon_pipe_buf_release() will reuse buf->page if
> > the refcount is 1, but that page might have already been stolen and its
> > flags modified (e.g. PG_lru added).
> >
> > This happens in the unlikely case of fuse_dev_splice_write() getting around
> > to calling pipe_buf_release() after a page has been stolen, added to the
> > page cache and removed from the page cache.
> >
> > Fix by calling pipe_buf_release() right after the page was inserted into
> > the page cache. In this case the page has an elevated refcount so any
> > release function will know that the page isn't reusable.
> >
> > Reported-by: Frank Dinoff <[email protected]>
> > Link: https://lore.kernel.org/r/CAAmZXrsGg2xsP1CK+cbuEMumtrqdvD-NKnWzhNcvn71RV3c1yw@mail.gmail.com/
> > Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
> > Cc: <[email protected]> # v2.6.35
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> It appears this patch causes a rather serious regression in flatpacks
> using portals to access files. Reverting this patch restores expected
> behavior. I have asked users in the Fedora bug to test with 5.16-rc2
> to see if we are just missing a dependent patch in stable, or if this
> is broken there as well, but no response yet.:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2025285
> https://github.com/flatpak/flatpak/issues/4595
Hi,
Thanks for the report. Can someone with the reproducer try the attached patch?
I think the race there is unlikely but possible.
Thanks,
Miklos
On Tue, Nov 23, 2021 at 1:22 PM Miklos Szeredi <[email protected]> wrote:
>
> On Tue, Nov 23, 2021 at 7:29 PM Justin Forbes <[email protected]> wrote:
> >
> > On Mon, Nov 15, 2021 at 7:04 PM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > From: Miklos Szeredi <[email protected]>
> > >
> > > commit 712a951025c0667ff00b25afc360f74e639dfabe upstream.
> > >
> > > It is possible to trigger a crash by splicing anon pipe bufs to the fuse
> > > device.
> > >
> > > The reason for this is that anon_pipe_buf_release() will reuse buf->page if
> > > the refcount is 1, but that page might have already been stolen and its
> > > flags modified (e.g. PG_lru added).
> > >
> > > This happens in the unlikely case of fuse_dev_splice_write() getting around
> > > to calling pipe_buf_release() after a page has been stolen, added to the
> > > page cache and removed from the page cache.
> > >
> > > Fix by calling pipe_buf_release() right after the page was inserted into
> > > the page cache. In this case the page has an elevated refcount so any
> > > release function will know that the page isn't reusable.
> > >
> > > Reported-by: Frank Dinoff <[email protected]>
> > > Link: https://lore.kernel.org/r/CAAmZXrsGg2xsP1CK+cbuEMumtrqdvD-NKnWzhNcvn71RV3c1yw@mail.gmail.com/
> > > Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
> > > Cc: <[email protected]> # v2.6.35
> > > Signed-off-by: Miklos Szeredi <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >
> > It appears this patch causes a rather serious regression in flatpacks
> > using portals to access files. Reverting this patch restores expected
> > behavior. I have asked users in the Fedora bug to test with 5.16-rc2
> > to see if we are just missing a dependent patch in stable, or if this
> > is broken there as well, but no response yet.:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=2025285
> > https://github.com/flatpak/flatpak/issues/4595
>
> Hi,
>
> Thanks for the report. Can someone with the reproducer try the attached patch?
>
> I think the race there is unlikely but possible.
>
Thanks, did a scratch build for that and dropped it in the bug. Only
one user has reported back, but the report was that it did not fix the
issue. I have also gotten confirmation now that the issue is occuring
with 5.16-rc2.
Thanks,
Justin
On Wed, Nov 24, 2021 at 1:40 AM Justin Forbes <[email protected]> wrote:
> Thanks, did a scratch build for that and dropped it in the bug. Only
> one user has reported back, but the report was that it did not fix the
> issue. I have also gotten confirmation now that the issue is occuring
> with 5.16-rc2.
Okay.
Morning light brings clarity to the mind. Here's a patch that should
definitely fix this bug, as well as the very unlikely race of the page
being truncated from the page cache before pipe_buf_release() is
called.
Please test.
Thanks,
Miklos
On Wed, Nov 24, 2021 at 3:23 AM Miklos Szeredi <[email protected]> wrote:
>
> On Wed, Nov 24, 2021 at 1:40 AM Justin Forbes <[email protected]> wrote:
> > Thanks, did a scratch build for that and dropped it in the bug. Only
> > one user has reported back, but the report was that it did not fix the
> > issue. I have also gotten confirmation now that the issue is occuring
> > with 5.16-rc2.
>
> Okay.
>
> Morning light brings clarity to the mind. Here's a patch that should
> definitely fix this bug, as well as the very unlikely race of the page
> being truncated from the page cache before pipe_buf_release() is
> called.
>
> Please test.
Thanks, did a scratch build. and multiple users have reported back
saying that this patch does in fact fix the issue.
Justin