From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Subject: Re: [PATCH v5 09/24] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM Date: Fri, 15 Nov 2019 10:08:42 -0400 Message-ID: <20191115140842.GA4055@ziepe.ca> References: <20191115055340.1825745-1-jhubbard@nvidia.com> <20191115055340.1825745-10-jhubbard@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20191115055340.1825745-10-jhubbard@nvidia.com> Sender: linux-kernel-owner@vger.kernel.org To: John Hubbard Cc: Andrew Morton , Al Viro , Alex Williamson , Benjamin Herrenschmidt , =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= , Christoph Hellwig , Dan Williams , Daniel Vetter , Dave Chinner , David Airlie , "David S . Miller" , Ira Weiny , Jan Kara , Jens Axboe , Jonathan Corbet , =?utf-8?B?SsOpcsO0bWU=?= Glisse , Magnus Karlsson , Mauro Carvalho Chehab , Michael Ellerman List-Id: dri-devel@lists.freedesktop.org On Thu, Nov 14, 2019 at 09:53:25PM -0800, John Hubbard wrote: > As it says in the updated comment in gup.c: current FOLL_LONGTERM > behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the > FS DAX check requirement on vmas. > > However, the corresponding restriction in get_user_pages_remote() was > slightly stricter than is actually required: it forbade all > FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers > that do not set the "locked" arg. > > Update the code and comments accordingly, and update the VFIO caller > to take advantage of this, fixing a bug as a result: the VFIO caller > is logically a FOLL_LONGTERM user. > > Also, remove an unnessary pair of calls that were releasing and > reacquiring the mmap_sem. There is no need to avoid holding mmap_sem > just in order to call page_to_pfn(). > > Also, move the DAX check ("if a VMA is DAX, don't allow long term > pinning") from the VFIO call site, all the way into the internals > of get_user_pages_remote() and __gup_longterm_locked(). That is: > get_user_pages_remote() calls __gup_longterm_locked(), which in turn > calls check_dax_vmas(). It's lightly explained in the comments as well. > > Thanks to Jason Gunthorpe for pointing out a clean way to fix this, > and to Dan Williams for helping clarify the DAX refactoring. > > Suggested-by: Jason Gunthorpe > Cc: Dan Williams > Cc: Jerome Glisse > Cc: Ira Weiny > Signed-off-by: John Hubbard > --- > drivers/vfio/vfio_iommu_type1.c | 30 +++++------------------------- > mm/gup.c | 27 ++++++++++++++++++++++----- > 2 files changed, 27 insertions(+), 30 deletions(-) Looks OK now Reviewed-by: Jason Gunthorpe