Re: [PATCH v3 07/15] iommufd: PFN handling for iopt_pages

From: Jason Gunthorpe <jgg@nvidia.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>,
	bpf@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	David Woodhouse <dwmw2@infradead.org>,
	iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	Kevin Tian <kevin.tian@intel.com>,
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org,
	llvm@lists.linux.dev, Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Shuah Khan <shuah@kernel.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Tom Rix <trix@redhat.com>, Will Deacon <will@kernel.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Chaitanya Kulkarni <chaitanyak@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Eric Auger <eric.auger@redhat.com>,
	Eric Farman <farman@linux.ibm.com>,
	Jason Wang <jasowang@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joao Martins <joao.m.martins@oracle.com>,
	kvm@vger.kernel.org, Matthew Rosato <mjrosato@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Shameerali Kolothum Thodi  <shameerali.kolothum.thodi@huawei.com>,
	Yi Liu <yi.l.liu@intel.com>, Keqian Zhu <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 07/15] iommufd: PFN handling for iopt_pages
Date: Wed, 2 Nov 2022 10:13:51 -0300	[thread overview]
Message-ID: <Y2JtD5Uq/ddcYf1g@nvidia.com> (raw)
In-Reply-To: <Y2F1yBnFpv8jhosS@Asurada-Nvidia>

On Tue, Nov 01, 2022 at 12:38:48PM -0700, Nicolin Chen wrote:

> > +static int pfn_reader_user_pin(struct pfn_reader_user *user,
> > +			       struct iopt_pages *pages,
> > +			       unsigned long start_index,
> > +			       unsigned long last_index)
> > +{
> > +	bool remote_mm = pages->source_mm != current->mm;
> > +	unsigned long npages;
> > +	uintptr_t uptr;
> > +	long rc;
> > +
> > +	if (!user->upages) {
> > +		/* All undone in pfn_reader_destroy() */
> > +		user->upages_len =
> > +			(last_index - start_index + 1) * sizeof(*user->upages);
> > +		user->upages = temp_kmalloc(&user->upages_len, NULL, 0);
> > +		if (!user->upages)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	if (user->locked == -1) {
> > +		/*
> > +		 * The majority of usages will run the map task within the mm
> > +		 * providing the pages, so we can optimize into
> > +		 * get_user_pages_fast()
> > +		 */
> > +		user->locked = 0;
> > +		if (remote_mm) {
> > +			if (!mmget_not_zero(pages->source_mm)) {
> > +				kfree(user->upages);
> > +				user->upages = NULL;
> 
> Coverity reports BAD_FREE at user->upages here.
> 
> In iopt_pages_fill_xarray and iopt_pages_fill_from_mm, user->upages
> is assigned by shifting the out_pages input of iopt_pages_add_access
> that could be originated from vfio_pin_pages, I am not sure if the
> remote_mm and mmget_not_zero value checks can prevent this though.

Yep, missed that when I reworked this. It should be like this:

                 * providing the pages, so we can optimize into
                 * get_user_pages_fast()
                 */
-               user->locked = 0;
                if (remote_mm) {
-                       if (!mmget_not_zero(pages->source_mm)) {
-                               kfree(user->upages);
-                               user->upages = NULL;
+                       if (!mmget_not_zero(pages->source_mm))
                                return -EFAULT;
-                       }
                }
+               user->locked = 0;
        }

ie locked is tracking the mmget and is completely independent of
upages.

Jason