Received-SPF: pass (google.com: domain of linux-kernel+bounces-116456-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 25 Mar 2024 03:22:30 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Zhaoyang Huang <huangzhaoyang@gmail.com>
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>
Subject: Re: summarize all information again at bottom//reply: reply: [PATCH]
 mm: fix a race scenario in folio_isolate_lru
Message-ID: <ZgDt9mwN-Py5Y-xr@casper.infradead.org>
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>
 <CAGWkznFtez1fj2L2CtFnA5k-Tn4WtxmDOw=fjOWPg-ZGJX=VWw@mail.gmail.com>
 <Zfz4_GJAHRInB8ul@casper.infradead.org>
 <CAGWkznEzpcWi4oXVn_WFahnQj3dHcwc_4VW6m1Ss-KJ8mD3F3Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGWkznEzpcWi4oXVn_WFahnQj3dHcwc_4VW6m1Ss-KJ8mD3F3Q@mail.gmail.com>

On Sun, Mar 24, 2024 at 07:14:27PM +0800, Zhaoyang Huang wrote:
> ok. It seems like madvise is robust enough to leave no BUGs. I presume
> another two scenarios which call folio_isloate_lru by any other ways
> but PTE. Besides, scenario 2 reminds me of a previous bug reported by
> me as find_get_entry entered in a livelock where the folio's refcnt ==
> 0 but remains at xarray which causes the reset->retry loops forever. I
> would like to reply in that thread for more details.
> 
> Scenario 1:
> 0. Thread_bad gets the folio by find_get_entry and preempted before
> folio_lock (could be the second round scan of
> truncate_inode_pages_range)
>     refcnt == 2(page_cache, fbatch_bad), PG_lru == true, PG_lock == false
>         folio = find_get_entry
>         folio_try_get_rcu
>         <preempted>
>         folio_try_lock
> 
> 1. Thread_truncate get the folio via
> truncate_inode_pages_range->find_lock_entries
>     refcnt == 3(page_cache, fbatch_bad, fbatch_truncate), PG_lru ==
> true, PG_lock == true

Hang on, you can't have two threads in truncate_inode_pages_range()
at the same time.  I appreciate that we don't have any documentation
of that, but if it were possible, we'd see other crashes.  Removing
the folio from the page cache sets folio->mapping to NULL.  And
__filemap_remove_folio() uses folio->mapping in
filemap_unaccount_folio() and page_cache_delete(), so we'd get NULL
pointer dereferences.

I see a hint in the DAX code that it's an fs-dependent lock:

        /*
         * This gets called from truncate / punch_hole path. As such, the caller
         * must hold locks protecting against concurrent modifications of the
         * page cache (usually fs-private i_mmap_sem for writing). Since the
         * caller has seen a DAX entry for this index, we better find it
         * at that index as well...
         */

so maybe that's why there's no lockdep_assert() in
truncate_inode_pages_range(), but there should be a comment.

> Scenario 2:
> 0. Thread_bad gets the folio by find_get_entry and preempted before
> folio_lock (could be the second round scan of
> truncate_inode_pages_range)
>     refcnt == 2(page_cache, fbatch_bad), PG_lru == true, PG_lock == false
>         folio = find_get_entry
>         folio_try_get_rcu
>         <preempted>
>         folio_try_lock
> 
> 1. Thread_readahead remove the folio from page cache and drop one
> refcnt by filemap_remove_folio(get rid of the folios which failed to
> launch IO during readahead)
>     refcnt == 1(fbatch_bad), PG_lru == true, PG_lock == true

So readaahead inserts the folio locked, and then calls
filemap_remove_folio() without having unlocked it.
filemap_remove_folio() sets folio->mapping to NULL in
page_cache_delete().  When "Thread_bad" wakes up, it gets the
folio lock, calls truncate_inode_folio() and sees that
folio->mapping != mapping, so it doesn't call filemap_remove_folio().

> 4. Thread_bad schedule back from step 0 and clear one refcnt wrongly
> when doing truncate_inode_folio->filemap_remove_folio as it take this
> refcnt as the page cache one
>     refcnt == 1'(thread_isolate), PG_lru == false, PG_lock == false
>         find_get_entries
>             folio = find_get_entry
>             folio_try_get_rcu
>         folio_lock
>         <no check as folio->mapping != mapping as folio_lock_entries does>
>         truncate_inode_folio
>             filemap_remove_folio
>             <preempted>