Received-SPF: pass (google.com: domain of linux-kernel+bounces-110864-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <be75859d7e4245939d825da5e881902c@BJMBX01.spreadtrum.com>
 <Zfg0WLrcOmCtdn_M@casper.infradead.org> <CAGWkznGhiuzkqfeewNq-ykKehvFTBjH2v_==xAS2_7iFqsFk5A@mail.gmail.com>
 <Zfj_-LbiIsAVWcPX@casper.infradead.org> <CAGWkznHOWVPpKHOuvvDF2zppkTT0_x4WyJ5S75j+EghhZmEDdg@mail.gmail.com>
 <ZfwpuRjAZV07_lc3@casper.infradead.org>
In-Reply-To: <ZfwpuRjAZV07_lc3@casper.infradead.org>
From: Zhaoyang Huang <huangzhaoyang@gmail.com>
Date: Fri, 22 Mar 2024 09:52:36 +0800
Message-ID: <CAGWkznFtez1fj2L2CtFnA5k-Tn4WtxmDOw=fjOWPg-ZGJX=VWw@mail.gmail.com>
Subject: Re: summarize all information again at bottom//reply: reply: [PATCH]
 mm: fix a race scenario in folio_isolate_lru
To: Matthew Wilcox <willy@infradead.org>
Cc: =?UTF-8?B?6buE5pyd6ZizIChaaGFveWFuZyBIdWFuZyk=?= <zhaoyang.huang@unisoc.com>, 
	Andrew Morton <akpm@linux-foundation.org>, "linux-mm@kvack.org" <linux-mm@kvack.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	=?UTF-8?B?5bq357qq5ruoIChTdGV2ZSBLYW5nKQ==?= <Steve.Kang@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 21, 2024 at 8:36=E2=80=AFPM Matthew Wilcox <willy@infradead.org=
> wrote:
>
> On Thu, Mar 21, 2024 at 04:25:07PM +0800, Zhaoyang Huang wrote:
> > ok. Could the scenario below be suspicious on leaving an orphan folio
> > in step 7 and introduce the bug in step 8. In the scenario,
> > Thread_filemap behaves as a backdoor for Thread_madv by creating the
> > pte after Thread_truncate finishes cleaning all page tables.
> >
> > 0. Thread_bad gets the folio by folio_get_entry and stores it in its
> > local fbatch_bad and go to sleep
>
> There's no function called folio_get_entry(), but clearly thread_bad
> should have a refcount on it at this point.
>
> > 1. Thread_filemap get the folio via
> > filemap_map_pages->next_uptodate_folio->xas_next_entry and gets
> > preempted
> >     refcnt =3D=3D 1(page_cache), PG_lru =3D=3D true
>
> so the refcount should be 2 here.
>
> > 2. Thread_truncate get the folio via
> > truncate_inode_pages_range->find_lock_entries
> >     refcnt =3D=3D 2(fbatch_trunc, page_cache), PG_lru =3D=3D true
> >
> > 3. Thread_truncate proceed to truncate_cleanup_folio
> >     refcnt =3D=3D 2(fbatch_trunc, page_cache), PG_lru =3D=3D true
> >
> > 4. Thread_truncate proceed to delete_from_page_cache_batch
> >     refcnt =3D=3D 1(fbatch_trunc), PG_lru =3D=3D true
> >
> > 5. Thread_filemap schedule back and proceed to setup a pte and have
> > folio->_mapcnt =3D 0 & folio->refcnt +=3D 1
> >     refcnt =3D=3D 2(pte, fbatch_temp), PG_lru =3D=3D true
> >
> > 6. Thread_madv clear folio's PG_lru by
> > madvise_xxx_pte_range->folio_isolate_lru->folio_test_clear_lru
> >     refcnt =3D=3D 2(pte,fbatch_temp), PG_lru =3D=3D false
> >
> > 7. Thread_truncate call folio_fbatch_release and failed in freeing
> > folio as refcnt not reach 0
> >     refcnt =3D=3D 1(pte), PG_lru =3D=3D false
> > ********folio becomes an orphan here which is not on the page cache
> > but on the task's VM**********
> >
> > 8. Thread_xxx scheduled back from 0 to do release_pages(fbatch_bad)
> > and have the folio introduce the bug.
>
> ... because if these steps happen as 7, 8, 6, you hit the BUG in
> folio_isolate_lru().
Thanks for the comments. fix the typo and update the timing sequence
by amending possible preempt points to have the refcnt make sense.

0. Thread_bad gets the folio by find_get_entry and preempted before
take refcnt(could be the second round scan of
truncate_inode_pages_range)
    refcnt =3D=3D 1(page_cache), PG_lru =3D=3D true, PG_lock =3D=3D false
    find_get_entry
        folio =3D xas_find
        <preempted>
        folio_try_get_rcu

1. Thread_filemap get the folio via
filemap_map_pages->next_uptodate_folio->xas_next_entry and gets preempted
    refcnt =3D=3D 1(page_cache), PG_lru =3D=3D true, PG_lock =3D=3D false
    filemap_map_pages
        next_uptodate_folio
           xas_next_entry
           <preempted>
           folio_try_get_rcu

2. Thread_truncate get the folio via
truncate_inode_pages_range->find_lock_entries
    refcnt =3D=3D 2(page_cache, fbatch_truncate), PG_lru =3D=3D true, PG_lo=
ck =3D=3D true

3. Thread_truncate proceed to truncate_cleanup_folio
    refcnt =3D=3D 2(page_cache, fbatch_truncate), PG_lru =3D=3D true, PG_lo=
ck =3D=3D true

4. Thread_truncate proceed to delete_from_page_cache_batch
    refcnt =3D=3D 1(fbatch_truncate), PG_lru =3D=3D true, PG_lock =3D=3D tr=
ue

4.1 folio_unlock
    refcnt =3D=3D 1(fbatch_truncate), PG_lru =3D=3D true, PG_lock =3D=3D fa=
lse

5. Thread_filemap schedule back from '1' and proceed to setup a pte
and have folio->_mapcnt =3D 0 & folio->refcnt +=3D 1
    refcnt =3D=3D 1->2(+fbatch_filemap)->3->2(pte, fbatch_truncate),
PG_lru =3D=3D true, PG_lock =3D=3D true->false

6. Thread_madv clear folio's PG_lru by
madvise_xxx_pte_range->folio_isolate_lru->folio_test_clear_lru
    refcnt =3D=3D 2(pte,fbatch_truncate), PG_lru =3D=3D false, PG_lock =3D=
=3D false

7. Thread_truncate call folio_fbatch_release and failed in freeing
folio as refcnt not reach 0
    refcnt =3D=3D 1(pte), PG_lru =3D=3D false, PG_lock =3D=3D false
********folio becomes an orphan here which is not on the page cache
but on the task's VM**********

8. Thread_bad scheduled back from '0' to be collected in fbatch_bad
    refcnt =3D=3D 2(pte, fbatch_bad), PG_lru =3D=3D false, PG_lock =3D=3D t=
rue

9. Thread_bad clear one refcnt wrongly when doing filemap_remove_folio
as it take this refcnt as the page cache one
    refcnt =3D=3D 1(fbatch_bad), PG_lru =3D=3D false, PG_lock =3D=3D true->=
false
    truncate_inode_folio
        filemap_remove_folio
             filemap_free_folio
******refcnt decreased wrongly here by being taken as the page cache one **=
****

10. Thread_bad calls release_pages(fbatch_bad) and has the folio
introduce the bug.
    release_pages
        folio_put_testzero =3D=3D true
        folio_test_lru =3D=3D false
        list_add(folio->lru, pages_to_free)