* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
@ 2021-04-07 9:00 ` Dmitry Baryshkov
2021-04-07 11:38 ` Daniel Palmer
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-07 9:00 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette
Cc: open list:DRM DRIVER FOR MSM ADRENO GPU,
open list:COMMON CLK FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, freedreno, Rob Clark,
Daniel Palmer
On Wed, 7 Apr 2021 at 02:06, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
>
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>
Forgot:
Fixes: 0b9266d295ce ("clk: fixed: add devm helper for
clk_hw_register_fixed_factor()")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>
> Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> getting it into 5.12, otherwise there will be a cross-dependency between
> msm-next and clk-next.
>
> ---
> drivers/clk/clk-fixed-factor.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
2021-04-07 9:00 ` Dmitry Baryshkov
@ 2021-04-07 11:38 ` Daniel Palmer
2021-04-07 22:41 ` Stephen Boyd
2021-04-07 23:01 ` Stephen Boyd
3 siblings, 0 replies; 8+ messages in thread
From: Daniel Palmer @ 2021-04-07 11:38 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette,
linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark
Hi Dmitry,
On Wed, 7 Apr 2021 at 08:06, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
Doh.
Sorry for not spotting this when I wrote the patch.
Thank you for cleaning up after me.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
2021-04-07 9:00 ` Dmitry Baryshkov
2021-04-07 11:38 ` Daniel Palmer
@ 2021-04-07 22:41 ` Stephen Boyd
2021-04-07 22:57 ` Dmitry Baryshkov
2021-04-07 23:01 ` Stephen Boyd
3 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2021-04-07 22:41 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Michael Turquette
Cc: linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark, Daniel Palmer
Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
>
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>
> Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> getting it into 5.12, otherwise there will be a cross-dependency between
> msm-next and clk-next.
Think I can get this into the last fixes PR. One question though, I
think this follows the pattern that things like clk-divider.c use for
devm. Are those also broken?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
2021-04-07 22:41 ` Stephen Boyd
@ 2021-04-07 22:57 ` Dmitry Baryshkov
[not found] ` <161783803315.3790633.10829887417379757624@swboyd.mtv.corp.google.com>
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-07 22:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andy Gross, Bjorn Andersson, Michael Turquette,
open list:DRM DRIVER FOR MSM ADRENO GPU,
open list:COMMON CLK FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, freedreno, Rob Clark,
Daniel Palmer
On Thu, 8 Apr 2021 at 01:41, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> > devm_clk_hw_register_fixed_factor_release(), the release function for
> > the devm_clk_hw_register_fixed_factor(), calls
> > clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> > after that the devres functions will also kfree the allocated data,
> > resulting in double free/memory corruption. Just call
> > clk_hw_unregister() instead, leaving kfree() to devres code.
> >
> > Reported-by: Rob Clark <robdclark@chromium.org>
> > Cc: Daniel Palmer <daniel@0x0f.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >
> > Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> > getting it into 5.12, otherwise there will be a cross-dependency between
> > msm-next and clk-next.
>
> Think I can get this into the last fixes PR. One question though, I
> think this follows the pattern that things like clk-divider.c use for
> devm. Are those also broken?
It looks so. See e.g. the devres_release() function. It calls
(*release) callback, then it will kfree the resource.
Also see Documentation/driver-api/driver-model/devres.rst, which does
not kfree() in release functions.
Do you wish for me to send all the fixes?
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
` (2 preceding siblings ...)
2021-04-07 22:41 ` Stephen Boyd
@ 2021-04-07 23:01 ` Stephen Boyd
3 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2021-04-07 23:01 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Michael Turquette
Cc: linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark, Daniel Palmer
Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
>
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Applied to clk-fixes. I also squashed this in to cleanup that ugly cast.
---8<----
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 390c16f321a6..4e4b6d367612 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -66,12 +66,14 @@ EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
static void devm_clk_hw_register_fixed_factor_release(struct device *dev, void *res)
{
+ struct clk_fixed_factor *fix = res;
+
/*
* We can not use clk_hw_unregister_fixed_factor, since it will kfree()
* the hw, resulting in double free. Just unregister the hw and let
* devres code kfree() it.
*/
- clk_hw_unregister(&((struct clk_fixed_factor *)res)->hw);
+ clk_hw_unregister(&fix->hw);
}
static struct clk_hw *
^ permalink raw reply related [flat|nested] 8+ messages in thread