Re: [PATCH] mm: Call ->free_folio() directly in folio_unmap_invalidate()

[Jan Kara <jack@suse.cz>]

On Mon 13-04-26 19:43:11, Matthew Wilcox (Oracle) wrote:
> We can only call filemap_free_folio() if we have a reference to (or hold a
> lock on) the mapping.  Otherwise, we've already removed the folio from the
> mapping so it no longer pins the mapping and the mapping can be removed,
> causing a use-after-free when accessing mapping->a_ops.
> 
> Follow the same pattern as __remove_mapping() and load the free_folio
> function pointer before dropping the lock on the mapping.  That lets
> us make filemap_free_folio() static as this was the only caller outside
> filemap.c.
> 
> Fixes: 4a9e23159fd3 (mm/truncate: add folio_unmap_invalidate() helper)
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: stable@vger.kernel.org
> Reported-by: Google Big Sleep <big-sleep-vuln-reports+bigsleep-501448199@google.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

The fix looks good to me. Regarding the Fixes tag, Christoph is right that
at that point and even for some time after that folio_unmap_invalidate()
was fine as it was only called when holding inode reference. It was more
like fb7d3bc41493 ("mm/filemap: drop streaming/uncached pages when
writeback completes") when the problem started.

								Honza

> ---
>  mm/filemap.c  | 3 ++-
>  mm/internal.h | 1 -
>  mm/truncate.c | 6 +++++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 406cef06b684..5a4fecb24257 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -228,7 +228,8 @@ void __filemap_remove_folio(struct folio *folio, void *shadow)
>  	page_cache_delete(mapping, folio, shadow);
>  }
>  
> -void filemap_free_folio(struct address_space *mapping, struct folio *folio)
> +static void filemap_free_folio(const struct address_space *mapping,
> +		struct folio *folio)
>  {
>  	void (*free_folio)(struct folio *);
>  
> diff --git a/mm/internal.h b/mm/internal.h
> index cb0af847d7d9..546114d3ee44 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -540,7 +540,6 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
>  		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices);
>  unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
>  		pgoff_t end, struct folio_batch *fbatch, pgoff_t *indices);
> -void filemap_free_folio(struct address_space *mapping, struct folio *folio);
>  int truncate_inode_folio(struct address_space *mapping, struct folio *folio);
>  bool truncate_inode_partial_folio(struct folio *folio, loff_t start,
>  		loff_t end);
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 12467c1bd711..8617a12cb169 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -622,6 +622,7 @@ static int folio_launder(struct address_space *mapping, struct folio *folio)
>  int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
>  			   gfp_t gfp)
>  {
> +	void (*free_folio)(struct folio *);
>  	int ret;
>  
>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> @@ -648,9 +649,12 @@ int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
>  	xa_unlock_irq(&mapping->i_pages);
>  	if (mapping_shrinkable(mapping))
>  		inode_lru_list_add(mapping->host);
> +	free_folio = mapping->a_ops->free_folio;
>  	spin_unlock(&mapping->host->i_lock);
>  
> -	filemap_free_folio(mapping, folio);
> +	if (free_folio)
> +		free_folio(folio);
> +	folio_put_refs(folio, folio_nr_pages(folio));
>  	return 1;
>  failed:
>  	xa_unlock_irq(&mapping->i_pages);
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR