s390/cio: make ccw_device_dma_* more robust

RFC PATCH In review Halil Pasic 4 participants 12 messages Oct 11, 2021 – Oct 13, 2021

Subsystems (3)

Stable branch s390 s390 common I/O layer

lore

Share & tools

lore

Halil Pasic

author

2021-10-11 12:00 UTC #0 lore raw

RFC PATCH 1/1 s390/cio: make ccw_device_dma_* more robust

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 form it. 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 <[email protected]> # v5.3

Signed-off-by Halil Pasic <[email protected]>

Fixes 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and

classic notifiers")

Reported-by [email protected]

---

FYI I've proposed a different fix to this very same problem:
https://lore.kernel.org/lkml/[email protected]/

This patch is more or less a result of that discussion.

---
 drivers/s390/cio/device_ops.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/s390/cio/device_ops.c +11 -1

index 0fe7b2f2e7f5..c533d1dadc6b 100644
@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
  */
 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);

base-commit: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc

Signature · 1 line

2.25.1

Pierre Morel

2021-10-11 13:46 UTC #1 lore raw

Quoted 53 lines · Halil Pasic

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 form it. 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 <[email protected]> # v5.3
Signed-off-by Halil Pasic <[email protected]>
Fixes 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers")
Reported-by [email protected]
---
FYI I've proposed a different fix to this very same problem:
https://lore.kernel.org/lkml/[email protected]/
This patch is more or less a result of that discussion.
---
  drivers/s390/cio/device_ops.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)
drivers/s390/cio/device_ops.c +0 -0
index 0fe7b2f2e7f5..c533d1dadc6b 100644

drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 833

Quoted 10 lines

drivers/s390/cio/device_ops.c +6 -1

@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
   */
  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))

I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.

So shouldn't we modify this function and just test for a NULL address here?

here what I mean:---------------------------------

drivers/s390/cio/css.c +0 -0

index 2bc55ccf3f23..b45fbaa7131b 100644
@@ -1176,7 +1176,7 @@ void *cio_gp_dma_zalloc(struct gen_pool *gp_dma, struct device *dma_dev,
                chunk_size = round_up(size, PAGE_SIZE);
                addr = (unsigned long) dma_alloc_coherent(dma_dev,
                                         chunk_size, &dma_addr,

CIO_DMA_GFP);
- if (!addr)
+ if (IS_ERR_OR_NULL(addr))

                        return NULL;
                gen_pool_add_virt(gp_dma, addr, dma_addr, chunk_size, -1);
                addr = gen_pool_alloc(gp_dma, size);

---------------------------------

drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 834

Quoted 1 line

drivers/s390/cio/device_ops.c +1 -0
+		put_device(&cdev->dev);

addr is not null if addr is ERR.

drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 835

Quoted 1 line

drivers/s390/cio/device_ops.c +1 -0
+	return addr;

may be return IS_ERR_OR_NULL(addr)? NULL : addr;

drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 842

Quoted 7 lines

drivers/s390/cio/device_ops.c +2 -0

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

no need, cpu_addr is already tested in cio_gp_dma_free()

Quoted 8 lines

drivers/s390/cio/device_ops.c +1 -0
  	cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);
+	put_device(&cdev->dev);
  }
  EXPORT_SYMBOL(ccw_device_dma_free);
 
base-commit: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc

Signature · 2 lines

Pierre Morel
IBM Lab Boeblingen

Cornelia Huck

2021-10-11 14:34 UTC #2 lore raw

Quoted 12 lines · Pierre Morel

Quoted 11 lines · Halil Pasic

drivers/s390/cio/device_ops.c +2 -1

index 0fe7b2f2e7f5..c533d1dadc6b 100644
@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
   */
  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;
+

Quoted 8 lines

drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 833
Quoted 4 lines
drivers/s390/cio/device_ops.c +4 -0
+	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))
I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.

Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?

Quoted 2 lines

So shouldn't we modify this function and just test for a NULL address here?

If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.

Halil Pasic

author

2021-10-11 18:49 UTC #3 lore raw

