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: 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
---
FYI I've proposed a different fix to this very same problem: https://lore.kernel.org/lkml/20210915215742.1793314-1-pasic@linux.ibm.com/
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(-)
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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
On 10/11/21 1:59 PM, Halil Pasic wrote:
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: 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
FYI I've proposed a different fix to this very same problem: https://lore.kernel.org/lkml/20210915215742.1793314-1-pasic@linux.ibm.com/
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(-)
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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:---------------------------------
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c index 2bc55ccf3f23..b45fbaa7131b 100644 --- a/drivers/s390/cio/css.c +++ b/drivers/s390/cio/css.c @@ -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);
---------------------------------
put_device(&cdev->dev);
addr is not null if addr is ERR.
- return addr;
may be return IS_ERR_OR_NULL(addr)? NULL : 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;
no need, cpu_addr is already tested in cio_gp_dma_free()
cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);
- put_device(&cdev->dev); } EXPORT_SYMBOL(ccw_device_dma_free);
base-commit: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
On Mon, Oct 11 2021, Pierre Morel pmorel@linux.ibm.com wrote:
On 10/11/21 1:59 PM, Halil Pasic wrote:
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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.
Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?
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.
On Mon, 11 Oct 2021 16:33:45 +0200 Cornelia Huck cohuck@redhat.com wrote:
On Mon, Oct 11 2021, Pierre Morel pmorel@linux.ibm.com wrote:
On 10/11/21 1:59 PM, Halil Pasic wrote:
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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.
Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?
Yes, that is what is documented.
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
On Mon, Oct 11 2021, Halil Pasic pasic@linux.ibm.com wrote:
On Mon, 11 Oct 2021 16:33:45 +0200 Cornelia Huck cohuck@redhat.com wrote:
On Mon, Oct 11 2021, Pierre Morel pmorel@linux.ibm.com wrote:
On 10/11/21 1:59 PM, Halil Pasic wrote:
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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.
Hm, I thought dma_alloc_coherent() returned either NULL or a valid address?
Yes, that is what is documented.
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.
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.
On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck cohuck@redhat.com wrote:
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.
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
On Wed, Oct 13 2021, Halil Pasic pasic@linux.ibm.com wrote:
On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck cohuck@redhat.com wrote:
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.
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 cohuck@redhat.com
On 10/11/21 16:33, Cornelia Huck wrote:
On Mon, Oct 11 2021, Pierre Morel pmorel@linux.ibm.com wrote:
On 10/11/21 1:59 PM, Halil Pasic wrote:
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c index 0fe7b2f2e7f5..c533d1dadc6b 100644 --- a/drivers/s390/cio/device_ops.c +++ b/drivers/s390/cio/device_ops.c @@ -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.
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.
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.
On Mon, 11 Oct 2021 15:45:55 +0200 Pierre Morel pmorel@linux.ibm.com wrote:
- 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().
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.
here what I mean:---------------------------------
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c index 2bc55ccf3f23..b45fbaa7131b 100644 --- a/drivers/s390/cio/css.c +++ b/drivers/s390/cio/css.c @@ -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);
put_device(&cdev->dev);addr is not null if addr is ERR.
Your point?
- return addr;
may be return IS_ERR_OR_NULL(addr)? NULL : addr;
See above. I don't think that is necessary.
} 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()
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
cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);
- put_device(&cdev->dev); } EXPORT_SYMBOL(ccw_device_dma_free);
Looks good. Thanks. Acked-by: Vineeth Vijayan vneethv@linux.ibm.com
Some minor questions below.
On Mon, 2021-10-11 at 13:59 +0200, Halil Pasic wrote:
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.
allocated from 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: 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
FYI I've proposed a different fix to this very same problem: https://lore.kernel.org/lkml/20210915215742.1793314-1-pasic@linux.ibm.com/
This patch is more or less a result of that discussion.
On Tue, 12 Oct 2021 15:36:36 +0200 Vineeth Vijayan vneethv@linux.ibm.com wrote:
Looks good. Thanks. Acked-by: Vineeth Vijayan vneethv@linux.ibm.com
Can I convince you to upgrade to Reviewed-by?
Some minor questions below.
On Mon, 2021-10-11 at 13:59 +0200, Halil Pasic wrote:
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.
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 ;)
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
FYI I've proposed a different fix to this very same problem: https://lore.kernel.org/lkml/20210915215742.1793314-1-pasic@linux.ibm.com/
This patch is more or less a result of that discussion.
On Tue, 2021-10-12 at 23:32 +0200, Halil Pasic wrote:
On Tue, 12 Oct 2021 15:36:36 +0200 Vineeth Vijayan vneethv@linux.ibm.com wrote:
Looks good. Thanks. Acked-by: Vineeth Vijayan vneethv@linux.ibm.com
Can I convince you to upgrade to Reviewed-by?
You got it.
Reviewed-by: Vineeth Vijayan vneethv@linux.ibm.com
Some minor questions below.
On Mon, 2021-10-11 at 13:59 +0200, Halil Pasic wrote:
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.
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 ;)
linux-stable-mirror@lists.linaro.org
-
Cornelia Huck -
Halil Pasic -
Pierre Morel -
Vineeth Vijayan