On Sun, May 03, 2020 at 01:49:01PM +0800, Lu Baolu wrote:
> > +static void iopf_handle_group(struct work_struct *work)
> > +{
> > + struct iopf_group *group;
> > + struct iopf_fault *iopf, *next;
> > + enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
> > +
> > + group = container_of(work, struct iopf_group, work);
> > +
> > + list_for_each_entry_safe(iopf, next, &group->faults, head) {
> > + /*
> > + * For the moment, errors are sticky: don't handle subsequent
> > + * faults in the group if there is an error.
> > + */
> > + if (status == IOMMU_PAGE_RESP_SUCCESS)
> > + status = iopf_handle_single(iopf);
> > +
> > + if (!(iopf->fault.prm.flags &
> > + IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
> > + kfree(iopf);
>
> The iopf is freed,but not removed from the list. This will cause wild
> pointer in code.
We free the list with the group below, so this one is fine.
>
> > + }
> > +
> > + iopf_complete_group(group->dev, &group->last_fault, status);
> > + kfree(group);
> > +}
> > +
>
> [...]
>
> > +/**
> > + * iopf_queue_flush_dev - Ensure that all queued faults have been processed
> > + * @dev: the endpoint whose faults need to be flushed.
> > + * @pasid: the PASID affected by this flush
> > + *
> > + * The IOMMU driver calls this before releasing a PASID, to ensure that all
> > + * pending faults for this PASID have been handled, and won't hit the address
> > + * space of the next process that uses this PASID. The driver must make sure
> > + * that no new fault is added to the queue. In particular it must flush its
> > + * low-level queue before calling this function.
> > + *
> > + * Return: 0 on success and <0 on error.
> > + */
> > +int iopf_queue_flush_dev(struct device *dev, int pasid)
> > +{
> > + int ret = 0;
> > + struct iopf_device_param *iopf_param;
> > + struct dev_iommu *param = dev->iommu;
> > +
> > + if (!param)
> > + return -ENODEV;
> > +
> > + mutex_lock(¶m->lock);
> > + iopf_param = param->iopf_param;
> > + if (iopf_param)
> > + flush_workqueue(iopf_param->queue->wq);
>
> There may be other pasid iopf in the workqueue. Flush all tasks in
> the workqueue will hurt other pasids. I might lose any context.
Granted this isn't optimal because we don't take the PASID argument into
account (I think I'll remove it, don't know how to use it). But I don't
think it affects other PASIDs, because all flush_workqueue() does is wait
until all faults currently in the worqueue are processed. So it only
blocks the current thread, but nothing is lost.
>
> > + else
> > + ret = -ENODEV;
> > + mutex_unlock(¶m->lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
> > +
> > +/**
> > + * iopf_queue_discard_partial - Remove all pending partial fault
> > + * @queue: the queue whose partial faults need to be discarded
> > + *
> > + * When the hardware queue overflows, last page faults in a group may have been
> > + * lost and the IOMMU driver calls this to discard all partial faults. The
> > + * driver shouldn't be adding new faults to this queue concurrently.
> > + *
> > + * Return: 0 on success and <0 on error.
> > + */
> > +int iopf_queue_discard_partial(struct iopf_queue *queue)
> > +{
> > + struct iopf_fault *iopf, *next;
> > + struct iopf_device_param *iopf_param;
> > +
> > + if (!queue)
> > + return -EINVAL;
> > +
> > + mutex_lock(&queue->lock);
> > + list_for_each_entry(iopf_param, &queue->devices, queue_list) {
> > + list_for_each_entry_safe(iopf, next, &iopf_param->partial, head)
> > + kfree(iopf);
>
> iopf is freed but not removed from the list.
Ouch yes this is wrong, will fix.
>
> > + }
> > + mutex_unlock(&queue->lock);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
> > +
> > +/**
> > + * iopf_queue_add_device - Add producer to the fault queue
> > + * @queue: IOPF queue
> > + * @dev: device to add
> > + *
> > + * Return: 0 on success and <0 on error.
> > + */
> > +int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
> > +{
> > + int ret = -EBUSY;
> > + struct iopf_device_param *iopf_param;
> > + struct dev_iommu *param = dev->iommu;
> > +
> > + if (!param)
> > + return -ENODEV;
> > +
> > + iopf_param = kzalloc(sizeof(*iopf_param), GFP_KERNEL);
> > + if (!iopf_param)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&iopf_param->partial);
> > + iopf_param->queue = queue;
> > + iopf_param->dev = dev;
> > +
> > + mutex_lock(&queue->lock);
> > + mutex_lock(¶m->lock);
> > + if (!param->iopf_param) {
> > + list_add(&iopf_param->queue_list, &queue->devices);
> > + param->iopf_param = iopf_param;
> > + ret = 0;
> > + }
> > + mutex_unlock(¶m->lock);
> > + mutex_unlock(&queue->lock);
> > +
> > + if (ret)
> > + kfree(iopf_param);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(iopf_queue_add_device);
> > +
> > +/**
> > + * iopf_queue_remove_device - Remove producer from fault queue
> > + * @queue: IOPF queue
> > + * @dev: device to remove
> > + *
> > + * Caller makes sure that no more faults are reported for this device.
> > + *
> > + * Return: 0 on success and <0 on error.
> > + */
> > +int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
> > +{
> > + int ret = 0;
> > + struct iopf_fault *iopf, *next;
> > + struct iopf_device_param *iopf_param;
> > + struct dev_iommu *param = dev->iommu;
> > +
> > + if (!param || !queue)
> > + return -EINVAL;
> > +
> > + mutex_lock(&queue->lock);
> > + mutex_lock(¶m->lock);
> > + iopf_param = param->iopf_param;
> > + if (iopf_param && iopf_param->queue == queue) {
> > + list_del(&iopf_param->queue_list);
> > + param->iopf_param = NULL;
> > + } else {
> > + ret = -EINVAL;
> > + }
> > + mutex_unlock(¶m->lock);
> > + mutex_unlock(&queue->lock);
> > + if (ret)
> > + return ret;
> > +
> > + /* Just in case some faults are still stuck */
> > + list_for_each_entry_safe(iopf, next, &iopf_param->partial, head)
> > + kfree(iopf);
>
> The same here.
Here is fine, we free the iopf_param below
Thanks,
Jean
>
> > +
> > + kfree(iopf_param);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iopf_queue_remove_device);
