Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Halil Pasic <pasic@linux.ibm.com>
To:     Cornelia Huck <cohuck@redhat.com>,
        Halil Pasic <pasic@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>,
        Michael Mueller <mimu@linux.ibm.com>,
        linux-s390@vger.kernel.org,
        virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     bfu@redhat.com, Vineeth Vijayan <vneethv@linux.ibm.com>
Subject: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
Date:   Wed, 15 Sep 2021 23:57:42 +0200
Message-Id: <20210915215742.1793314-1-pasic@linux.ibm.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

To accomplish this let us take a reference on the ccw device before we
allocate the dma_area and give it up after dma_area was freed.

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

I'm not certain this is the only hot-unplug and teardonw related problem
with virtio-ccw.

Some things that are not perfectly clear to me:
* What would happen if we observed an hot-unplug while we are doing
  wait_event() in ccw_io_helper()? Do we get stuck? I don't thin we
  are guaranteed to receive an irq for a subchannel that is gone.
* cdev->online seems to be manipulated under cdev->ccwlock, but
  in virtio_ccw_remove() we look at it to decide should we clean up
  or not. What is the idea there? I guess we want to avoid doing
  if nothing is there or twice. But I don't understand how stuff
  interlocks.
* Can virtio_ccw_remove() get called while !cdev->online and 
  virtio_ccw_online() is running on a different cpu? If yes, what would
  happen then?
 
The main addresse of these questions is Conny ;).

An alternative to this approach would be to inc and dec the refcount
in ccw_device_dma_zalloc() and ccw_device_dma_free() respectively.

---
 drivers/s390/virtio/virtio_ccw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index d35e7a3f7067..99141df3259b 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1006,10 +1006,12 @@ static void virtio_ccw_release_dev(struct device *_d)
 {
 	struct virtio_device *dev = dev_to_virtio(_d);
 	struct virtio_ccw_device *vcdev = to_vc_device(dev);
+	struct ccw_device *cdev = READ_ONCE(vcdev->cdev);
 
 	ccw_device_dma_free(vcdev->cdev, vcdev->dma_area,
 			    sizeof(*vcdev->dma_area));
 	kfree(vcdev);
+	put_device(&cdev->dev);
 }
 
 static int irb_is_error(struct irb *irb)
@@ -1262,6 +1264,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	struct virtio_ccw_device *vcdev;
 	unsigned long flags;
 
+	get_device(&cdev->dev);
 	vcdev = kzalloc(sizeof(*vcdev), GFP_KERNEL);
 	if (!vcdev) {
 		dev_warn(&cdev->dev, "Could not get memory for virtio\n");
@@ -1315,6 +1318,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 				    sizeof(*vcdev->dma_area));
 	}
 	kfree(vcdev);
+	put_device(&cdev->dev);
 	return ret;
 }
 

base-commit: 3ca706c189db861b2ca2019a0901b94050ca49d8
-- 
2.25.1