2020-04-25 12:57:47

by Xiyu Yang

Subject: [PATCH] mmc: owl-mmc: Fix dma_chan refcnt leak in owl_mmc_probe()

owl_mmc_probe() invokes dma_request_chan(), which returns a reference of
the specified dma_chan object to "owl_host->dma" with increased refcnt.

When owl_mmc_probe() encounters error, it calls mmc_free_host() to free
the "mmc" memory. Since "owl_host" comes from one of "mmc" fields, this
"free" behavior causes "owl_host" and "owl_host->dma" become invalid, so
the refcount for its field should be decreased to keep refcount balanced
before mmc_free_host() calls.

The reference counting issue happens in several exception handling paths
of owl_mmc_probe(). When those error scenarios occur such as failed to
request irq, the function forgets to decrease the refcnt increased by
dma_request_chan(), causing a refcnt leak.

Fix this issue by jumping to "err_put_dma" label when those error
scenarios occur.

Signed-off-by: Xiyu Yang <[email protected]>
Signed-off-by: Xin Tan <[email protected]>
---
drivers/mmc/host/owl-mmc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/owl-mmc.c b/drivers/mmc/host/owl-mmc.c
index 01ffe51f413d..4dc72f5f32f5 100644
--- a/drivers/mmc/host/owl-mmc.c
+++ b/drivers/mmc/host/owl-mmc.c
@@ -635,7 +635,7 @@ static int owl_mmc_probe(struct platform_device *pdev)
owl_host->irq = platform_get_irq(pdev, 0);
if (owl_host->irq < 0) {
ret = -EINVAL;
- goto err_free_host;
+ goto err_put_dma;
}

ret = devm_request_irq(&pdev->dev, owl_host->irq, owl_irq_handler,
@@ -643,19 +643,22 @@ static int owl_mmc_probe(struct platform_device *pdev)
if (ret) {
dev_err(&pdev->dev, "Failed to request irq %d\n",
owl_host->irq);
- goto err_free_host;
+ goto err_put_dma;
}

ret = mmc_add_host(mmc);
if (ret) {
dev_err(&pdev->dev, "Failed to add host\n");
- goto err_free_host;
+ goto err_put_dma;
}

dev_dbg(&pdev->dev, "Owl MMC Controller Initialized\n");

return 0;

+err_put_dma:
+ if (owl_host->dma)
+ dma_release_channel(owl_host->dma);
err_free_host:
mmc_free_host(mmc);

--
2.7.4

2020-04-25 19:35:22

by Markus Elfring

Subject: Re: [PATCH] mmc: owl-mmc: Fix dma_chan refcnt leak in owl_mmc_probe()

> Fix this issue by jumping to "err_put_dma" label when those error
> scenarios occur.

I suggest to reconsider your jump target selection.


…
> +++ b/drivers/mmc/host/owl-mmc.c
…
> @@ -643,19 +643,22 @@ static int owl_mmc_probe(struct platform_device *pdev)
> return 0;
>
> +err_put_dma:
> + if (owl_host->dma)
> + dma_release_channel(owl_host->dma);

I interpret the source code in the way that you would like to call
this function for the desired exception handling only after a call
of the function “dma_request_chan” succeeded.
Thus I would expect that the passed pointer will usually be still valid.
(Can the proposed null pointer check be omitted then?)

How do you think about the following change possibility?

+err_release_channel:
+ dma_release_channel(owl_host->dma);


Would you like to add the tag “Fixes” to the change description?

Regards,
Markus