Re: [PATCH v2 1/2] mm, thp: check page mapping when truncating page cache

[Yang Shi]

On Thu, Sep 23, 2021 at 7:43 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 23 Sep 2021 01:04:54 +0800 Rongwei Wang <rongwei.wang@xxxxxxxxxxxxxxxxx> wrote:
>
> >
> >
> > > On Sep 22, 2021, at 7:37 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Sep 22, 2021 at 03:06:44PM +0800, Rongwei Wang wrote:
> > >> Transparent huge page has supported read-only non-shmem files. The file-
> > >> backed THP is collapsed by khugepaged and truncated when written (for
> > >> shared libraries).
> > >>
> > >> However, there is race in two possible places.
> > >>
> > >> 1) multiple writers truncate the same page cache concurrently;
> > >> 2) collapse_file rolls back when writer truncates the page cache;
> > >
> > > As I've said before, the bug here is that somehow there is a writable fd
> > > to a file with THPs.  That's what we need to track down and fix.
> > Hi, Matthew
> > I am not sure get your means. We know “mm, thp: relax the VM_DENYWRITE constraint on file-backed THPs"
> > Introduced file-backed THPs for DSO. It is possible {very rarely} for DSO to be opened in writeable way.
> >
> > ...
> >
> > > https://lore.kernel.org/linux-mm/YUdL3lFLFHzC80Wt@xxxxxxxxxxxxxxxxxxxx/
> > All in all, what you mean is that we should solve this race at the source?
>
> Matthew is being pretty clear here: we shouldn't be permitting
> userspace to get a writeable fd for a thp-backed file.

No, he doesn't mean it IIRC. Actually we had the same conversation for
another patch. Quoted below:

" > > Things have already gone wrong before we get to this point.  See
> > do_dentry_open().  You aren't supposed to be able to get a writable file
> > descriptor on a file which has had huge pages added to the page cache
> > without the filesystem's knowledge.  That's the problem that needs to
> > be fixed.
>
> I don't quite understand your point here. Do you mean do_dentry_open()
> should fail for such cases instead of truncating the page cache?

No, do_dentry_open() should have truncated the page cache when it was
called and found that there were THPs in the cache.  Then khugepaged
should see that someone has the file open for write and decline to create
new THPs.  So it shouldn't be possible to get here with THPs in the cache."

Please see https://lore.kernel.org/linux-mm/YUkCI2I085Sos%2F64@xxxxxxxxxxxxxxxxxxxx/

But actually "mm, thp: relax the VM_DENYWRITE constraint on
file-backed THPs" did so exactly.

>
> Why are we permitting the DSO to be opened writeably?  If there's a
> legitimate case for doing this then presumably "mm, thp: relax the
> VM_DENYWRITE constraint on file-backed THPs: should be fixed or
> reverted.

Unfortunately we can't revert this commit anymore since VM_DENYWRITE
is gone due to commit 8d0920bde5eb ("mm: remove VM_DENYWRITE")

>
> If there is no legitimate use case for returning a writeable fd for a
> thp-backed file then we should fail such an attempt at open().  This
> approach has back-compatibility issues which need to be thought about.
> Perhaps we should permit the open-writeably attempt to appear to
> succeed, but to really return a read-only fd?
>
>