viommu_remove() resets the device and deletes the virtqueues while
userspace may still hold domains that it maps or attaches, and nothing
stopped a request from being queued - or a drain from walking the queue -
on a virtqueue that was being torn down.
Set ->removed under request_lock before the teardown - the lock a
queueing path holds while it checks liveness and adds the request, and
the one the drain holds for its whole run - check it in the queueing
helper, and let the drain return early when it is set.
A drain that returns early leaves the requests still on the queue
behind, and the device cannot complete them either. Detach them
from the request virtqueue - which unmaps their DMA mappings - and
free them after the device is reset, when it can no longer be using
them, and before the queues are deleted, so that the objects do not
leak.
Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
Signed-off-by: Anlai Lu <[email protected]>
---
drivers/iommu/virtio-iommu.c | 49 ++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index f886941d8f3b..7119030dbcb3 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -42,6 +42,11 @@ struct viommu_dev {
spinlock_t request_lock;
struct list_head requests;
void *evts;
+ /*
+ * Set before the teardown: nothing may be queued or drained after
+ * that.
+ */
+ bool removed;
/* Device configuration */
struct iommu_domain_geometry geometry;
@@ -116,6 +121,16 @@ static struct viommu_domain viommu_identity_domain;
#define to_viommu_domain(domain) \
container_of(domain, struct viommu_domain, domain)
+/*
+ * The device can be removed while its domains still exist (userspace may hold
+ * them for a while), and the queues go away with it: nothing may be queued or
+ * drained after that.
+ */
+static bool viommu_device_live(struct viommu_dev *viommu)
+{
+ return !viommu->removed;
+}
+
static int viommu_get_req_errno(void *buf, size_t len)
{
struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
@@ -206,6 +221,10 @@ static int viommu_sync_req(struct viommu_dev *viommu)
unsigned long flags;
spin_lock_irqsave(&viommu->request_lock, flags);
+ if (!viommu_device_live(viommu)) {
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+ return 0;
+ }
ret = __viommu_sync_req(viommu);
if (ret)
dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
@@ -229,6 +248,9 @@ static int __viommu_queue_req(struct viommu_dev *viommu,
assert_spin_locked(&viommu->request_lock);
+ if (!viommu_device_live(viommu))
+ return -ENODEV;
+
sg_init_one(&top_sg, req->buf, write_offset);
sg_init_one(&bottom_sg, req->buf + write_offset,
req->len - write_offset);
@@ -1569,12 +1591,39 @@ static int viommu_probe(struct virtio_device *vdev)
static void viommu_remove(struct virtio_device *vdev)
{
struct viommu_dev *viommu = vdev->priv;
+ struct viommu_request *req;
+ struct virtqueue *vq;
+ unsigned long flags;
iommu_device_sysfs_remove(&viommu->iommu);
iommu_device_unregister(&viommu->iommu);
+ /*
+ * The queues go away here: nothing may be queued or drained from now
+ * on. Taking request_lock is what tells a drain in flight that it has
+ * to finish before the teardown, since the drain holds it throughout.
+ */
+ spin_lock_irqsave(&viommu->request_lock, flags);
+ viommu->removed = true;
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+
/* Stop all virtqueues */
virtio_reset_device(vdev);
+
+ /*
+ * The device cannot complete the requests still on the queue, and
+ * the drain does not touch them anymore: detach them from the
+ * request virtqueue - which releases their scatterlists - and free
+ * them.
+ */
+ spin_lock_irqsave(&viommu->request_lock, flags);
+ vq = viommu->vqs[VIOMMU_REQUEST_VQ];
+ while ((req = virtqueue_detach_unused_buf(vq))) {
+ list_del(&req->list);
+ kfree(req);
+ }
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+
vdev->config->del_vqs(vdev);
dev_info(&vdev->dev, "device removed\n");
--
2.55.0
