| From ad9a14517263a16af040598c7920c09ca9670a31 Mon Sep 17 00:00:00 2001 |
| From: Halil Pasic <pasic@linux.ibm.com> |
| Date: Wed, 8 Sep 2021 17:36:23 +0200 |
| Subject: s390/cio: make ccw_device_dma_* more robust |
| |
| From: Halil Pasic <pasic@linux.ibm.com> |
| |
| commit ad9a14517263a16af040598c7920c09ca9670a31 upstream. |
| |
| Since commit 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and |
| classic notifiers") we were supposed to make sure that |
| virtio_ccw_release_dev() completes before the ccw device and the |
| attached dma pool are torn down, but unfortunately we did not. Before |
| that commit it used to be OK to delay cleaning up the memory allocated |
| by virtio-ccw indefinitely (which isn't really intuitive for guys used |
| to destruction happens in reverse construction order), but now we |
| trigger a BUG_ON if the genpool is destroyed before all memory allocated |
| from it is deallocated. Which brings down the guest. We can observe this |
| problem, when unregister_virtio_device() does not give up the last |
| reference to the virtio_device (e.g. because a virtio-scsi attached scsi |
| disk got removed without previously unmounting its previously mounted |
| partition). |
| |
| To make sure that the genpool is only destroyed after all the necessary |
| freeing is done let us take a reference on the ccw device on each |
| ccw_device_dma_zalloc() and give it up on each ccw_device_dma_free(). |
| |
| Actually there are multiple approaches to fixing the problem at hand |
| that can work. The upside of this one is that it is the safest one while |
| remaining simple. We don't crash the guest even if the driver does not |
| pair allocations and frees. The downside is the reference counting |
| overhead, that the reference counting for ccw devices becomes more |
| complex, in a sense that we need to pair the calls to the aforementioned |
| functions for it to be correct, and that if we happen to leak, we leak |
| more than necessary (the whole ccw device instead of just the genpool). |
| |
| Some alternatives to this approach are taking a reference in |
| virtio_ccw_online() and giving it up in virtio_ccw_release_dev() or |
| making sure virtio_ccw_release_dev() completes its work before |
| virtio_ccw_remove() returns. The downside of these approaches is that |
| these are less safe against programming errors. |
| |
| Cc: <stable@vger.kernel.org> # v5.3 |
| Signed-off-by: Halil Pasic <pasic@linux.ibm.com> |
| Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and classic notifiers") |
| Reported-by: bfu@redhat.com |
| Reviewed-by: Vineeth Vijayan <vneethv@linux.ibm.com> |
| Acked-by: Cornelia Huck <cohuck@redhat.com> |
| Signed-off-by: Vasily Gorbik <gor@linux.ibm.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/s390/cio/device_ops.c | 12 +++++++++++- |
| 1 file changed, 11 insertions(+), 1 deletion(-) |
| |
| --- a/drivers/s390/cio/device_ops.c |
| +++ b/drivers/s390/cio/device_ops.c |
| @@ -717,13 +717,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_schid); |
| */ |
| void *ccw_device_dma_zalloc(struct ccw_device *cdev, size_t size) |
| { |
| - return cio_gp_dma_zalloc(cdev->private->dma_pool, &cdev->dev, size); |
| + void *addr; |
| + |
| + if (!get_device(&cdev->dev)) |
| + return NULL; |
| + addr = cio_gp_dma_zalloc(cdev->private->dma_pool, &cdev->dev, size); |
| + if (IS_ERR_OR_NULL(addr)) |
| + put_device(&cdev->dev); |
| + return addr; |
| } |
| EXPORT_SYMBOL(ccw_device_dma_zalloc); |
| |
| void ccw_device_dma_free(struct ccw_device *cdev, void *cpu_addr, size_t size) |
| { |
| + if (!cpu_addr) |
| + return; |
| cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size); |
| + put_device(&cdev->dev); |
| } |
| EXPORT_SYMBOL(ccw_device_dma_free); |
| |