Re: [PATCH v6 04/25] iommu: Add a page fault handler

Lu Baolu
2020-05-03T05:49:15Z

Hi Jean,

On 2020/4/30 22:34, Jean-Philippe Brucker wrote:
> Some systems allow devices to handle I/O Page Faults in the core mm. For
> example systems implementing the PCIe PRI extension or Arm SMMU stall
> model. Infrastructure for reporting these recoverable page faults was
> added to the IOMMU core by commit 0c830e6b3282 ("iommu: Introduce device
> fault report API"). Add a page fault handler for host SVA.
>
> IOMMU driver can now instantiate several fault workqueues and link them
> to IOPF-capable devices. Drivers can choose between a single global
> workqueue, one per IOMMU device, one per low-level fault queue, one per
> domain, etc.
>
> When it receives a fault event, supposedly in an IRQ handler, the IOMMU
> driver reports the fault using iommu_report_device_fault(), which calls
> the registered handler. The page fault handler then calls the mm fault
> handler, and reports either success or failure with iommu_page_response().
> When the handler succeeded, the IOMMU retries the access.
>
> The iopf_param pointer could be embedded into iommu_fault_param. But
> putting iopf_param into the iommu_param structure allows us not to care
> about ordering between calls to iopf_queue_add_device() and
> iommu_register_device_fault_handler().
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> v5->v6: Simplify flush. As we're not flushing in the mm exit path
> anymore, we can mandate that IOMMU drivers flush their low-level queue
> themselves before calling iopf_queue_flush_dev(). No need to register
> a flush callback anymore.
> ---
> drivers/iommu/Kconfig | 3 +
> drivers/iommu/Makefile | 1 +
> include/linux/iommu.h | 51 +++++
> drivers/iommu/io-pgfault.c | 383 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 438 insertions(+)
> create mode 100644 drivers/iommu/io-pgfault.c
>

[...]

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

> + }
> +
> + 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(&param->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.

> + else
> + ret = -ENODEV;
> + mutex_unlock(&param->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.

> + }
> + 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(&param->lock);
> + if (!param->iopf_param) {
> + list_add(&iopf_param->queue_list, &queue->devices);
> + param->iopf_param = iopf_param;
> + ret = 0;
> + }
> + mutex_unlock(&param->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(&param->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(&param->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.

> +
> + kfree(iopf_param);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iopf_queue_remove_device);
> +
> +/**
> + * iopf_queue_alloc - Allocate and initialize a fault queue
> + * @name: a unique string identifying the queue (for workqueue)
> + *
> + * Return: the queue on success and NULL on error.
> + */
> +struct iopf_queue *iopf_queue_alloc(const char *name)
> +{
> + struct iopf_queue *queue;
> +
> + queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> + if (!queue)
> + return NULL;
> +
> + /*
> + * The WQ is unordered because the low-level handler enqueues faults by
> + * group. PRI requests within a group have to be ordered, but once
> + * that's dealt with, the high-level function can handle groups out of
> + * order.
> + */
> + queue->wq = alloc_workqueue("iopf_queue/%s", WQ_UNBOUND, 0, name);
> + if (!queue->wq) {
> + kfree(queue);
> + return NULL;
> + }
> +
> + INIT_LIST_HEAD(&queue->devices);
> + mutex_init(&queue->lock);
> +
> + return queue;
> +}
> +EXPORT_SYMBOL_GPL(iopf_queue_alloc);
> +
> +/**
> + * iopf_queue_free - Free IOPF queue
> + * @queue: queue to free
> + *
> + * Counterpart to iopf_queue_alloc(). The driver must not be queuing faults or
> + * adding/removing devices on this queue anymore.
> + */
> +void iopf_queue_free(struct iopf_queue *queue)
> +{
> + struct iopf_device_param *iopf_param, *next;
> +
> + if (!queue)
> + return;
> +
> + list_for_each_entry_safe(iopf_param, next, &queue->devices, queue_list)
> + iopf_queue_remove_device(queue, iopf_param->dev);
> +
> + destroy_workqueue(queue->wq);
> + kfree(queue);
> +}
> +EXPORT_SYMBOL_GPL(iopf_queue_free);
>

Best regards,
baolu