Date:   Thu, 29 Sep 2022 16:33:27 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>, Dave Chinner <david@...morbit.com>
CC:     Jan Kara <jack@...e.cz>, Dan Williams <dan.j.williams@...el.com>,
        "Jason Gunthorpe" <jgg@...dia.com>, <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        "Darrick J. Wong" <djwong@...nel.org>,
        "Christoph Hellwig" <hch@....de>,
        John Hubbard <jhubbard@...dia.com>,
        <linux-fsdevel@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
        <linux-xfs@...r.kernel.org>, <linux-mm@...ck.org>,
        <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2 05/18] xfs: Add xfs_break_layouts() to the inode
 eviction path

Jan Kara wrote:
> On Mon 26-09-22 09:54:07, Dave Chinner wrote:
> > On Fri, Sep 23, 2022 at 11:38:03AM +0200, Jan Kara wrote:
> > > On Fri 23-09-22 12:10:12, Dave Chinner wrote:
> > > > On Thu, Sep 22, 2022 at 05:41:08PM -0700, Dan Williams wrote:
> > > > > Dave Chinner wrote:
> > > > > > On Wed, Sep 21, 2022 at 07:28:51PM -0300, Jason Gunthorpe wrote:
> > > > > > > On Thu, Sep 22, 2022 at 08:14:16AM +1000, Dave Chinner wrote:
> > > > > > > 
> > > > > > > > Where are these DAX page pins that don't require the pin holder to
> > > > > > > > also hold active references to the filesystem objects coming from?
> > > > > > > 
> > > > > > > O_DIRECT and things like it.
> > > > > > 
> > > > > > O_DIRECT IO to a file holds a reference to a struct file which holds
> > > > > > an active reference to the struct inode. Hence you can't reclaim an
> > > > > > inode while an O_DIRECT IO is in progress to it. 
> > > > > > 
> > > > > > Similarly, file-backed pages pinned from user vmas have the inode
> > > > > > pinned by the VMA having a reference to the struct file passed to
> > > > > > them when they are instantiated. Hence anything using mmap() to pin
> > > > > > file-backed pages (i.e. applications using FSDAX access from
> > > > > > userspace) should also have a reference to the inode that prevents
> > > > > > the inode from being reclaimed.
> > > > > > 
> > > > > > So I'm at a loss to understand what "things like it" might actually
> > > > > > mean. Can you actually describe a situation where we actually permit
> > > > > > (even temporarily) these use-after-free scenarios?
> > > > > 
> > > > > Jason mentioned a scenario here:
> > > > > 
> > > > > https://lore.kernel.org/all/YyuoE8BgImRXVkkO@nvidia.com/
> > > > > 
> > > > > Multi-thread process where thread1 does open(O_DIRECT)+mmap()+read() and
> > > > > thread2 does memunmap()+close() while the read() is inflight.
> > > > 
> > > > And, ah, what production application does this and expects to be
> > > > able to process the result of the read() operation without getting a
> > > > SEGV?
> > > > 
> > > > There's a huge difference between an unlikely scenario which we need
> > > > to work (such as O_DIRECT IO to/from a mmap() buffer at a different
> > > > offset on the same file) and this sort of scenario where even if we
> > > > handle it correctly, the application can't do anything with the
> > > > result and will crash immediately....
> > > 
> > > I'm not sure I fully follow what we are concerned about here. As you've
> > > written above direct IO holds reference to the inode until it is completed
> > > (through kiocb->file->inode chain). So direct IO should be safe?
> > 
> > AFAICT, it's the user buffer allocated by mmap() that the direct IO
> > is DMAing into/out of that is the issue here. i.e. mmap() a file
> > that is DAX enabled, pass the mmap region to DIO on a non-dax file,
> > GUP in the DIO path takes a page pin on user pages that are DAX
> > mapped, the userspace application then unmaps the file pages and
> > unlinks the FSDAX file.
> > 
> > At this point the FSDAX mapped inode has no active references, so
> > the filesystem frees the inode and it's allocated storage space, and
> > now the DIO or whatever is holding the GUP reference is
> > now a moving storage UAF violation. What ever is holding the GUP
> > reference doesn't even have a reference to the FSDAX filesystem -
> > the DIO fd could point to a file in a different filesystem
> > altogether - and so the fsdax filesytem could be unmounted at this
> > point whilst the application is still actively using the storage
> > underlying the filesystem.
> > 
> > That's just .... broken.
> 
> Hum, so I'm confused (and my last email probably was as well). So let me
> spell out the details here so that I can get on the same page about what we
> are trying to solve:
> 
> For FSDAX, backing storage for a page must not be freed (i.e., filesystem
> must to free corresponding block) while there are some references to the
> page. This is achieved by calls to dax_layout_busy_page() from the
> filesystem before truncating file / punching hole into a file. So AFAICT
> this is working correctly and I don't think the patch series under
> discussion aims to change this besides the change in how page without
> references is detected.

