Re: [PATCH v2 4/6] iommufd: Deliver fault messages to user space

[Jason Gunthorpe]

On Fri, Jan 12, 2024 at 05:46:13PM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Thursday, October 26, 2023 3:49 AM
> > To: Jason Gunthorpe <jgg@xxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Joerg
> > Roedel <joro@xxxxxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Robin Murphy
> > <robin.murphy@xxxxxxx>; Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>;
> > Nicolin Chen <nicolinc@xxxxxxxxxx>; Yi Liu <yi.l.liu@xxxxxxxxx>; Jacob Pan
> > <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Cc: iommu@xxxxxxxxxxxxxxx; linux-kselftest@xxxxxxxxxxxxxxx;
> > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Lu
> > Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Subject: [PATCH v2 4/6] iommufd: Deliver fault messages to user space
> > 
> [...]
> 
> Hi,
> 
> > +static ssize_t hwpt_fault_fops_write(struct file *filep,
> > +				     const char __user *buf,
> > +				     size_t count, loff_t *ppos)
> > +{
> > +	size_t response_size = sizeof(struct iommu_hwpt_page_response);
> > +	struct hw_pgtable_fault *fault = filep->private_data;
> > +	struct iommu_hwpt_page_response response;
> > +	struct iommufd_hw_pagetable *hwpt;
> > +	struct iopf_group *iter, *group;
> > +	struct iommufd_device *idev;
> > +	size_t done = 0;
> > +	int rc = 0;
> > +
> > +	if (*ppos || count % response_size)
> > +		return -ESPIPE;
> > +
> > +	mutex_lock(&fault->mutex);
> > +	while (!list_empty(&fault->response) && count > done) {
> > +		rc = copy_from_user(&response, buf + done, response_size);
> > +		if (rc)
> > +			break;
> > +
> > +		/* Get the device that this response targets at. */
> > +		idev = container_of(iommufd_get_object(fault->ictx,
> > +						       response.dev_id,
> > +						       IOMMUFD_OBJ_DEVICE),
> > +				    struct iommufd_device, obj);
> > +		if (IS_ERR(idev)) {
> > +			rc = PTR_ERR(idev);
> > +			break;
> > +		}
> > +
> > +		/*
> > +		 * Get the hw page table that this response was generated for.
> > +		 * It must match the one stored in the fault data.
> > +		 */
> > +		hwpt = container_of(iommufd_get_object(fault->ictx,
> > +						       response.hwpt_id,
> > +
> > IOMMUFD_OBJ_HW_PAGETABLE),
> > +				    struct iommufd_hw_pagetable, obj);
> > +		if (IS_ERR(hwpt)) {
> > +			iommufd_put_object(&idev->obj);
> > +			rc = PTR_ERR(hwpt);
> > +			break;
> > +		}
> > +
> > +		if (hwpt != fault->hwpt) {
> > +			rc = -EINVAL;
> > +			goto put_obj;
> > +		}
> > +
> > +		group = NULL;
> > +		list_for_each_entry(iter, &fault->response, node) {
> > +			if (response.grpid != iter->last_fault.fault.prm.grpid)
> > +				continue;
> > +
> > +			if (idev->dev != iter->dev)
> > +				continue;
> > +
> > +			if ((iter->last_fault.fault.prm.flags &
> > +			     IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) &&
> > +			    response.pasid != iter->last_fault.fault.prm.pasid)
> > +				continue;
> 
> I am trying to get vSVA working with this series and got hit by the above check.
> On ARM platforms, page responses to stall events(CMD_RESUME) do not have
> an associated pasid.  I think, either we need to check here using
> IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID or remove the check 
> as it will be eventually done in iommu_page_response(). 

That doesn't sound right..

The PASID is the only information we have for userspace to identify
the domain that is being faulted. It cannot be optional on the request
side.

If it is valid when userspace does read() then it should be valid when
userspace does write() too.

It is the only way the kernel can actually match request and response
here.

So, I think you have a userspace issue to not provide the right
pasid??

Jason