> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> > 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." > + > + 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. > + 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. > + > +static int iommufd_fault_fops_release(struct inode *inode, struct file *filep) > +{ > + struct iommufd_fault *fault = filep->private_data; > + > + iommufd_ctx_put(fault->ictx); > + refcount_dec(&fault->obj.users); > + return 0; > +} hmm this doesn't sound correct. the context and refcount are acquired in iommufd_fault_alloc() but here they are reverted when the fd is closed... > + > + filep = anon_inode_getfile("[iommufd-pgfault]", > &iommufd_fault_fops, > + fault, O_RDWR); > + if (IS_ERR(filep)) { > + rc = PTR_ERR(filep); > + goto out_abort; > + } > + > + refcount_inc(&fault->obj.users); > + iommufd_ctx_get(fault->ictx); > + fault->filep = filep; those 3 lines can be moved after below fdno get. It's reads slightly clearer to put file related work together before getting to the last piece of intiailzation. > + > + fdno = get_unused_fd_flags(O_CLOEXEC); > + if (fdno < 0) { > + rc = fdno; > + goto out_fput; > + } > + > @@ -332,6 +332,7 @@ union ucmd_buffer { > struct iommu_ioas_unmap unmap; > struct iommu_option option; > struct iommu_vfio_ioas vfio_ioas; > + struct iommu_fault_alloc fault; alphabetic > @@ -381,6 +382,8 @@ static const struct iommufd_ioctl_op > iommufd_ioctl_ops[] = { > val64), > IOCTL_OP(IOMMU_VFIO_IOAS, iommufd_vfio_ioas, struct > iommu_vfio_ioas, > __reserved), > + IOCTL_OP(IOMMU_FAULT_QUEUE_ALLOC, iommufd_fault_alloc, > struct iommu_fault_alloc, > + out_fault_fd), ditto