Correct. All the nominal truncate paths via hole punch and
truncate_setsize() are already handled for a long time now. However,
what was not covered was the truncate that happens at iput_final() time.
In that case the code has just been getting lucky for all that time.
There is thankfully a WARN() that will trigger if the iput_final()
truncate happens while a page is referenced, so it is at least not
silent.

I know Dave is tired of this discussion, but every time he engages the
solution gets better, like finding this iput_final() bug, so I hope he
continues to engage here and I'm grateful for the help.

> Now there is a separate question that while someone holds a reference to
> FSDAX page, the inode this page belongs to can get evicted from memory. For
> FSDAX nothing prevents that AFAICT. If this happens, we loose track of the
> page<->inode association so if somebody later comes and truncates the
> inode, we will not detect the page belonging to the inode is still in use
> (dax_layout_busy_page() does not find the page) and we have a problem.
> Correct?

The WARN would fire at iput_final(). Everything that happens after
that is in UAF territory. In my brief search I did not see reports of
this WARN firing, but it is past time to fix it.

> > > I'd be more worried about stuff like vmsplice() that can add file pages
> > > into pipe without holding inode alive in any way and keeping them there for
> > > arbitrarily long time. Didn't we want to add FOLL_LONGTERM to gup executed
> > > from vmsplice() to avoid issues like this?
> > 
> > Yes, ISTR that was part of the plan - use FOLL_LONGTERM to ensure
> > FSDAX can't run operations that pin pages but don't take fs
> > references. I think that's how we prevented RDMA users from pinning
> > FSDAX direct mapped storage media in this way. It does not, however,
> > prevent the above "short term" GUP UAF situation from occurring.
> 
> If what I wrote above is correct, then I understand and agree.
> 
> > > I agree that freeing VMA while there are pinned pages is ... inconvenient.
> > > But that is just how gup works since the beginning - the moment you have
> > > struct page reference, you completely forget about the mapping you've used
> > > to get to the page. So anything can happen with the mapping after that
> > > moment. And in case of pages mapped by multiple processes I can easily see
> > > that one of the processes decides to unmap the page (and it may well be
> > > that was the initial process that acquired page references) while others
> > > still keep accessing the page using page references stored in some internal
> > > structure (RDMA anyone?).
> > 
> > Yup, and this is why RDMA on FSDAX using this method of pinning pages
> > will end up corrupting data and filesystems, hence FOLL_LONGTERM
> > protecting against most of these situations from even arising. But
> > that's that workaround, not a long term solution that allows RDMA to
> > be run on FSDAX managed storage media.
> > 
> > I said on #xfs a few days ago:
> > 
> > [23/9/22 10:23] * dchinner is getting deja vu over this latest round
> > of "dax mappings don't pin the filesystem objects that own the
> > storage media being mapped"
> > 
> > And I'm getting that feeling again right now...
> > 
> > > I think it will be rather difficult to come up
> > > with some scheme keeping VMA alive while there are pages pinned without
> > > regressing userspace which over the years became very much tailored to the
> > > peculiar gup behavior.
> > 
> > Perhaps all we should do is add a page flag for fsdax mapped pages
> > that says GUP must pin the VMA, so only mapped pages that fall into
> > this category take the perf penalty of VMA management.
> 
> Possibly. But my concern with VMA pinning was not only about performance
> but also about applications relying on being able to unmap pages that are
> currently pinned. At least from some processes one of which may be the one
> doing the original pinning. But yeah, the fact that FOLL_LONGTERM is
> forbidden with DAX somewhat restricts the insanity we have to deal with. So
> maybe pinning the VMA for DAX mappings might actually be a workable
> solution.

As far as I can see, VMAs are not currently reference counted they are
just added / deleted from an mm_struct, and nothing guarantees
mapping_mapped() stays true while a page is pinned.

I like Dave's mental model that the inode is the arbiter for the page,
and the arbiter is not allowed to go out of scope before asserting that
everything it granted previously has been returned.

write_inode_now() unconditionally invokes dax_writeback_mapping_range()
when the inode is committed to going out of scope. write_inode_now() is
allowed to sleep until all dirty mapping entries are written back. I see
nothing wrong with additionally checking for entries with elevated page
reference counts and doing a:

    __wait_var_event(page, dax_page_idle(page));

Since the inode is out of scope there should be no concerns with racing
new 0 -> 1 page->_refcount transitions. Just wait for transient page
pins to finally drain to zero which should already be on the order of
the wait time to complete synchrounous writeback in the dirty inode
case.

Powered by blists - more mailing lists