linux-kernel - Re: More filesystem need this fix (xfs: use MMAPLOCK around filemap_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <df9eb392-8b86-591a-b1be-535a13b874d9@suse.com>
Date:   Thu, 17 Sep 2020 08:37:17 +0300
From:   Nikolay Borisov <nborisov@...e.com>
To:     Dave Chinner <david@...morbit.com>, Jan Kara <jack@...e.cz>
Cc:     Amir Goldstein <amir73il@...il.com>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        Theodore Tso <tytso@....edu>,
        Martin Brandenburg <martin@...ibond.com>,
        Mike Marshall <hubcap@...ibond.com>,
        Damien Le Moal <damien.lemoal@....com>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Qiuyang Sun <sunqiuyang@...wei.com>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>, nborisov@...e.de
Subject: Re: More filesystem need this fix (xfs: use MMAPLOCK around
 filemap_map_pages())



On 17.09.20 г. 4:44 ч., Dave Chinner wrote:
> On Wed, Sep 16, 2020 at 05:58:51PM +0200, Jan Kara wrote:
>> On Sat 12-09-20 09:19:11, Amir Goldstein wrote:
>>> On Tue, Jun 23, 2020 at 8:21 AM Dave Chinner <david@...morbit.com> wrote:

<snip>

> 
> So....
> 
> P0					p1
> 
> hole punch starts
>   takes XFS_MMAPLOCK_EXCL
>   truncate_pagecache_range()
>     unmap_mapping_range(start, end)
>       <clears ptes>
> 					<read fault>
> 					do_fault_around()
> 					  ->map_pages
> 					    filemap_map_pages()
> 					      page mapping valid,
> 					      page is up to date
> 					      maps PTEs
> 					<fault done>
>     truncate_inode_pages_range()
>       truncate_cleanup_page(page)
>         invalidates page
>       delete_from_page_cache_batch(page)
>         frees page
> 					<pte now points to a freed page>
> 
> That doesn't seem good to me.
> 
> Sure, maybe the page hasn't been freed back to the free lists
> because of elevated refcounts. But it's been released by the
> filesystem and not longer in the page cache so nothing good can come
> of this situation...
> 
> AFAICT, this race condition exists for the truncate case as well
> as filemap_map_pages() doesn't have a "page beyond inode size" check
> in it. 

(It's not relevant to the discussion at hand but for the sake of
completeness):

It does have a check:

 max_idx = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE);
 if (page->index >= max_idx)
      goto unlock;


<snip>