Quoted 23 lines · On Mon, 11 Oct 2021 16:33:45 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 12 lines · Pierre Morel
Quoted 11 lines · Halil Pasic
drivers/s390/cio/device_ops.c +2 -1
index 0fe7b2f2e7f5..c533d1dadc6b 100644
@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
   */
  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;
+
Quoted 8 lines
drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 833
Quoted 4 lines
drivers/s390/cio/device_ops.c +4 -0
+	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))  
I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.

Quoted 2 lines

Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?

Yes, that is what is documented.

Quoted 7 lines

Quoted 3 lines
So shouldn't we modify this function and just test for a NULL address here?
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.

I don't think the extra care will hurt us too badly. I prefer to keep the IS_ERR_OR_NULL() check because it needs less domain specific knowledge to be understood, and because it is more robust.

Regards,
Halil

Cornelia Huck

2021-10-12 13:50 UTC #4 lore raw

Quoted 35 lines · Halil Pasic

Quoted 23 lines · On Mon, 11 Oct 2021 16:33:45 +0200 Cornelia Huck <[email protected]> wrote:
Quoted 12 lines · Pierre Morel
Quoted 11 lines · Halil Pasic
drivers/s390/cio/device_ops.c +2 -1
index 0fe7b2f2e7f5..c533d1dadc6b 100644
@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
   */
  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;
+
Quoted 8 lines
drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 833
Quoted 4 lines
drivers/s390/cio/device_ops.c +4 -0
+	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))  
I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.
Quoted 2 lines
Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?
Yes, that is what is documented.
Quoted 4 lines
Quoted 3 lines
So shouldn't we modify this function and just test for a NULL address here?

Quoted 7 lines

Quoted 3 lines
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.
I don't think the extra care will hurt us too badly. I prefer to keep the IS_ERR_OR_NULL() check because it needs less domain specific knowledge to be understood, and because it is more robust.

It feels weird, though -- I'd rather have a comment that tells me exactly what cio_gp_dma_zalloc() is supposed to return; I would have expected that a _zalloc function always gives me a valid pointer or NULL.

Halil Pasic

author

2021-10-12 22:37 UTC #5 lore raw

Quoted 9 lines · On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 7 lines
Quoted 3 lines
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.
I don't think the extra care will hurt us too badly. I prefer to keep the IS_ERR_OR_NULL() check because it needs less domain specific knowledge to be understood, and because it is more robust.
It feels weird, though -- I'd rather have a comment that tells me

This way the change feels simpler and safer to me. I believe I explained the why above. But if you insist I can change it. I double checked the cio_gp_dma_zalloc() code, and more or less the code called by it. So now I don't feel uncomfortable with the simpler check.

On the other hand, I'm not very happy doing changes solely based on somebody's feelings. It would feel much more comfortable with a reason based discussion.

One reason to change this to a simple NULL check, is that the IS_ERR_OR_NULL() check could upset the reader of the client code, which only checks for NULL.

On the other hand I do believe we have some risk of lumping together different errors here. E.g. dma_pool is NULL or dma ops are not set up properly. Currently we would communicate that kind of a problem as -ENOMEM, which wouldn't be a great match. But since dma_alloc_coherent() returns either NULL or a valid pointer, and furthermore this looks like a common thing in all the mm-api, I decided to be inline with that.

TLDR; If you insist, I will change this to a simple null pointer check.

Quoted 3 lines

exactly what cio_gp_dma_zalloc() is supposed to return; I would have expected that a _zalloc function always gives me a valid pointer or NULL.

I don't think we have such a comment for dma_alloc_coherent() or even kmalloc(). I agree, it would be nice to have this behavior documented in the apidoc all over the place. But IMHO that is a different issue.

Regards,
Halil

Cornelia Huck

Acked-by

2021-10-13 06:52 UTC #6 lore raw

Quoted 35 lines · Halil Pasic

