linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] drivers/base: wrong return value of dmam_declare_coherent_memory
@ 2016-04-28 14:31 Andrey Gursky
  2016-04-28 15:11 ` Vyacheslav Yurkov
  0 siblings, 1 reply; 3+ messages in thread
From: Andrey Gursky @ 2016-04-28 14:31 UTC (permalink / raw)
  To: Vyacheslav Yurkov; +Cc: linux-kernel, Greg Kroah-Hartman

Vyacheslav,

thanks for your patch.

For now it introduces a new bug.

Vyacheslav Yurkov <uvv.mail <at> gmail.com> writes:

> 
> Hi guys,
> I found an issue in managed version of dma_declare_coherent_memory,
> i.e. in dmam_declare_coherent_memory. It looks like the return value
> of dma_declare_coherent_memory is zero in case of error and a
> requested flag on success,

You should take this into account and not ignore the value of rc at all
by using flags instead, which is not being altered by
dma_declare_coherent_memory() making the latter appear to succeed
always.

> which dmam_* version doesn't take into
> account and this leads to leaking of resources.
> 
> ---
> Yours sincerely,
> Vyacheslav V. Yurkov
> 
> P.S.: I'm not subscribed to the maillist, so please include me in CC
> when responding to this Email.
> 
> ---
> --- linux-4.5.2/drivers/base/dma-mapping.c.orig	2016-04-20
> 08:46:35.000000000 +0200
> +++ linux-4.5.2/drivers/base/dma-mapping.c	2016-04-28 10:15:34.295080491
> +0200
>  <at>  <at>  -198,10 +198,15  <at>  <at>  int dmam_declare_coherent_memory(struct
> 
>  	rc = dma_declare_coherent_memory(dev, phys_addr, device_addr, size,
>  					 flags);
> -	if (rc == 0)
> +
> +	if ((flags & DMA_MEMORY_MAP) == DMA_MEMORY_MAP ||
> +	    (flags & DMA_MEMORY_IO) == DMA_MEMORY_IO) {
>  		devres_add(dev, res);
> -	else
> +		rc = 0;
> +	} else {
>  		devres_free(res);
> +		rc = -ENOMEM;
> +	}
> 
>  	return rc;
>  }
> 
> Signed-off-by: Vyacheslav Yurkov <uvv.mail <at> gmail.com>
> 

Best Regards,
Andrey

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drivers/base: wrong return value of dmam_declare_coherent_memory
  2016-04-28 14:31 [PATCH] drivers/base: wrong return value of dmam_declare_coherent_memory Andrey Gursky
@ 2016-04-28 15:11 ` Vyacheslav Yurkov
  2016-06-14  7:58   ` [PATCH] [RESEND] drivers/base dmam_declare_coherent_memory leaks Vyacheslav V. Yurkov
  0 siblings, 1 reply; 3+ messages in thread
From: Vyacheslav Yurkov @ 2016-04-28 15:11 UTC (permalink / raw)
  To: Andrey Gursky; +Cc: linux-kernel, Greg Kroah-Hartman

On 28.04.2016 16:31, Andrey Gursky wrote:
> Vyacheslav,
> 
> thanks for your patch.
> 
> For now it introduces a new bug.
> 
> Vyacheslav Yurkov <uvv.mail <at> gmail.com> writes:
> 
>>
>> Hi guys,
>> I found an issue in managed version of dma_declare_coherent_memory,
>> i.e. in dmam_declare_coherent_memory. It looks like the return value
>> of dma_declare_coherent_memory is zero in case of error and a
>> requested flag on success,
> 
> You should take this into account and not ignore the value of rc at all
> by using flags instead, which is not being altered by
> dma_declare_coherent_memory() making the latter appear to succeed
> always.
> 
> 

Hi Andrey,
Thanks for spotting this. My intention was to compare rc of course,
not the flags. While thinking it over I came up with improved version,
which is inlined below.
--- 
Yours sincerely,
Vyacheslav V. Yurkov

P.S.: I'm not subscribed to the maillist, so please include me in CC
when responding to this Email.

---
--- linux-4.5.2/drivers/base/dma-mapping.c.orig	2016-04-20 08:46:35.000000000 +0200
+++ linux-4.5.2/drivers/base/dma-mapping.c	2016-04-28 16:55:13.211945276 +0200
@@ -198,10 +198,14 @@ int dmam_declare_coherent_memory(struct
 
 	rc = dma_declare_coherent_memory(dev, phys_addr, device_addr, size,
 					 flags);
-	if (rc == 0)
+
+	if ((rc & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) != 0) {
 		devres_add(dev, res);
-	else
+		rc = 0;
+	} else {
 		devres_free(res);
+		rc = -ENOMEM;
+	}
 
 	return rc;
 }
 
Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] [RESEND] drivers/base dmam_declare_coherent_memory leaks
  2016-04-28 15:11 ` Vyacheslav Yurkov
@ 2016-06-14  7:58   ` Vyacheslav V. Yurkov
  0 siblings, 0 replies; 3+ messages in thread
From: Vyacheslav V. Yurkov @ 2016-06-14  7:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, Vyacheslav V. Yurkov

dmam_declare_coherent_memory doesn't take into account the return
value of dma_declare_coherent_memory, which leads to incorrect resource
handling

Signed-off-by: Vyacheslav V. Yurkov <uvv.mail@gmail.com>
---
 drivers/base/dma-mapping.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index d799662..f5d2132 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -198,10 +198,13 @@ int dmam_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
 
 	rc = dma_declare_coherent_memory(dev, phys_addr, device_addr, size,
 					 flags);
-	if (rc == 0)
+	if (rc) {
 		devres_add(dev, res);
-	else
+		rc = 0;
+	} else {
 		devres_free(res);
+		rc = -ENOMEM;
+	}
 
 	return rc;
 }
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-06-14  7:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-28 14:31 [PATCH] drivers/base: wrong return value of dmam_declare_coherent_memory Andrey Gursky
2016-04-28 15:11 ` Vyacheslav Yurkov
2016-06-14  7:58   ` [PATCH] [RESEND] drivers/base dmam_declare_coherent_memory leaks Vyacheslav V. Yurkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).