Messages in this thread | | | Date | Mon, 19 Jun 2023 12:24:41 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH v2] clk: Fix memory leak in devm_clk_notifier_register() |
| |
On Mon, Jun 19, 2023 at 05:05:47PM +0800, Fei Shao wrote: > On Mon, Jun 19, 2023 at 4:48 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > On Mon, Jun 19, 2023 at 11:22:53AM +0800, Fei Shao wrote: > > > devm_clk_notifier_register() allocates a devres resource for clk > > > notifier but didn't register that to the device, so the notifier didn't > > > get unregistered on device detach and the allocated resource was leaked. > > > > > > Fix the issue by registering the resource through devres_add(). > > > > > > Fixes: 6d30d50d037d ("clk: add devm variant of clk_notifier_register") > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > --- > > > > > > > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> > > > > How did you find this bug? > > > > I can think of some ways to find this bug with static analysis. > > > > It was actually detected by kmemleak on an unreleased Chromebook device. > I added the trace snippet in the message at first but removed that > before sending this. Maybe I shouldn't have. > > I can resend a v3 to add that back if that's preferable. What do you think?
I'm not a clk maintainer, but let's not go overboard resending patches, especially when they're as straight forward as this one.
This is good information though so I would include that kind of stuff in future patches. I don't really need to see the kmemleak warning itself because I know what those look like already. But to me it says a lot that actually this was detected at runtime. It says good things about your test infrastructure and makes me feel more confident that your patch is correct. So maybe just a comment that "This leak was detected by kmemleak".
regards, dan carpenter
| |