Re: [PATCH 1/3] xfs: Remove xfs_filemap_map_pages() wrapper

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 08, 2023 at 02:53:33PM +0000, Matthew Wilcox (Oracle) wrote:
> XFS doesn't actually need to be holding the XFS_MMAPLOCK_SHARED
> to do this, any more than it needs the XFS_MMAPLOCK_SHARED for a
> read() that hits in the page cache.

Hmm.  From commit cd647d5651c0 ("xfs: use MMAPLOCK around
filemap_map_pages()"):

    The page faultround path ->map_pages is implemented in XFS via
    filemap_map_pages(). This function checks that pages found in page
    cache lookups have not raced with truncate based invalidation by
    checking page->mapping is correct and page->index is within EOF.

    However, we've known for a long time that this is not sufficient to
    protect against races with invalidations done by operations that do
    not change EOF. e.g. hole punching and other fallocate() based
    direct extent manipulations. The way we protect against these
    races is we wrap the page fault operations in a XFS_MMAPLOCK_SHARED
    lock so they serialise against fallocate and truncate before calling
    into the filemap function that processes the fault.

    Do the same for XFS's ->map_pages implementation to close this
    potential data corruption issue.

How do we prevent faultaround from racing with fallocate and reflink
calls that operate below EOF?

--D

> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/xfs/xfs_file.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 705250f9f90a..528fc538b6b9 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1388,25 +1388,10 @@ xfs_filemap_pfn_mkwrite(
>  	return __xfs_filemap_fault(vmf, PE_SIZE_PTE, true);
>  }
>  
> -static vm_fault_t
> -xfs_filemap_map_pages(
> -	struct vm_fault		*vmf,
> -	pgoff_t			start_pgoff,
> -	pgoff_t			end_pgoff)
> -{
> -	struct inode		*inode = file_inode(vmf->vma->vm_file);
> -	vm_fault_t ret;
> -
> -	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> -	ret = filemap_map_pages(vmf, start_pgoff, end_pgoff);
> -	xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> -	return ret;
> -}
> -
>  static const struct vm_operations_struct xfs_file_vm_ops = {
>  	.fault		= xfs_filemap_fault,
>  	.huge_fault	= xfs_filemap_huge_fault,
> -	.map_pages	= xfs_filemap_map_pages,
> +	.map_pages	= filemap_map_pages,
>  	.page_mkwrite	= xfs_filemap_page_mkwrite,
>  	.pfn_mkwrite	= xfs_filemap_pfn_mkwrite,
>  };
> -- 
> 2.35.1
>