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.
--- drivers/clk/clk-fixed-factor.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 4f7bf3929d6d..390c16f321a6 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -66,7 +66,12 @@ EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
static void devm_clk_hw_register_fixed_factor_release(struct device *dev, void *res) { - clk_hw_unregister_fixed_factor(&((struct clk_fixed_factor *)res)->hw); + /* + * 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); }
static struct clk_hw *
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(-)
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
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?
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?
Quoting Dmitry Baryshkov (2021-04-07 15:57:01)
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?
Yes please send more fixes. They're not high priority though so I'll probably leave them to bake in next for a week or so.
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 *
dri-devel@lists.freedesktop.org
-
Daniel Palmer -
Dmitry Baryshkov -
Stephen Boyd