[PATCH 1/2] misc: sram: fix resource leaks in probe error path

[PATCH 1/2] misc: sram: fix resource leaks in probe error path

[Johan Hovold]

Make sure to disable clocks and deregister any exported partitions
before returning on late probe errors.

Note that since commit ee895ccdf776 ("misc: sram: fix enabled clock leak
on error path"), partitions are deliberately exported before enabling
the clock so we stick to that logic here. A follow up patch will address
this.

Fixes: 2ae2e28852f2 ("misc: sram: add Atmel securam support")
Cc: stable <stable@vger.kernel.org>     # 4.9
Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/misc/sram.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index c5dc6095686a..679647713e36 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -407,13 +407,20 @@ static int sram_probe(struct platform_device *pdev)
 	if (init_func) {
 		ret = init_func();
 		if (ret)
-			return ret;
+			goto err_disable_clk;
 	}
 
 	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
 		gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
+
+err_disable_clk:
+	if (sram->clk)
+		clk_disable_unprepare(sram->clk);
+	sram_free_partitions(sram);
+
+	return ret;
 }
 
 static int sram_remove(struct platform_device *pdev)
-- 
2.18.0

[PATCH 2/2] misc: sram: enable clock before registering regions

[Johan Hovold]

Make sure to enable the clock before registering regions and exporting
partitions to user space at which point we must be prepared for I/O.

Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/misc/sram.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 679647713e36..74b183baf044 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -391,23 +391,23 @@ static int sram_probe(struct platform_device *pdev)
 	if (IS_ERR(sram->pool))
 		return PTR_ERR(sram->pool);
 
-	ret = sram_reserve_regions(sram, res);
-	if (ret)
-		return ret;
-
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
 		sram->clk = NULL;
 	else
 		clk_prepare_enable(sram->clk);
 
+	ret = sram_reserve_regions(sram, res);
+	if (ret)
+		goto err_disable_clk;
+
 	platform_set_drvdata(pdev, sram);
 
 	init_func = of_device_get_match_data(&pdev->dev);
 	if (init_func) {
 		ret = init_func();
 		if (ret)
-			goto err_disable_clk;
+			goto err_free_partitions;
 	}
 
 	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
@@ -415,10 +415,11 @@ static int sram_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_free_partitions:
+	sram_free_partitions(sram);
 err_disable_clk:
 	if (sram->clk)
 		clk_disable_unprepare(sram->clk);
-	sram_free_partitions(sram);
 
 	return ret;
 }
-- 
2.18.0

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

Hi Johan,

On 07/03/2018 01:05 PM, Johan Hovold wrote:
> Make sure to enable the clock before registering regions and exporting
> partitions to user space at which point we must be prepared for I/O.
> 
> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

thank you for the change, however please note that the identified commit
for the fix is incorrect one apparently.

In my opinion the proper tag contents would be

Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")

I hope you agree to it, also I would suggest to swap the changes in
the series.

--
Best wishes,
Vladimir

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Johan Hovold]

On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
> Hi Johan,
> 
> On 07/03/2018 01:05 PM, Johan Hovold wrote:
> > Make sure to enable the clock before registering regions and exporting
> > partitions to user space at which point we must be prepared for I/O.
> > 
> > Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> > Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> thank you for the change, however please note that the identified commit
> for the fix is incorrect one apparently.
> 
> In my opinion the proper tag contents would be
> 
> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
> 
> I hope you agree to it, also I would suggest to swap the changes in
> the series.

No, I think I used the right commit in the Fixes tag as that was the
commit which moved the clock enable to after the memory-region
registration (at which point the memory could potentially be accessed).

Johan

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

On 07/03/2018 02:47 PM, Johan Hovold wrote:
> On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
>> Hi Johan,
>>
>> On 07/03/2018 01:05 PM, Johan Hovold wrote:
>>> Make sure to enable the clock before registering regions and exporting
>>> partitions to user space at which point we must be prepared for I/O.
>>>
>>> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
>>> Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>> Signed-off-by: Johan Hovold <johan@kernel.org>
>>
>> thank you for the change, however please note that the identified commit
>> for the fix is incorrect one apparently.
>>
>> In my opinion the proper tag contents would be
>>
>> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
>>
>> I hope you agree to it, also I would suggest to swap the changes in
>> the series.
> 
> No, I think I used the right commit in the Fixes tag as that was the
> commit which moved the clock enable to after the memory-region
> registration (at which point the memory could potentially be accessed).

I was confused by the moved sram_reserve_regions() call, which was added
way later.

Allright, if it is assumed that gen_pool_get() interface requires only
a registered memory pool provider device, and it does, then there is
another kind of a problem, a SRAM/genpool consumer may not get access
to a valid region in SRAM before the latter is added to the SRAM pool
in sram_probe().

Instantly I don't know how to solve the issue above, it may require
a change to lib/genalloc.c to request a registration of genpool device
driver, but then such a change solves the problem identified by you
as well.

For your change as a proper (partial?) fix:

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

--
Best wishes,
Vladimir

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Johan Hovold]

On Tue, Jul 03, 2018 at 03:30:09PM +0300, Vladimir Zapolskiy wrote:
> On 07/03/2018 02:47 PM, Johan Hovold wrote:
> > On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
> >> Hi Johan,
> >>
> >> On 07/03/2018 01:05 PM, Johan Hovold wrote:
> >>> Make sure to enable the clock before registering regions and exporting
> >>> partitions to user space at which point we must be prepared for I/O.
> >>>
> >>> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> >>> Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >>> Signed-off-by: Johan Hovold <johan@kernel.org>
> >>
> >> thank you for the change, however please note that the identified commit
> >> for the fix is incorrect one apparently.
> >>
> >> In my opinion the proper tag contents would be
> >>
> >> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
> >>
> >> I hope you agree to it, also I would suggest to swap the changes in
> >> the series.
> > 
> > No, I think I used the right commit in the Fixes tag as that was the
> > commit which moved the clock enable to after the memory-region
> > registration (at which point the memory could potentially be accessed).
> 
> I was confused by the moved sram_reserve_regions() call, which was added
> way later.
> 
> Allright, if it is assumed that gen_pool_get() interface requires only
> a registered memory pool provider device, and it does, then there is
> another kind of a problem, a SRAM/genpool consumer may not get access
> to a valid region in SRAM before the latter is added to the SRAM pool
> in sram_probe().

Right, this whole genpool interface is fragile, but that's a different
story.

> Instantly I don't know how to solve the issue above, it may require
> a change to lib/genalloc.c to request a registration of genpool device
> driver, but then such a change solves the problem identified by you
> as well.

The resource (genpool) should not be registered before it's been fully
initialised, while any prior attempts to look it up could cause the
consumer driver to defer their probes, for example. But again, that's
beyond the scope here.

> For your change as a proper (partial?) fix:
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Thanks for the review!

Johan