Re: [PATCH] xfs: use MMAPLOCK around filemap_map_pages()

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 30, 2020 at 11:26:45AM -0700, Darrick J. Wong wrote:
> On Tue, Jun 30, 2020 at 06:23:12PM +0300, Amir Goldstein wrote:
> > > /me wonders if someone could please check all the *_ops that point to
> > > generic helpers to see if we're missing obvious things like lock
> > > taking.  Particularly someone who wants to learn about xfs' locking
> > > strategy; I promise it won't let out a ton of bees.
> > >
> > 
> > The list was compiled manually by auditing 'git grep '_operations.*=' fs/xfs'
> > structs for non xfs_/iomap_/noop_ functions.
> > I am not sure if all iomap_ functions are safe in that respect, but I suppose
> > those were done recently with sufficient xfs developers review...
> 
> The iomap functions shouldn't be taking/releasing any locks at all; it's
> up to the filesystem to provide the concurrency controls.
> 
> > fs/xfs/xfs_aops.c:const struct address_space_operations
> > xfs_address_space_operations = {
> >         .error_remove_page      = generic_error_remove_page,
> > 
> > generic_error_remove_page() calls truncate_inode_page() without MMAPLOCK
> > Is that safe? not sure
> 
> /me has a funny feeling it isn't, since this does the same thing to the
> pagecache as a holepunch.

And I really can't tell, because this comes from the convoluted 
hardware memory error path. We know that path is completely screwed
up w.r.t. memory errors in pmem and DAX filesystems. Hence I think
there's a good chance it's completely screwed up for cached
file-backed page cache pages, too.

Indeed, look at the comment in me_pagecache_clean():

        /*
         * Truncation is a bit tricky. Enable it per file system for now.
         *
         * Open: to take i_mutex or not for this? Right now we don't.
         */
        return truncate_error_page(p, pfn, mapping);

the call path is:

me_pagecache_clean()
  truncate_error_page()
    ->error_remove_page()

IOWs, the authors of this code did not know what to do, and like the
DAX failure stuff, merged the code without having resolving the
fundamental issues around interfacing with filesystem owned pages
directly...

I don't really have the time to look at it in any more depth right
now. I also suspect the memory failure code is a path we simply
cannot exercise in any useful manner so it's unlikely that we'll
ever be able to tell if this stuff works correctly or not....

Cheers,

-Dave.
-- 
Dave Chinner
david@fromorbit.com