Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

From: Peter Xu <peterx@redhat.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: John Hubbard <jhubbard@nvidia.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Jan Kara <jack@suse.cz>, Michal Hocko <mhocko@suse.com>,
	Kirill Tkhai <ktkhai@virtuozzo.com>,
	Kirill Shutemov <kirill@shutemov.name>,
	Hugh Dickins <hughd@google.com>, Christoph Hellwig <hch@lst.de>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Leon Romanovsky <leonro@nvidia.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jann Horn <jannh@google.com>
Subject: Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned
Date: Thu, 24 Sep 2020 13:55:31 -0400	[thread overview]
Message-ID: <20200924175531.GH79898@xz-x1> (raw)
In-Reply-To: <20200924165152.GE9916@ziepe.ca>

On Thu, Sep 24, 2020 at 01:51:52PM -0300, Jason Gunthorpe wrote:
> > Regarding the solution here, I think we can also cover read-only fast-gup too
> > in the future - IIUC what we need to do is to make it pte_protnone() instead of
> > pte_wrprotect(), then in the fault handler we should identify this special
> > pte_protnone() against numa balancing (change_prot_numa()).  I think it should
> > work fine too, iiuc, because I don't think we should migrate a page at all if
> > it's pinned for any reason...

[1]

> 
> With your COW breaking patch the read only fast-gup should break the
> COW because of the write protect, just like for the write side. Not
> seeing why we need to do something more?

Consider this sequence of a parent process managed to fork() a child:

       buf = malloc();
       // RDONLY gup
       pin_user_pages(buf, !WRITE);
       // pte of buf duplicated on both sides
       fork();
       mprotect(buf, WRITE);
       *buf = 1;
       // buf page replaced as cow triggered

Currently when fork() we'll happily share a pinned read-only page with the
child by copying the pte directly.  However it also means that starting from
this point, the child started to share this pinned page with the parent.  Then
if we can somehow trigger a "page unshare"/"cow", problem could occur.

In this case I'm using cow (by another mprotect() to trigger).  However I'm not
sure whether this is the only way to replace the pinned page for the parent.

As a summary: imho the important thing is we should not allow any kind of
sharing of any dma page, even it's pinned for read.

If my above understanding is correct - Above [1] may provide a solution for us
(in the future) when we want to block read-only fast-gup too in this patch just
like how we do that using wrprotect().

-- 
Peter Xu