linux-kernel - Re: [PATCH] mm: Fix error handling in __filemap_get_folio() with FGP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKhLTr0UA42AC2yCyFtDbFoS34vvg05EVnf5J4MSit_Sr7JETw@mail.gmail.com>
Date: Mon, 24 Feb 2025 04:59:48 -0300
From: "Raphael S. Carvalho" <raphaelsc@...lladb.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org, 
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, djwong@...nel.org, 
	Dave Chinner <david@...morbit.com>, hch@....de
Subject: Re: [PATCH] mm: Fix error handling in __filemap_get_folio() with FGP_NOWAIT

On Mon, Feb 24, 2025 at 1:14 AM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Sun, Feb 23, 2025 at 08:57:19PM -0300, Raphael S. Carvalho wrote:
> > This is likely a regression caused by 66dabbb65d67 ("mm: return an ERR_PTR
> > from __filemap_get_folio"), which performed the following changes:
> >     --- a/fs/iomap/buffered-io.c
> >     +++ b/fs/iomap/buffered-io.c
> >     @@ -468,19 +468,12 @@ EXPORT_SYMBOL_GPL(iomap_is_partially_uptodate);
> >     struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos)
> >     {
> >             unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS;
> >     -       struct folio *folio;
> >
> >             if (iter->flags & IOMAP_NOWAIT)
> >                     fgp |= FGP_NOWAIT;
> >
> >     -       folio = __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
> >     +       return __filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT,
> >                             fgp, mapping_gfp_mask(iter->inode->i_mapping));
> >     -       if (folio)
> >     -               return folio;
> >     -
> >     -       if (iter->flags & IOMAP_NOWAIT)
> >     -               return ERR_PTR(-EAGAIN);
> >     -       return ERR_PTR(-ENOMEM);
> >     }
>
> We don't usually put this in the changelog ...
>
> > Essentially, that patch is moving error picking decision to
> > __filemap_get_folio, but it missed proper FGP_NOWAIT handling, so ENOMEM
> > is being escaped to user space. Had it correctly returned -EAGAIN with NOWAIT,
> > either io_uring or user space itself would be able to retry the request.
> > It's not enough to patch io_uring since the iomap interface is the one
> > responsible for it, and pwritev2(RWF_NOWAIT) and AIO interfaces must return
> > the proper error too.
> >
> > The patch was tested with scylladb test suite (its original reproducer), and
> > the tests all pass now when memory is pressured.
> >
> > Signed-off-by: Raphael S. Carvalho <raphaelsc@...lladb.com>
>
> Instead, we add:
>
> Fixes: 66dabbb65d67 (mm: return an ERR_PTR from __filemap_get_folio)

Thanks, will fix it in v2.

>
> > ---
> >  mm/filemap.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 804d7365680c..b06bd6eedaf7 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1986,8 +1986,15 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
> >
> >               if (err == -EEXIST)
> >                       goto repeat;
> > -             if (err)
> > +             if (err) {
> > +                     /*
> > +                      * Presumably ENOMEM, either from when allocating or
> > +                      * adding folio (this one for xarray node)
> > +                      */
>
> I don't like the comment.  Better to do that in code:
>

Initially I was doing exactly what you proposed above, but after
reading do_read_cache_folio() and the patch the introduces the
regression, which transforms failure to get a folio (a NULL) with
FGP_NOWAIT into NOAGAIN, I decided to do this, but it's indeed better
to remove assumptions. Not ideal for the long run. Will change in v2.
thanks.