Re: [PATCH v1 2/2] mm/hugetlb: support write-faults in shared mappings

[Mike Kravetz]

On 08/10/22 15:52, Peter Xu wrote:
> On Wed, Aug 10, 2022 at 09:40:11PM +0200, David Hildenbrand wrote:
> > On 10.08.22 21:29, Peter Xu wrote:
> > > On Wed, Aug 10, 2022 at 11:37:13AM +0200, David Hildenbrand wrote:
> > >> On 09.08.22 00:08, Peter Xu wrote:
> > >>> On Mon, Aug 08, 2022 at 04:21:39PM -0400, Peter Xu wrote:
> > >>>> On Mon, Aug 08, 2022 at 06:25:21PM +0200, David Hildenbrand wrote:
> > >>>>>>> Relying on VM_SHARED to detect MAP_PRIVATE vs. MAP_SHARED is
> > >>>>>>> unfortunately wrong.
> > >>>>>>>
> > >>>>>>> If you're curious, take a look at f83a275dbc5c ("mm: account for
> > >>>>>>> MAP_SHARED mappings using VM_MAYSHARE and not VM_SHARED in hugetlbfs")
> > >>>>>>> and mmap() code.
> > >>>>>>>
> > >>>>>>> Long story short: if the file is read-only, we only have VM_MAYSHARE but
> > >>>>>>> not VM_SHARED (and consequently also not VM_MAYWRITE).
> > >>>>>>
> > >>>>>> To ask in another way: if file is RO but mapped RW (mmap() will have
> > >>>>>> VM_SHARED cleared but VM_MAYSHARE set), then if we check VM_MAYSHARE here
> > >>>>>> won't we grant write bit errornously while we shouldn't? As the user
> > >>>>>> doesn't really have write permission to the file.
> > >>>>>
> > >>>>> Thus the VM_WRITE check. :)
> > >>>>>
> > >>>>> I wonder if we should just do it cleanly and introduce the maybe_mkwrite
> > >>>>> semantics here as well. Then there is no need for additional VM_WRITE
> > >>>>> checks and hugetlb will work just like !hugetlb.
> > >>>>
> > >>>> Hmm yeah I think the VM_MAYSHARE check is correct, since we'll need to fail
> > >>>> the cases where MAYSHARE && !SHARE - we used to silently let it pass.
> > >>>
> > >>> Sorry I think this is a wrong statement I made..  IIUC we'll fail correctly
> > >>> with/without the patch on any write to hugetlb RO regions.
> > >>>
> > >>> Then I just don't see a difference on checking VM_SHARED or VM_MAYSHARE
> > >>> here, it's just that VM_MAYSHARE check should work too like VM_SHARED so I
> > >>> don't see a problem.
> > >>>
> > >>> It also means I can't think of any valid case of having VM_WRITE when
> > >>> reaching here, then the WARN_ON_ONCE() is okay but maybe also redundant.
> > >>> Using maybe_mkwrite() seems misleading to me if FOLL_FORCE not ready for
> > >>> hugetlbfs after all.
> > >>>
> > >>
> > >> The main reason we'd have it would be to scream out lout and fail
> > >> gracefully if someone would -- for example -- use it for something like
> > >> FOLL_FORCE.
> > > 
> > > Having that WARN_ON_ONCE() is okay to me, but just to double check we're on
> > > the same page: why there's concern on using FOLL_FORCE? IIUC we're talking
> > > about shared mappings here, then no FOLL_FORCE possible at all?  IOW,
> > > "!is_cow_mapping()" should fail in check_vma_flags() already.
> > 
> > This code path also covers the anon case.
> 
> But this specific warning is under the VM_MAYSHARE if clause just added in
> this patch?
> 
> My understanding is any FOLL_FORCE will always constantly fail before this
> patch, and it should keep failing as usual and I don't see any case it'll
> be failing at the warn_on_once here.
> 
> So again, I'm fine with having the warning, but I just want to make sure
> what you want to capture is what you expected..
> 
> > > 
> > > The other thing is I'm wondering whether patch 2 should be postponed anyway
> > > so that we can wait for a full resolution of the problem from Mike.
> > 
> > To make the code more robust and avoid any other such surprises I prefer
> > to have this in rather earlier than later.
> > 
> > As the commit says "Let's add a safety net ..."
> 
> Sure, no strong opinion.  I'll leave that to Mike.  Thanks,
> 

Sorry that I am not contributing to this thread more.  Need to spend
some time educating myself on the relatively new semantics here.

As mentioned, softdirty is on my todo list but has been there for over a
year.  So, better to add a safety net until that code moves forward.

However, just for clarification.  The only way we KNOW of to encounter
these situations today via softdirty.  Patch 1 takes care of that.  This
patch catches any unknown ways we may get here.  Correct?  i.e. We don't
really expect to exercise these code paths.
-- 
Mike Kravetz