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

[Peter Xu]

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.

But then OTOH using WARN_ON_ONCE on the VM_WRITE check is probably not
right, because iiuc it can be triggered easily by the userspace. E.g. as
simple as mapping hugetlb as RO+shared then write to it?

So maybe_mkwrite() seems not an option now - IIUC we really need that
!VM_WRITE check to fail properly, but just without the warning to pollute
dmesg?

Thanks,

-- 
Peter Xu