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

From: Jason Gunthorpe <jgg@nvidia.com>
To: David Hildenbrand <david@redhat.com>
Cc: Lorenzo Stoakes <lstoakes@gmail.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>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	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 12:27:43 -0300	[thread overview]
Message-ID: <ZEvl717EANEu8113@nvidia.com> (raw)
In-Reply-To: <094d2074-5b69-5d61-07f7-9f962014fa68@redhat.com>

On Fri, Apr 28, 2023 at 05:08:27PM +0200, David Hildenbrand wrote:

> > I think this is broken today and we should block it. We know from
> > experiments with RDMA that doing exactly this triggers kernel oop's.
> 
> I never saw similar reports in the wild (especially targeted at RHEL), so is
> this still a current issue that has not been mitigated? Or is it just so
> hard to actually trigger?

People send RDMA related bug reports to us, and we tell them not to do
this stuff :)

> > I'm skeptical that anyone can actually do this combination of things
> > successfully without getting kernel crashes or file data corruption -
> > ie there is no real user to break.
> 
> I am pretty sure that there are such VM users, because on the libvirt level
> it's completely unclear which features trigger what behavior :/

IDK, why on earth would anyone want to do this? Using VFIO forces all
the memory to become resident so what was the point of making it file
backed in the first place?

I'm skeptical there are real users even if it now requires special
steps to be crashy/corrupty.

> > > Sure, we could warn, or convert individual users using a flag (io_uring).
> > > But maybe we should invest more energy on a fix?
> > 
> > It has been years now, I think we need to admit a fix is still years
> > away. Blocking the security problem may even motivate more people to
> > work on a fix.
> 
> Maybe we should make this a topic this year at LSF/MM (again?). At least we
> learned a lot about GUP, what might work, what might not work, and got a
> depper understanding (+ motivation to fix? :) ) the issue at hand.

We keep having the topic.. This is the old argument that the FS people
say the MM isn't following its inode and dirty lifetime rules and the
MM people say the FS isn't following its refcounting rules <shrug>

> > 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.

It is incremental and a temperature check to see what kind of real
users exist. We have no idea right now, just speculation.

Like I said, if there is feedback we can weaken it even further.

> Everything else sounds like band-aids to me, is insufficient, and might
> cause more harm than actually help IMHO. Especially the gup-fast case is
> extremely easy to work-around in malicious user space.

It is true this patch should probably block gup_fast when using
FOLL_LONGTERM as well, just like we used to do for the DAX check.

Jason