Quoted 9 lines · On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines
Quoted 3 lines
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.
I don't think the extra care will hurt us too badly. I prefer to keep the IS_ERR_OR_NULL() check because it needs less domain specific knowledge to be understood, and because it is more robust.
It feels weird, though -- I'd rather have a comment that tells me
This way the change feels simpler and safer to me. I believe I explained the why above. But if you insist I can change it. I double checked the cio_gp_dma_zalloc() code, and more or less the code called by it. So now I don't feel uncomfortable with the simpler check.
On the other hand, I'm not very happy doing changes solely based on somebody's feelings. It would feel much more comfortable with a reason based discussion.
One reason to change this to a simple NULL check, is that the IS_ERR_OR_NULL() check could upset the reader of the client code, which only checks for NULL.
On the other hand I do believe we have some risk of lumping together different errors here. E.g. dma_pool is NULL or dma ops are not set up properly. Currently we would communicate that kind of a problem as -ENOMEM, which wouldn't be a great match. But since dma_alloc_coherent() returns either NULL or a valid pointer, and furthermore this looks like a common thing in all the mm-api, I decided to be inline with that.
TLDR; If you insist, I will change this to a simple null pointer check.

Quoted 7 lines

Quoted 3 lines
exactly what cio_gp_dma_zalloc() is supposed to return; I would have expected that a _zalloc function always gives me a valid pointer or NULL.
I don't think we have such a comment for dma_alloc_coherent() or even kmalloc(). I agree, it would be nice to have this behavior documented in the apidoc all over the place. But IMHO that is a different issue.

So, I think that a function returning NULL/valid pointer is the more expected case, and functions that can return an error as well should document this. But it's not really worth arguing more about this, as this is not my code anyway, and your patch does look correct.

Acked-by Cornelia Huck <[email protected]>

Pierre Morel

2021-10-12 14:11 UTC #7 lore raw

In reply to Cornelia Huck

Quoted 23 lines · Cornelia Huck

Quoted 12 lines · Pierre Morel
Quoted 11 lines · Halil Pasic
drivers/s390/cio/device_ops.c +2 -1
index 0fe7b2f2e7f5..c533d1dadc6b 100644
@@ -825,13 +825,23 @@ EXPORT_SYMBOL_GPL(ccw_device_get_chid);
    */
   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;
+
Quoted 8 lines
drivers/s390/cio/device_ops.c EXPORT_SYMBOL_GPL(ccw_device_get_chid); · line 833
Quoted 4 lines
drivers/s390/cio/device_ops.c +4 -0
+	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))
I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.

Quoted 2 lines

Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?

hum, my bad, checked the wrong function, should have use my glasses or connect my brain.

Quoted 7 lines

Quoted 3 lines
So shouldn't we modify this function and just test for a NULL address here?
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.

Signature · 2 lines

Pierre Morel
IBM Lab Boeblingen

Halil Pasic

author

2021-10-11 18:43 UTC #8 lore raw

In reply to Pierre Morel

Quoted 5 lines · On Mon, 11 Oct 2021 15:45:55 +0200 Pierre Morel <[email protected]> wrote:

Review
Quoted 1 line
(quoted diff) +1 -0
+	if (IS_ERR_OR_NULL(addr))  
I can be wrong but it seems that only dma_alloc_coherent() used in cio_gp_dma_zalloc() report an error but the error is ignored and used as a valid pointer.

https://www.kernel.org/doc/Documentation/DMA-API.txt says:

Part Ia - Using large DMA-coherent buffers
------------------------------------------

void *
dma_alloc_coherent(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t flag)

[..]

