linux-kernel - RE: [PATCH v6 06/10] iommufd: Add iommufd fault object

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 7 Jun 2024 09:17:28 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Jason Gunthorpe <jgg@...pe.ca>,
	"Joerg Roedel" <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy
	<robin.murphy@....com>, Jean-Philippe Brucker <jean-philippe@...aro.org>,
	Nicolin Chen <nicolinc@...dia.com>, "Liu, Yi L" <yi.l.liu@...el.com>, "Jacob
 Pan" <jacob.jun.pan@...ux.intel.com>, Joel Granados <j.granados@...sung.com>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"virtualization@...ts.linux-foundation.org"
	<virtualization@...ts.linux-foundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 06/10] iommufd: Add iommufd fault object

> From: Lu Baolu <baolu.lu@...ux.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."

> +
> +	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