| 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
| ||
|
Date: Sat, 8 Jun 2024 17:58:34 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.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: baolu.lu@...ux.intel.com, "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
On 6/7/24 5:17 PM, Tian, Kevin wrote:
>> 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."
My understanding is that reading zero bytes is likely to check if a file
descriptor is valid and ready for reading without actually taking any
data from it.
In this code, it just returns 0 and it's compatible with the man page.
Or, I overlooked anything?
>
>> +
>> + 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. I will make it like this:
if (copy_to_user(buf + done, &data, fault_size)) {
xa_erase(&fault->response, group->cookie);
rc = -EFAULT;
break;
}
>
>> + 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.
I don't quite follow here. The code is doing the following:
- If done == 0, it means nothing has been copied to user space. This
could be due to two reasons:
1) the user read with a count set to 0, or
2) a failure case.
The code returns 0 for the first case and an error number for the
second.
- If done != 0, some data has been copied to user space. In this case,
the code returns the number of data copied regardless of the value of
rc.
>
>> +
>> +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...
These two refcounts were requested when the fault object was installed
to the fault FD.
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;
These refcounts must then be released when the FD is released.
>> +
>> + 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.
The filep is allocated and initialized together.
>> +
>> + 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
Yes, sure. I wasn't aware of the order.
Best regards,
baolu
Powered by blists - more mailing lists