It returns a pointer to the allocated region (in the processor's virtual address space) or NULL if the allocation failed.

I hope that is still true. If not we should fix cio_gp_dma_zalloc().

Quoted 3 lines

So shouldn't we modify this function and just test for a NULL address here?

Isn't IS_ERR_OR_NULL() safer, in a sense that even if we decided to eventually return an error code, this piece of code would be robust and safe?

We may exploit the knowledge that cio_gp_dma_zalloc() either returns NULL or a valid pointer, but doing it like this is IMHO also an option.

Quoted 18 lines

here what I mean:---------------------------------

drivers/s390/cio/css.c +1 -1

index 2bc55ccf3f23..b45fbaa7131b 100644
@@ -1176,7 +1176,7 @@ void *cio_gp_dma_zalloc(struct gen_pool *gp_dma, 
 struct device *dma_dev,
                 chunk_size = round_up(size, PAGE_SIZE);
                 addr = (unsigned long) dma_alloc_coherent(dma_dev,
                                          chunk_size, &dma_addr, 
 CIO_DMA_GFP);
-               if (!addr)
+               if (IS_ERR_OR_NULL(addr))
                         return NULL;

                 gen_pool_add_virt(gp_dma, addr, dma_addr, chunk_size, -1);
                 addr = gen_pool_alloc(gp_dma, size);

Quoted 6 lines

---------------------------------
Review
Quoted 1 line
(quoted diff) +1 -0
+		put_device(&cdev->dev);  
addr is not null if addr is ERR.

Your point?

Quoted 4 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+	return addr;  
may be return IS_ERR_OR_NULL(addr)? NULL : addr;

See above. I don't think that is necessary.

Quoted 10 lines

Review
Quoted 7 lines
  }
  EXPORT_SYMBOL(ccw_device_dma_zalloc);
  
  void ccw_device_dma_free(struct ccw_device *cdev, void *cpu_addr, size_t size)
  {
(quoted diff) +2 -0
+	if (!cpu_addr)
+		return;  
no need, cpu_addr is already tested in cio_gp_dma_free()

This is added in because of the put_device(). An alternative would be to call cio_gp_dma_free() unconditionally do the check just for the put_device(). But I like this one better.

Thanks for your feedback!

Halil

Quoted 6 lines

  	cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);

(quoted diff) +1 -0

+	put_device(&cdev->dev);
  }
  EXPORT_SYMBOL(ccw_device_dma_free);

Vineeth Vijayan

maintainer Acked-by on v1 1/1

2021-10-12 13:37 UTC #9 lore raw

In reply to Halil Pasic

Looks good. Thanks.

Acked-by Vineeth Vijayan <[email protected]>

Some minor questions below.

Quoted 6 lines · Halil Pasic

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

Quoted 8 lines

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
form it.

allocated from it ?

Quoted 44 lines

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 <[email protected]> # v5.3
Signed-off-by Halil Pasic <[email protected]>
Fixes 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers")
Reported-by [email protected]
---
FYI I've proposed a different fix to this very same problem:
https://lore.kernel.org/lkml/[email protected]/
This patch is more or less a result of that discussion.

Halil Pasic

author

2021-10-12 21:32 UTC #10 lore raw

Quoted 2 lines · On Tue, 12 Oct 2021 15:36:36 +0200 Vineeth Vijayan <[email protected]>…

Looks good. Thanks.
Acked-by Vineeth Vijayan <[email protected]>

Can I convince you to upgrade to Reviewed-by?

Quoted 18 lines

Some minor questions below.
Quoted 6 lines · Halil Pasic
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
Quoted 8 lines
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
form it.

Quoted 1 line

allocated from it ?

Yes. And I think I should add "is deallocated." to the end as well, because we don't destroy memory, we deallocate it ;)

Quoted 45 lines

Quoted 44 lines
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 <[email protected]> # v5.3
Signed-off-by Halil Pasic <[email protected]>
Fixes 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers")
Reported-by [email protected]
---
FYI I've proposed a different fix to this very same problem:
https://lore.kernel.org/lkml/[email protected]/
This patch is more or less a result of that discussion.

Vineeth Vijayan

maintainer Reviewed-by

2021-10-13 07:29 UTC #11 lore raw

Quoted 7 lines · Halil Pasic

Quoted 2 lines · On Tue, 12 Oct 2021 15:36:36 +0200 Vineeth Vijayan <[email protected]>…
Looks good. Thanks.
Acked-by Vineeth Vijayan <[email protected]>
Can I convince you to upgrade to Reviewed-by?

You got it.

Reviewed-by Vineeth Vijayan <[email protected]>

Quoted 23 lines

Quoted 19 lines
Some minor questions below.
Quoted 7 lines · Halil Pasic
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
Quoted 8 lines
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
form it.
Quoted 1 line
allocated from it ?
Yes. And I think I should add "is deallocated." to the end as well, because we don't destroy memory, we deallocate it ;)