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

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

[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: <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
-- 
2.25.1

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

[Pierre Morel]

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
> 

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

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.

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

[Halil Pasic]

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

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

[Halil Pasic]

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

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

[Vineeth Vijayan]

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

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

[Cornelia Huck]

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.

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

[Pierre Morel]

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

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Halil Pasic]

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

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

[Halil Pasic]

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

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

[Cornelia Huck]

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>

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

[Vineeth Vijayan]

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