Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings by default

From: David Hildenbrand <david@redhat.com>
To: Peter Xu <peterx@redhat.com>,
	"Kirill A . Shutemov" <kirill@shutemov.name>
Cc: Lorenzo Stoakes <lstoakes@gmail.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Jens Axboe <axboe@kernel.dk>,
	Matthew Wilcox <willy@infradead.org>,
	Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Leon Romanovsky <leon@kernel.org>,
	Christian Benvenuti <benve@cisco.com>,
	Nelson Escobar <neescoba@cisco.com>,
	Bernard Metzler <bmt@zurich.ibm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	Ian Rogers <irogers@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Bjorn Topel <bjorn@kernel.org>,
	Magnus Karlsson <magnus.karlsson@intel.com>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Christian Brauner <brauner@kernel.org>,
	Richard Cochran <richardcochran@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	linux-fsdevel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	Oleg Nesterov <oleg@redhat.com>,
	John Hubbard <jhubbard@nvidia.com>, Jan Kara <jack@suse.cz>,
	Pavel Begunkov <asml.silence@gmail.com>,
	Mika Penttila <mpenttil@redhat.com>,
	David Howells <dhowells@redhat.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings by default
Date: Fri, 28 Apr 2023 18:51:46 +0200	[thread overview]
Message-ID: <173337c0-14f4-3246-15ff-7fbf03861c94@redhat.com> (raw)
In-Reply-To: <ZEv2196tk5yWvgW5@x1n>

On 28.04.23 18:39, Peter Xu wrote:
> On Fri, Apr 28, 2023 at 07:22:07PM +0300, Kirill A . Shutemov wrote:
>> On Fri, Apr 28, 2023 at 06:13:03PM +0200, David Hildenbrand wrote:
>>> On 28.04.23 18:09, Kirill A . Shutemov wrote:
>>>> On Fri, Apr 28, 2023 at 05:43:52PM +0200, David Hildenbrand wrote:
>>>>> On 28.04.23 17:34, David Hildenbrand wrote:
>>>>>> On 28.04.23 17:33, Lorenzo Stoakes wrote:
>>>>>>> On Fri, Apr 28, 2023 at 05:23:29PM +0200, David Hildenbrand wrote:
>>>>>>>>>>
>>>>>>>>>> Security is the primary case where we have historically closed uAPI
>>>>>>>>>> items.
>>>>>>>>>
>>>>>>>>> As this patch
>>>>>>>>>
>>>>>>>>> 1) Does not tackle GUP-fast
>>>>>>>>> 2) Does not take care of !FOLL_LONGTERM
>>>>>>>>>
>>>>>>>>> I am not convinced by the security argument in regard to this patch.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If we want to sells this as a security thing, we have to block it
>>>>>>>>> *completely* and then CC stable.
>>>>>>>>
>>>>>>>> Regarding GUP-fast, to fix the issue there as well, I guess we could do
>>>>>>>> something similar as I did in gup_must_unshare():
>>>>>>>>
>>>>>>>> If we're in GUP-fast (no VMA), and want to pin a !anon page writable,
>>>>>>>> fallback to ordinary GUP. IOW, if we don't know, better be safe.
>>>>>>>
>>>>>>> How do we determine it's non-anon in the first place? The check is on the
>>>>>>> VMA. We could do it by following page tables down to folio and checking
>>>>>>> folio->mapping for PAGE_MAPPING_ANON I suppose?
>>>>>>
>>>>>> PageAnon(page) can be called from GUP-fast after grabbing a reference.
>>>>>> See gup_must_unshare().
>>>>>
>>>>> IIRC, PageHuge() can also be called from GUP-fast and could special-case
>>>>> hugetlb eventually, as it's table while we hold a (temporary) reference.
>>>>> Shmem might be not so easy ...
>>>>
>>>> page->mapping->a_ops should be enough to whitelist whatever fs you want.
>>>>
>>>
>>> The issue is how to stabilize that from GUP-fast, such that we can safely
>>> dereference the mapping. Any idea?
>>>
>>> At least for anon page I know that page->mapping only gets cleared when
>>> freeing the page, and we don't dereference the mapping but only check a
>>> single flag stored alongside the mapping. Therefore, PageAnon() is fine in
>>> GUP-fast context.
>>
>> What codepath you are worry about that clears ->mapping on pages with
>> non-zero refcount?
>>
>> I can only think of truncate (and punch hole). READ_ONCE(page->mapping)
>> and fail GUP_fast if it is NULL should be fine, no?
>>
>> I guess we should consider if the inode can be freed from under us and the
>> mapping pointer becomes dangling. But I think we should be fine here too:
>> VMA pins inode and VMA cannot go away from under GUP.
> 
> Can vma still go away if during a fast-gup?
> 

So, after we grabbed the page and made sure the the PTE didn't change 
(IOW, the PTE was stable while we processed it), the page can get 
unmapped (but not freed, because we hold a reference) and the VMA can 
theoretically go away (and as far as I understand, nothing stops the 
file from getting deleted, truncated etc).

So we might be looking at folio->mapping and the VMA is no longer there. 
Maybe even the file is no longer there.

-- 
Thanks,

David / dhildenb