linux-kernel - Re: [PATCH 6/7] mm, fs: introduce file_operations->post

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]

Date:   Wed, 27 Sep 2017 08:36:36 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        Christoph Hellwig <hch@....de>,
        Dave Chinner <david@...morbit.com>,
        Jeff Layton <jlayton@...chiereds.net>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        linux-xfs@...r.kernel.org
Subject: Re: [PATCH 6/7] mm, fs: introduce file_operations->post_mmap()

On Wed, Sep 27, 2017 at 8:07 AM, Jan Kara <jack@...e.cz> wrote:
> On Wed 27-09-17 07:00:53, Dan Williams wrote:
>> On Wed, Sep 27, 2017 at 4:35 AM, Jan Kara <jack@...e.cz> wrote:
>> > On Tue 26-09-17 14:41:53, Dan Williams wrote:
>> >> On Tue, Sep 26, 2017 at 2:06 PM, Ross Zwisler
>> >> <ross.zwisler@...ux.intel.com> wrote:
>> >> > On Tue, Sep 26, 2017 at 12:19:21PM -0700, Dan Williams wrote:
>> >> >> On Tue, Sep 26, 2017 at 11:57 AM, Ross Zwisler
>> >> > <>
>> >> >> > This decision can only be made (in this
>> >> >> > proposed scheme) *after* the inode->i_mapping->i_mmap  tree has been
>> >> >> > populated, which means we need another call into the filesystem after this
>> >> >> > insertion has happened.
>> >> >>
>> >> >> I get that, but it seems over-engineered and something that can also
>> >> >> be safely cleaned up after the fact by the code path that is disabling
>> >> >> DAX.
>> >> >
>> >> > I don't think you can safely clean it up after the fact because some thread
>> >> > might have already called ->mmap() to set up the vma->vm_flags for their new
>> >> > mapping, but they haven't added it to inode->i_mapping->i_mmap.
>> >>
>> >> If madvise(MADV_NOHUGEPAGE) can dynamically change vm_flags, then the
>> >> DAX disable path can as well. VM_MIXEDMAP looks to be a nop for normal
>> >> memory mappings.
>> >>
>> >> > The inode->i_mapping->i_mmap tree is the only way (that I know of at least)
>> >> > that the filesystem has any idea about about the mapping.  This is the method
>> >> > by which we would try and clean up mapping flags, if we were to do so, and
>> >> > it's the only way that the filesystem can know whether or not mappings exist.
>> >> >
>> >> > The only way that I could think of to make this safely work is to have the
>> >> > insertion into the inode->i_mapping->i_mmap tree be our sync point.  After
>> >> > that the filesystem and the mapping code can communicate on the state of DAX,
>> >> > but before that I think it's basically indeterminate.
>> >>
>> >> If we lose the race and leak VM_HUGEPAGE to a non-DAX mapping what
>> >> breaks? I'd rather be in favor of not setting VM_HUGEPAGE at all in
>> >> the ->mmap() handler and let the default THP policy take over. In
>> >> fact, see transparent_hugepage_enabled() we already auto-enable huge
>> >> page support for dax mappings regardless of VM_HUGEPAGE.
>> >
>> > Hum, this is an interesting option. So do you suggest that filesystems
>> > supporting DAX would always setup mappings with VM_MIXEDMAP and without
>> > VM_HUGEPAGE and thus we'd get rid of dependency on S_DAX flag in ->mmap?
>> > That could actually work. The only possible issue I can see is that
>> > VM_MIXEDMAP is still slightly different from normal page mappings and it
>> > could have some performance implications - e.g. copy_page_range() does more
>> > work on VM_MIXEDMAP mappings but not on normal page mappings.
>>
>> We can also get rid of VM_MIXEDMAP if we disable DAX in the
>> !pfn_t_has_page() case.
>
> Yeah, although it would be a pity to require struct page just to avoid
> having to set VM_MIXEDMAP flag...

Yes, but the real motivation is fixing all the basic things that break
without struct page, like ptrace and direct-i/o. The removal of
needing to set VM_MIXEDMAP is just a nice side effect. I'll send a
patch because DAX without pages has too many surprise failure cases.