Re: [PATCH] mm: fix a race scenario in folio_isolate_lru
From: Matthew Wilcox
Date: Sun Mar 17 2024 - 16:46:59 EST
On Sun, Mar 17, 2024 at 12:07:40PM +0800, Zhaoyang Huang wrote:
> Could it be this scenario, where folio comes from pte(thread 0), local
> fbatch(thread 1) and page cache(thread 2) concurrently and proceed
> intermixed without lock's protection? Actually, IMO, thread 1 also
> could see the folio with refcnt==1 since it doesn't care if the page
> is on the page cache or not.
>
> madivise_cold_and_pageout does no explicit folio_get thing since the
> folio comes from pte which implies it has one refcnt from pagecache
Mmm, no. It's implicit, but madvise_cold_or_pageout_pte_range()
does guarantee that the folio has at least one refcount.
Since we get the folio from vm_normal_folio(vma, addr, ptent);
we know that there is at least one mapcount on the folio. refcount
is always >= mapcount. Since we hold pte_offset_map_lock(), we
know that mapcount (and therefore refcount) cannot be decremented
until we call pte_unmap_unlock(), which we don't do until we have
called folio_isolate_lru().
Good try though, took me a few minutes of looking at it to convince
myself that it was safe.
Something to bear in mind is that if the race you outline is real,
failing to hold a refcount on the folio leaves the caller susceptible
to the VM_BUG_ON_FOLIO(!folio_ref_count(folio), folio); if the other
thread calls folio_put().
I can't understand any of the scenarios you outline below.
Please try again without relying on indentation.
> #thread 0(madivise_cold_and_pageout) #1
> (lru_add_drain->fbatch_release_pages)
> #2(read_pages->filemap_remove_folios)
> refcnt == 1(represent page cache)
>
> refcnt==2(another one represent LRU)
> folio comes from page cache
> folio_isolate_lru
> release_pages
> filemap_free_folio
>
>
> refcnt==1(decrease the one of page cache)
>
> folio_put_testzero == true
>
> <No lruvec_del_folio>
>
> list_add(folio->lru, pages_to_free) //current folio will break LRU's
> integrity since it has not been deleted
>
> In case of gmail's wrap, split above chart to two parts
>
> #thread 0(madivise_cold_and_pageout) #1
> (lru_add_drain->fbatch_release_pages)
> refcnt == 1(represent page cache)
>
> refcnt==2(another one represent LRU)
> folio_isolate_lru release_pages
>
> folio_put_testzero == true
>
> <No lruvec_del_folio>
>
> list_add(folio->lru, pages_to_free)
>
> //current folio will break LRU's integrity since it has not been
> deleted
>
> #1 (lru_add_drain->fbatch_release_pages)
> #2(read_pages->filemap_remove_folios)
> refcnt==2(another one represent LRU)
> folio comes from page cache
> release_pages
> filemap_free_folio
>
> refcnt==1(decrease the one of page cache)
> folio_put_testzero == true
> <No lruvec_del_folio>
> list_add(folio->lru, pages_to_free)
> //current folio will break LRU's integrity since it has not been deleted
> >
> > > #0 folio_isolate_lru #1 release_pages
> > > BUG_ON(!folio_refcnt)
> > > if (folio_put_testzero())
> > > folio_get(folio)
> > > if (folio_test_clear_lru())