From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
To: David Wysochanski <dwysocha@redhat.com>
Cc: David Howells <dhowells@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
Matthew Wilcox <willy@infradead.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Jeff Layton <jlayton@kernel.org>,
Christoph Hellwig <hch@infradead.org>,
linux-afs@lists.infradead.org, linux-nfs@vger.kernel.org,
linux-cifs@vger.kernel.org, ceph-devel@vger.kernel.org,
v9fs-developer@lists.sourceforge.net,
linux-erofs@lists.ozlabs.org, linux-ext4@vger.kernel.org,
linux-cachefs@redhat.com, linux-fsdevel@vger.kernel.org,
Rohith Surabattula <rohiths.msft@gmail.com>,
Steve French <sfrench@samba.org>,
Shyam Prasad N <nspmangalore@gmail.com>,
Dominique Martinet <asmadeus@codewreck.org>,
Ilya Dryomov <idryomov@gmail.com>,
linux-mm@kvack.org, Daire Byrne <daire.byrne@gmail.com>
Subject: Re: [BUG mm-unstable] BUG: KASAN: use-after-free in shrink_folio_list+0x9f4/0x1ae0
Date: Sat, 8 Jul 2023 03:27:42 +0900 [thread overview]
Message-ID: <ZKhZHg6LSGnvryIe@fedora> (raw)
In-Reply-To: <CALF+zO=nGdoxcT-ya3aaUCBi-4iKPo3kZyzcWYCKMCf4n2wVbA@mail.gmail.com>
On Fri, Jul 07, 2023 at 02:12:06PM -0400, David Wysochanski wrote:
> On Fri, Jul 7, 2023 at 12:46 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> >
> > On Sat, Jul 8, 2023 at 1:39 AM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> > >
> > > On Wed, Jun 28, 2023 at 11:48:52AM +0100, David Howells wrote:
> > > > Fscache has an optimisation by which reads from the cache are skipped until
> > > > we know that (a) there's data there to be read and (b) that data isn't
> > > > entirely covered by pages resident in the netfs pagecache. This is done
> > > > with two flags manipulated by fscache_note_page_release():
> > > >
> > > > if (...
> > > > test_bit(FSCACHE_COOKIE_HAVE_DATA, &cookie->flags) &&
> > > > test_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags))
> > > > clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
> > > >
> > > > where the NO_DATA_TO_READ flag causes cachefiles_prepare_read() to indicate
> > > > that netfslib should download from the server or clear the page instead.
> > > >
> > > > The fscache_note_page_release() function is intended to be called from
> > > > ->releasepage() - but that only gets called if PG_private or PG_private_2
> > > > is set - and currently the former is at the discretion of the network
> > > > filesystem and the latter is only set whilst a page is being written to the
> > > > cache, so sometimes we miss clearing the optimisation.
> > > >
> > > > Fix this by following Willy's suggestion[1] and adding an address_space
> > > > flag, AS_RELEASE_ALWAYS, that causes filemap_release_folio() to always call
> > > > ->release_folio() if it's set, even if PG_private or PG_private_2 aren't
> > > > set.
> > > >
> > > > Note that this would require folio_test_private() and page_has_private() to
> > > > become more complicated. To avoid that, in the places[*] where these are
> > > > used to conditionalise calls to filemap_release_folio() and
> > > > try_to_release_page(), the tests are removed the those functions just
> > > > jumped to unconditionally and the test is performed there.
> > > >
> > > > [*] There are some exceptions in vmscan.c where the check guards more than
> > > > just a call to the releaser. I've added a function, folio_needs_release()
> > > > to wrap all the checks for that.
> > > >
> > > > AS_RELEASE_ALWAYS should be set if a non-NULL cookie is obtained from
> > > > fscache and cleared in ->evict_inode() before truncate_inode_pages_final()
> > > > is called.
> > > >
> > > > Additionally, the FSCACHE_COOKIE_NO_DATA_TO_READ flag needs to be cleared
> > > > and the optimisation cancelled if a cachefiles object already contains data
> > > > when we open it.
> > > >
> > > > Fixes: 1f67e6d0b188 ("fscache: Provide a function to note the release of a page")
> > > > Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
> > > > Reported-by: Rohith Surabattula <rohiths.msft@gmail.com>
> > > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > >
> > > Hi David,
> > >
> > > I was bisecting a use-after-free BUG on the latest mm-unstable,
> > > where HEAD is 347e208de0e4 ("rmap: pass the folio to __page_check_anon_rmap()").
> > >
> > > According to my bisection, this is the first bad commit.
> > > Use-After-Free is triggered on reclamation path when swap is enabled.
> >
> > This was originally occurred during kernel compilation but
> > can easily be reproduced via:
> >
> > stress-ng --bigheap $(nproc)
> >
> > > (and couldn't trigger without swap enabled)
> > >
> > > the config, KASAN splat, bisect log are attached.
> > > hope this isn't too late :(
> > >
> > > > cc: Matthew Wilcox <willy@infradead.org>
> > > > cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > > cc: Steve French <sfrench@samba.org>
> > > > cc: Shyam Prasad N <nspmangalore@gmail.com>
> > > > cc: Rohith Surabattula <rohiths.msft@gmail.com>
> > > > cc: Dave Wysochanski <dwysocha@redhat.com>
> > > > cc: Dominique Martinet <asmadeus@codewreck.org>
> > > > cc: Ilya Dryomov <idryomov@gmail.com>
> > > > cc: linux-cachefs@redhat.com
> > > > cc: linux-cifs@vger.kernel.org
> > > > cc: linux-afs@lists.infradead.org
> > > > cc: v9fs-developer@lists.sourceforge.net
> > > > cc: ceph-devel@vger.kernel.org
> > > > cc: linux-nfs@vger.kernel.org
> > > > cc: linux-fsdevel@vger.kernel.org
> > > > cc: linux-mm@kvack.org
> > > > ---
> > > >
> > > > Notes:
> > > > ver #7)
> > > > - Make NFS set AS_RELEASE_ALWAYS.
> > > >
> > > > ver #4)
> > > > - Split out merging of folio_has_private()/filemap_release_folio() call
> > > > pairs into a preceding patch.
> > > > - Don't need to clear AS_RELEASE_ALWAYS in ->evict_inode().
> > > >
> > > > ver #3)
> > > > - Fixed mapping_clear_release_always() to use clear_bit() not set_bit().
> > > > - Moved a '&&' to the correct line.
> > > >
> > > > ver #2)
> > > > - Rewrote entirely according to Willy's suggestion[1].
> > > >
> > > > fs/9p/cache.c | 2 ++
> > > > fs/afs/internal.h | 2 ++
> > > > fs/cachefiles/namei.c | 2 ++
> > > > fs/ceph/cache.c | 2 ++
> > > > fs/nfs/fscache.c | 3 +++
> > > > fs/smb/client/fscache.c | 2 ++
> > > > include/linux/pagemap.h | 16 ++++++++++++++++
> > > > mm/internal.h | 5 ++++-
> > > > 8 files changed, 33 insertions(+), 1 deletion(-)
>
>
> I think myself / Daire Byrne may have already tracked this down and I
> found a 1-liner that fixed a similar crash in his environment.
>
> Can you try this patch on top and let me know if it still crashes?
> https://github.com/DaveWysochanskiRH/kernel/commit/902c990e311120179fa5de99d68364b2947b79ec
Oh, it does not crash with the patch applied.
Hmm, was it UAF because it references wrong field ->mapping,
instead of swapper address space?
WARNING: multiple messages have this Message-ID (diff)
From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
To: David Wysochanski <dwysocha@redhat.com>
Cc: Dominique Martinet <asmadeus@codewreck.org>,
David Howells <dhowells@redhat.com>,
linux-mm@kvack.org, linux-afs@lists.infradead.org,
Shyam Prasad N <nspmangalore@gmail.com>,
linux-cifs@vger.kernel.org, Matthew Wilcox <willy@infradead.org>,
Christoph Hellwig <hch@infradead.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
linux-cachefs@redhat.com, v9fs-developer@lists.sourceforge.net,
Ilya Dryomov <idryomov@gmail.com>,
linux-ext4@vger.kernel.org, ceph-devel@vger.kernel.org,
linux-nfs@vger.kernel.org,
Rohith Surabattula <rohiths.msft@gmail.com>,
Daire Byrne <daire.byrne@gmail.com>,
Jeff Layton <jlayton@kernel.org>,
Steve French <sfrench@samba.org>,
linux-fsdevel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
linux-erofs@lists.ozlabs.org
Subject: Re: [BUG mm-unstable] BUG: KASAN: use-after-free in shrink_folio_list+0x9f4/0x1ae0
Date: Sat, 8 Jul 2023 03:27:42 +0900 [thread overview]
Message-ID: <ZKhZHg6LSGnvryIe@fedora> (raw)
In-Reply-To: <CALF+zO=nGdoxcT-ya3aaUCBi-4iKPo3kZyzcWYCKMCf4n2wVbA@mail.gmail.com>
On Fri, Jul 07, 2023 at 02:12:06PM -0400, David Wysochanski wrote:
> On Fri, Jul 7, 2023 at 12:46 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> >
> > On Sat, Jul 8, 2023 at 1:39 AM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> > >
> > > On Wed, Jun 28, 2023 at 11:48:52AM +0100, David Howells wrote:
> > > > Fscache has an optimisation by which reads from the cache are skipped until
> > > > we know that (a) there's data there to be read and (b) that data isn't
> > > > entirely covered by pages resident in the netfs pagecache. This is done
> > > > with two flags manipulated by fscache_note_page_release():
> > > >
> > > > if (...
> > > > test_bit(FSCACHE_COOKIE_HAVE_DATA, &cookie->flags) &&
> > > > test_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags))
> > > > clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
> > > >
> > > > where the NO_DATA_TO_READ flag causes cachefiles_prepare_read() to indicate
> > > > that netfslib should download from the server or clear the page instead.
> > > >
> > > > The fscache_note_page_release() function is intended to be called from
> > > > ->releasepage() - but that only gets called if PG_private or PG_private_2
> > > > is set - and currently the former is at the discretion of the network
> > > > filesystem and the latter is only set whilst a page is being written to the
> > > > cache, so sometimes we miss clearing the optimisation.
> > > >
> > > > Fix this by following Willy's suggestion[1] and adding an address_space
> > > > flag, AS_RELEASE_ALWAYS, that causes filemap_release_folio() to always call
> > > > ->release_folio() if it's set, even if PG_private or PG_private_2 aren't
> > > > set.
> > > >
> > > > Note that this would require folio_test_private() and page_has_private() to
> > > > become more complicated. To avoid that, in the places[*] where these are
> > > > used to conditionalise calls to filemap_release_folio() and
> > > > try_to_release_page(), the tests are removed the those functions just
> > > > jumped to unconditionally and the test is performed there.
> > > >
> > > > [*] There are some exceptions in vmscan.c where the check guards more than
> > > > just a call to the releaser. I've added a function, folio_needs_release()
> > > > to wrap all the checks for that.
> > > >
> > > > AS_RELEASE_ALWAYS should be set if a non-NULL cookie is obtained from
> > > > fscache and cleared in ->evict_inode() before truncate_inode_pages_final()
> > > > is called.
> > > >
> > > > Additionally, the FSCACHE_COOKIE_NO_DATA_TO_READ flag needs to be cleared
> > > > and the optimisation cancelled if a cachefiles object already contains data
> > > > when we open it.
> > > >
> > > > Fixes: 1f67e6d0b188 ("fscache: Provide a function to note the release of a page")
> > > > Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
> > > > Reported-by: Rohith Surabattula <rohiths.msft@gmail.com>
> > > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > >
> > > Hi David,
> > >
> > > I was bisecting a use-after-free BUG on the latest mm-unstable,
> > > where HEAD is 347e208de0e4 ("rmap: pass the folio to __page_check_anon_rmap()").
> > >
> > > According to my bisection, this is the first bad commit.
> > > Use-After-Free is triggered on reclamation path when swap is enabled.
> >
> > This was originally occurred during kernel compilation but
> > can easily be reproduced via:
> >
> > stress-ng --bigheap $(nproc)
> >
> > > (and couldn't trigger without swap enabled)
> > >
> > > the config, KASAN splat, bisect log are attached.
> > > hope this isn't too late :(
> > >
> > > > cc: Matthew Wilcox <willy@infradead.org>
> > > > cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > > cc: Steve French <sfrench@samba.org>
> > > > cc: Shyam Prasad N <nspmangalore@gmail.com>
> > > > cc: Rohith Surabattula <rohiths.msft@gmail.com>
> > > > cc: Dave Wysochanski <dwysocha@redhat.com>
> > > > cc: Dominique Martinet <asmadeus@codewreck.org>
> > > > cc: Ilya Dryomov <idryomov@gmail.com>
> > > > cc: linux-cachefs@redhat.com
> > > > cc: linux-cifs@vger.kernel.org
> > > > cc: linux-afs@lists.infradead.org
> > > > cc: v9fs-developer@lists.sourceforge.net
> > > > cc: ceph-devel@vger.kernel.org
> > > > cc: linux-nfs@vger.kernel.org
> > > > cc: linux-fsdevel@vger.kernel.org
> > > > cc: linux-mm@kvack.org
> > > > ---
> > > >
> > > > Notes:
> > > > ver #7)
> > > > - Make NFS set AS_RELEASE_ALWAYS.
> > > >
> > > > ver #4)
> > > > - Split out merging of folio_has_private()/filemap_release_folio() call
> > > > pairs into a preceding patch.
> > > > - Don't need to clear AS_RELEASE_ALWAYS in ->evict_inode().
> > > >
> > > > ver #3)
> > > > - Fixed mapping_clear_release_always() to use clear_bit() not set_bit().
> > > > - Moved a '&&' to the correct line.
> > > >
> > > > ver #2)
> > > > - Rewrote entirely according to Willy's suggestion[1].
> > > >
> > > > fs/9p/cache.c | 2 ++
> > > > fs/afs/internal.h | 2 ++
> > > > fs/cachefiles/namei.c | 2 ++
> > > > fs/ceph/cache.c | 2 ++
> > > > fs/nfs/fscache.c | 3 +++
> > > > fs/smb/client/fscache.c | 2 ++
> > > > include/linux/pagemap.h | 16 ++++++++++++++++
> > > > mm/internal.h | 5 ++++-
> > > > 8 files changed, 33 insertions(+), 1 deletion(-)
>
>
> I think myself / Daire Byrne may have already tracked this down and I
> found a 1-liner that fixed a similar crash in his environment.
>
> Can you try this patch on top and let me know if it still crashes?
> https://github.com/DaveWysochanskiRH/kernel/commit/902c990e311120179fa5de99d68364b2947b79ec
Oh, it does not crash with the patch applied.
Hmm, was it UAF because it references wrong field ->mapping,
instead of swapper address space?
next prev parent reply other threads:[~2023-07-07 18:28 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-28 10:48 [PATCH v7 0/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2023-06-28 10:48 ` David Howells
2023-06-28 10:48 ` [PATCH v7 1/2] mm: Merge folio_has_private()/filemap_release_folio() call pairs David Howells
2023-06-28 10:48 ` David Howells
2023-06-28 10:48 ` [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2023-06-28 10:48 ` David Howells
2023-06-29 0:39 ` [Linux-cachefs] " Xiubo Li
2023-06-29 0:39 ` Xiubo Li
2023-06-30 3:20 ` Jingbo Xu
2023-06-30 3:20 ` Jingbo Xu
2023-07-07 16:38 ` [BUG mm-unstable] BUG: KASAN: use-after-free in shrink_folio_list+0x9f4/0x1ae0 Hyeonggon Yoo
2023-07-07 16:38 ` Hyeonggon Yoo
2023-07-07 16:46 ` Hyeonggon Yoo
2023-07-07 16:46 ` Hyeonggon Yoo
2023-07-07 18:12 ` David Wysochanski
2023-07-07 18:12 ` David Wysochanski
2023-07-07 18:27 ` Hyeonggon Yoo [this message]
2023-07-07 18:27 ` Hyeonggon Yoo
2023-07-07 18:40 ` Matthew Wilcox
2023-07-07 18:40 ` Matthew Wilcox
2023-07-07 18:33 ` Matthew Wilcox
2023-07-07 18:33 ` Matthew Wilcox
2023-07-07 19:23 ` SeongJae Park
2023-07-07 19:23 ` SeongJae Park
2023-07-17 7:34 ` [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache kernel test robot
2023-07-17 7:34 ` kernel test robot
2023-07-17 12:43 ` David Wysochanski
2023-07-17 12:43 ` David Wysochanski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZKhZHg6LSGnvryIe@fedora \
--to=42.hyeyoo@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=asmadeus@codewreck.org \
--cc=ceph-devel@vger.kernel.org \
--cc=daire.byrne@gmail.com \
--cc=dhowells@redhat.com \
--cc=dwysocha@redhat.com \
--cc=hch@infradead.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=linux-afs@lists.infradead.org \
--cc=linux-cachefs@redhat.com \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=nspmangalore@gmail.com \
--cc=rohiths.msft@gmail.com \
--cc=sfrench@samba.org \
--cc=torvalds@linux-foundation.org \
--cc=v9fs-developer@lists.sourceforge.net \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.