Received-SPF: pass (google.com: domain of linux-kernel+bounces-211553-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 12 Jun 2024 10:23:58 -0300
From: Jason Gunthorpe <jgg@ziepe.ca>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Nicolin Chen <nicolinc@nvidia.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	Jacob Pan <jacob.jun.pan@linux.intel.com>,
	Joel Granados <j.granados@samsung.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 06/10] iommufd: Add iommufd fault object
Message-ID: <20240612132358.GU791043@ziepe.ca>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-7-baolu.lu@linux.intel.com>
 <BN9PR11MB527615EC664698C340A0CA878CFB2@BN9PR11MB5276.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN9PR11MB527615EC664698C340A0CA878CFB2@BN9PR11MB5276.namprd11.prod.outlook.com>

On Fri, Jun 07, 2024 at 09:17:28AM +0000, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > Sent: Monday, May 27, 2024 12:05 PM
> > 
> > +static ssize_t iommufd_fault_fops_read(struct file *filep, char __user *buf,
> > +				       size_t count, loff_t *ppos)
> > +{
> > +	size_t fault_size = sizeof(struct iommu_hwpt_pgfault);
> > +	struct iommufd_fault *fault = filep->private_data;
> > +	struct iommu_hwpt_pgfault data;
> > +	struct iommufd_device *idev;
> > +	struct iopf_group *group;
> > +	struct iopf_fault *iopf;
> > +	size_t done = 0;
> > +	int rc = 0;
> > +
> > +	if (*ppos || count % fault_size)
> > +		return -ESPIPE;
> 
> the man page says:
> 
> "If count is zero, read() returns zero and has no  other  results."

The above does that? 0 % X == 0

> > +
> > +	mutex_lock(&fault->mutex);
> > +	while (!list_empty(&fault->deliver) && count > done) {
> > +		group = list_first_entry(&fault->deliver,
> > +					 struct iopf_group, node);
> > +
> > +		if (group->fault_count * fault_size > count - done)
> > +			break;
> > +
> > +		rc = xa_alloc(&fault->response, &group->cookie, group,
> > +			      xa_limit_32b, GFP_KERNEL);
> > +		if (rc)
> > +			break;
> > +
> > +		idev = to_iommufd_handle(group->attach_handle)->idev;
> > +		list_for_each_entry(iopf, &group->faults, list) {
> > +			iommufd_compose_fault_message(&iopf->fault,
> > +						      &data, idev,
> > +						      group->cookie);
> > +			rc = copy_to_user(buf + done, &data, fault_size);
> > +			if (rc) {
> 
> 'rc' should be converted to -EFAULT.

Yes


> > +				xa_erase(&fault->response, group->cookie);
> > +				break;
> > +			}
> > +			done += fault_size;
> > +		}
> > +
> > +		list_del(&group->node);
> > +	}
> > +	mutex_unlock(&fault->mutex);
> > +
> > +	return done == 0 ? rc : done;
> 
> again this doesn't match the manual:
> 
> "On error, -1 is returned, and errno is set appropriately. "
> 
> it doesn't matter whether 'done' is 0.

It is setup so that once the list_del() below happens it is guarenteed
that the system call will return a positive result so that the
list_del'd items are always returned to userspace.

If we hit any fault here on the Nth item we should still return the
prior items and ignore the fault.

If we hit a fault on the first item then we should return the fault.

Jason