[PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[Hugh Dickins]

It is not easily reproducible, but on 5.16-rc I have several times hit
the VM_BUG_ON_PAGE(PageTail(page), page) in page_cache_add_speculative():
usually from filemap_get_read_batch() for an ext4 read, yesterday from
next_uptodate_page() from filemap_map_pages() for a shmem fault.

That BUG used to be placed where page_ref_add_unless() had succeeded,
but now it is placed before folio_ref_add_unless() is attempted: that
is not safe, since it is only the acquired reference which makes the
page safe from racing THP collapse or split.

We could keep the BUG, checking PageTail only when folio_ref_try_add_rcu()
has succeeded; but I don't think it adds much value - just delete it.

Fixes: 020853b6f5ea ("mm: Add folio_try_get_rcu()")
Signed-off-by: Hugh Dickins <hughd@google.com>
---

 include/linux/pagemap.h |    1 -
 1 file changed, 1 deletion(-)

--- 5.16-rc4/include/linux/pagemap.h
+++ linux/include/linux/pagemap.h
@@ -285,7 +285,6 @@ static inline struct inode *folio_inode(
 
 static inline bool page_cache_add_speculative(struct page *page, int count)
 {
-	VM_BUG_ON_PAGE(PageTail(page), page);
 	return folio_ref_try_add_rcu((struct folio *)page, count);
 }
 

Re: [PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[Kirill A. Shutemov]

On Wed, Dec 08, 2021 at 11:19:18PM -0800, Hugh Dickins wrote:
> It is not easily reproducible, but on 5.16-rc I have several times hit
> the VM_BUG_ON_PAGE(PageTail(page), page) in page_cache_add_speculative():
> usually from filemap_get_read_batch() for an ext4 read, yesterday from
> next_uptodate_page() from filemap_map_pages() for a shmem fault.
> 
> That BUG used to be placed where page_ref_add_unless() had succeeded,
> but now it is placed before folio_ref_add_unless() is attempted: that
> is not safe, since it is only the acquired reference which makes the
> page safe from racing THP collapse or split.
> 
> We could keep the BUG, checking PageTail only when folio_ref_try_add_rcu()
> has succeeded; but I don't think it adds much value - just delete it.
> 
> Fixes: 020853b6f5ea ("mm: Add folio_try_get_rcu()")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[Matthew Wilcox]

On Wed, Dec 08, 2021 at 11:19:18PM -0800, Hugh Dickins wrote:
> It is not easily reproducible, but on 5.16-rc I have several times hit
> the VM_BUG_ON_PAGE(PageTail(page), page) in page_cache_add_speculative():
> usually from filemap_get_read_batch() for an ext4 read, yesterday from
> next_uptodate_page() from filemap_map_pages() for a shmem fault.
> 
> That BUG used to be placed where page_ref_add_unless() had succeeded,
> but now it is placed before folio_ref_add_unless() is attempted: that
> is not safe, since it is only the acquired reference which makes the
> page safe from racing THP collapse or split.
> 
> We could keep the BUG, checking PageTail only when folio_ref_try_add_rcu()
> has succeeded; but I don't think it adds much value - just delete it.

Whoops, that was careless of me.  I agree with your reasoning and patch.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

> Fixes: 020853b6f5ea ("mm: Add folio_try_get_rcu()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> 
>  include/linux/pagemap.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> --- 5.16-rc4/include/linux/pagemap.h
> +++ linux/include/linux/pagemap.h
> @@ -285,7 +285,6 @@ static inline struct inode *folio_inode(
>  
>  static inline bool page_cache_add_speculative(struct page *page, int count)
>  {
> -	VM_BUG_ON_PAGE(PageTail(page), page);
>  	return folio_ref_try_add_rcu((struct folio *)page, count);
>  }
>  

Re: [PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[Andrew Morton]

On Wed, 8 Dec 2021 23:19:18 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:

> It is not easily reproducible, but on 5.16-rc I have several times hit
> the VM_BUG_ON_PAGE(PageTail(page), page) in page_cache_add_speculative():
> usually from filemap_get_read_batch() for an ext4 read, yesterday from
> next_uptodate_page() from filemap_map_pages() for a shmem fault.
> 
> That BUG used to be placed where page_ref_add_unless() had succeeded,
> but now it is placed before folio_ref_add_unless() is attempted: that
> is not safe, since it is only the acquired reference which makes the
> page safe from racing THP collapse or split.
> 
> We could keep the BUG, checking PageTail only when folio_ref_try_add_rcu()
> has succeeded; but I don't think it adds much value - just delete it.
> 
> Fixes: 020853b6f5ea ("mm: Add folio_try_get_rcu()")
> Signed-off-by: Hugh Dickins <hughd@google.com>

I added cc:stable to this.

Re: [PATCH] mm: delete unsafe BUG from page_cache_add_speculative()

[Hugh Dickins]

On Fri, 10 Dec 2021, Andrew Morton wrote:
> On Wed, 8 Dec 2021 23:19:18 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> 
> > It is not easily reproducible, but on 5.16-rc I have several times hit
> > the VM_BUG_ON_PAGE(PageTail(page), page) in page_cache_add_speculative():
> > usually from filemap_get_read_batch() for an ext4 read, yesterday from
> > next_uptodate_page() from filemap_map_pages() for a shmem fault.
> > 
> > That BUG used to be placed where page_ref_add_unless() had succeeded,
> > but now it is placed before folio_ref_add_unless() is attempted: that
> > is not safe, since it is only the acquired reference which makes the
> > page safe from racing THP collapse or split.
> > 
> > We could keep the BUG, checking PageTail only when folio_ref_try_add_rcu()
> > has succeeded; but I don't think it adds much value - just delete it.
> > 
> > Fixes: 020853b6f5ea ("mm: Add folio_try_get_rcu()")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> I added cc:stable to this.

Thanks, but no, cc:stable not needed: the fixed commit went into 5.16-rc1,
and did not go to stable itself. There was an identical VM_BUG_ON_PAGE in
the old __page_cache_add_speculative(), but that one was correctly placed,
so there's no need for the old one to be removed.

Hugh