| Message ID | 20221208075938.13764-2-shangxiaojing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: Fix some memleaks | expand |
Hi On Thu, 8 Dec 2022 at 08:01, Shang XiaoJing <shangxiaojing@huawei.com> wrote: > > There is a kmemleak when testing the media/i2c/ov2740.c with bpf mock > device: > > unreferenced object 0xffff8881090e19e0 (size 16): > comm "51-i2c-ov2740", pid 278, jiffies 4294781584 (age 23.613s) > hex dump (first 16 bytes): > 00 f3 7c 0b 81 88 ff ff 80 75 6a 09 81 88 ff ff ..|......uj..... > backtrace: > [<000000004e9fad8f>] __kmalloc_node+0x44/0x1b0 > [<0000000039c802f4>] kvmalloc_node+0x34/0x180 > [<000000009b8b5c63>] v4l2_ctrl_handler_init_class+0x11d/0x180 > [videodev] > [<0000000038644056>] ov2740_probe+0x37d/0x84f [ov2740] > [<0000000092489f59>] i2c_device_probe+0x28d/0x680 > [<000000001038babe>] really_probe+0x17c/0x3f0 > [<0000000098c7af1c>] __driver_probe_device+0xe3/0x170 > [<00000000e1b3dc24>] device_driver_attach+0x34/0x80 > [<000000005a04a34d>] bind_store+0x10b/0x1a0 > [<00000000ce25d4f2>] drv_attr_store+0x49/0x70 > [<000000007d9f4e9a>] sysfs_kf_write+0x8c/0xb0 > [<00000000be6cff0f>] kernfs_fop_write_iter+0x216/0x2e0 > [<0000000031ddb40a>] vfs_write+0x658/0x810 > [<0000000041beecdd>] ksys_write+0xd6/0x1b0 > [<0000000023755840>] do_syscall_64+0x38/0x90 > [<00000000b2cc2da2>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > ov2740_init_controls() won't clean all the allocated resources in fail > path, which may causes the memleaks. Add v4l2_ctrl_handler_free() to > prevent memleak. > > Fixes: 866edc895171 ("media: i2c: Add ov2740 image sensor driver") > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > --- > drivers/media/i2c/ov2740.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 5d74ad479214..628ab86698c0 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -630,8 +630,10 @@ static int ov2740_init_controls(struct ov2740 *ov2740) > V4L2_CID_TEST_PATTERN, > ARRAY_SIZE(ov2740_test_pattern_menu) - 1, > 0, 0, ov2740_test_pattern_menu); > - if (ctrl_hdlr->error) > + if (ctrl_hdlr->error) { > + v4l2_ctrl_handler_free(ctrl_hdlr); > return ctrl_hdlr->error; I know this has been merged, but I happened to be looking at ov2740 for other reasons and noted this patch. v4l2_ctrl_handler_free includes setting "hdl->error = 0;" [1], so as I read it, calling it here means that ov2740_init_controls will now be returning 0 rather than the error code. There is a v4l2_ctrl_handler_free call in ov2740_probe, but it's freeing ov2740->sd.ctrl_handler which is only set by ov2740_init_controls on success. Perhaps it's better to call it with &ov2740->ctrl_handler instead? The same issue applies to the other patch in this series for ov5675 [3]. Doing a very quick sweep through the tree, it looks like at least imx296, imx334, imx335, imx412, ov7251, and ov9282 all appear to have the same issue. As it is a useful pattern, is it better to NOT clear hdl->error in v4l2_ctrl_handler_free? It'll be set to 0 in any subsequent call to v4l2_ctrl_handler_init, so leaving it set has no real adverse effect. Sakari & Hans - your thoughts? Dave [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ctrls-core.c#L1330 [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov2740.c#L1196 [3] https://github.com/torvalds/linux/commit/dd74ed6c213003533e3abf4c204374ef01d86978 > + } > > ov2740->sd.ctrl_handler = ctrl_hdlr; > > -- > 2.17.1 >
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index 5d74ad479214..628ab86698c0 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -630,8 +630,10 @@ static int ov2740_init_controls(struct ov2740 *ov2740) V4L2_CID_TEST_PATTERN, ARRAY_SIZE(ov2740_test_pattern_menu) - 1, 0, 0, ov2740_test_pattern_menu); - if (ctrl_hdlr->error) + if (ctrl_hdlr->error) { + v4l2_ctrl_handler_free(ctrl_hdlr); return ctrl_hdlr->error; + } ov2740->sd.ctrl_handler = ctrl_hdlr;
There is a kmemleak when testing the media/i2c/ov2740.c with bpf mock device: unreferenced object 0xffff8881090e19e0 (size 16): comm "51-i2c-ov2740", pid 278, jiffies 4294781584 (age 23.613s) hex dump (first 16 bytes): 00 f3 7c 0b 81 88 ff ff 80 75 6a 09 81 88 ff ff ..|......uj..... backtrace: [<000000004e9fad8f>] __kmalloc_node+0x44/0x1b0 [<0000000039c802f4>] kvmalloc_node+0x34/0x180 [<000000009b8b5c63>] v4l2_ctrl_handler_init_class+0x11d/0x180 [videodev] [<0000000038644056>] ov2740_probe+0x37d/0x84f [ov2740] [<0000000092489f59>] i2c_device_probe+0x28d/0x680 [<000000001038babe>] really_probe+0x17c/0x3f0 [<0000000098c7af1c>] __driver_probe_device+0xe3/0x170 [<00000000e1b3dc24>] device_driver_attach+0x34/0x80 [<000000005a04a34d>] bind_store+0x10b/0x1a0 [<00000000ce25d4f2>] drv_attr_store+0x49/0x70 [<000000007d9f4e9a>] sysfs_kf_write+0x8c/0xb0 [<00000000be6cff0f>] kernfs_fop_write_iter+0x216/0x2e0 [<0000000031ddb40a>] vfs_write+0x658/0x810 [<0000000041beecdd>] ksys_write+0xd6/0x1b0 [<0000000023755840>] do_syscall_64+0x38/0x90 [<00000000b2cc2da2>] entry_SYSCALL_64_after_hwframe+0x63/0xcd ov2740_init_controls() won't clean all the allocated resources in fail path, which may causes the memleaks. Add v4l2_ctrl_handler_free() to prevent memleak. Fixes: 866edc895171 ("media: i2c: Add ov2740 image sensor driver") Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> --- drivers/media/i2c/ov2740.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)