| Message ID | 20220620171815.114212-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Fix pointer casting to prevent oops in devm_clk_release() | expand |
On 20.06.2022 19:18, Uwe Kleine-König wrote: > The release function is called with a pointer to the memory returned by > devres_alloc(). I was confused about that by the code before the > generalization that used a struct clk **ptr. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote: >>> - clk_put(*(struct clk **)res); >>> + struct devm_clk_state *state = *(struct devm_clk_state **)res; >> This should be: >> >> struct devm_clk_state *state = res; >> >> otherwise it nukes badly during cleanup: >> [...] > How embarrassing. I understood how I confused that, but I wonder how > that didn't pop up earlier. > > FTR: I didn't test that now, but assume you did. My focus now was to get > out an applicable patch fast. > > Thanks for your report > Uwe > > drivers/clk/clk-devres.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c > index c822f4ef1584..1bb086695051 100644 > --- a/drivers/clk/clk-devres.c > +++ b/drivers/clk/clk-devres.c > @@ -11,7 +11,7 @@ struct devm_clk_state { > > static void devm_clk_release(struct device *dev, void *res) > { > - struct devm_clk_state *state = *(struct devm_clk_state **)res; > + struct devm_clk_state *state = res; > > if (state->exit) > state->exit(state->clk); > > base-commit: abae8e57e49aa75f6db76aa866c775721523908f Best regards
Hello, On Tue, Jun 21, 2022 at 08:25:23AM +0200, Marek Szyprowski wrote: > On 20.06.2022 19:18, Uwe Kleine-König wrote: > > The release function is called with a pointer to the memory returned by > > devres_alloc(). I was confused about that by the code before the > > generalization that used a struct clk **ptr. > > > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for your confirmation here. > > --- > > On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote: > >>> - clk_put(*(struct clk **)res); > >>> + struct devm_clk_state *state = *(struct devm_clk_state **)res; > >> This should be: > >> > >> struct devm_clk_state *state = res; > >> > >> otherwise it nukes badly during cleanup: > >> [...] > > How embarrassing. I understood how I confused that, but I wonder how > > that didn't pop up earlier. > > > > FTR: I didn't test that now, but assume you did. My focus now was to get > > out an applicable patch fast. I fear that this issue will break some more tests, so I think it would be good to get this fix quick into next. Best regards Uwe
Hello, On Mon, Jun 20, 2022 at 07:18:15PM +0200, Uwe Kleine-König wrote: > The release function is called with a pointer to the memory returned by > devres_alloc(). I was confused about that by the code before the > generalization that used a struct clk **ptr. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Forwarding from https://lore.kernel.org/linux-clk/CA+G9fYtUu2VCZ2NRpKMV4iCimi8koQ3OTeqQ3byZ9W11sE9fSg@mail.gmail.com: Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> Up to now there were three reports about this breakage. Best regards Uwe
Quoting Uwe Kleine-König (2022-06-20 10:18:15) > The release function is called with a pointer to the memory returned by > devres_alloc(). I was confused about that by the code before the > generalization that used a struct clk **ptr. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- Applied to clk-next
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index c822f4ef1584..1bb086695051 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -11,7 +11,7 @@ struct devm_clk_state { static void devm_clk_release(struct device *dev, void *res) { - struct devm_clk_state *state = *(struct devm_clk_state **)res; + struct devm_clk_state *state = res; if (state->exit) state->exit(state->clk);
The release function is called with a pointer to the memory returned by devres_alloc(). I was confused about that by the code before the generalization that used a struct clk **ptr. Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote: > > - clk_put(*(struct clk **)res); > > + struct devm_clk_state *state = *(struct devm_clk_state **)res; > > This should be: > > struct devm_clk_state *state = res; > > otherwise it nukes badly during cleanup: > [...] How embarrassing. I understood how I confused that, but I wonder how that didn't pop up earlier. FTR: I didn't test that now, but assume you did. My focus now was to get out an applicable patch fast. Thanks for your report Uwe drivers/clk/clk-devres.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: abae8e57e49aa75f6db76aa866c775721523908f