media: davinci: vpif_capture: fix memory leak in vpif_probe()

PATCH In review Young Xiao 2 participants 4 messages Jun 4, 2019 Media (V4L/DVB)

Share & tools

Young Xiao

author

2019-06-04 07:49 UTC #0 lore raw

PATCH 1/1 media: davinci: vpif_capture: fix memory leak in vpif_probe()

If vpif_probe() fails on v4l2_device_register() and vpif_probe_complete(), then memory allocated at initialize_vpif() for global vpif_obj.dev[i] become unreleased.

The patch adds deallocation of vpif_obj.dev[i] on the error path.

Signed-off-by Young Xiao <[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +16 -3

index b5aacb0..277d500 100644
@@ -1385,6 +1385,14 @@ static int initialize_vpif(void)
 	return err;
 }
 
+static void free_vpif_objs(void)
+{
+	int i;
+
+	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)
+		kfree(vpif_obj.dev[i]);
+}
+
 static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 			    struct v4l2_subdev *subdev,
 			    struct v4l2_async_subdev *asd)

@@ -1654,7 +1662,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 	err = v4l2_device_register(vpif_dev, &vpif_obj.v4l2_dev);
 	if (err) {
 		v4l2_err(vpif_dev->driver, "Error registering v4l2 device\n");
-		goto cleanup;
+		goto vpif_free;
 	}
 
 	while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {

@@ -1701,7 +1709,10 @@ static __init int vpif_probe(struct platform_device *pdev)
 				  "registered sub device %s\n",
 				   subdevdata->name);
 		}
-		vpif_probe_complete();
+		err = vpif_probe_complete();
+		if (err) {
+			goto probe_subdev_out;
+		}
 	} else {
 		vpif_obj.notifier.ops = &vpif_async_ops;
 		err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,

@@ -1720,6 +1731,8 @@ static __init int vpif_probe(struct platform_device *pdev)
 	kfree(vpif_obj.sd);
 vpif_unregister:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
+vpif_free:
+	free_vpif_objs();
 cleanup:
 	v4l2_async_notifier_cleanup(&vpif_obj.notifier);

@@ -1748,8 +1761,8 @@ static int vpif_remove(struct platform_device *device)
 		ch = vpif_obj.dev[i];
 		/* Unregister video device */
 		video_unregister_device(&ch->video_dev);
-		kfree(vpif_obj.dev[i]);
 	}
+	free_vpif_objs();
 	return 0;
 }

Signature · 1 line

2.7.4

Lad, Prabhakar

2019-06-04 08:15 UTC #1 lore raw

Hi Young,

Thanks for the patch.

Quoted 17 lines · Young Xiao

If vpif_probe() fails on v4l2_device_register() and vpif_probe_complete(), then memory allocated at initialize_vpif() for global vpif_obj.dev[i] become unreleased.
The patch adds deallocation of vpif_obj.dev[i] on the error path.
Signed-off-by Young Xiao <[email protected]>
---
 drivers/media/platform/davinci/vpif_capture.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
drivers/media/platform/davinci/vpif_capture.c +0 -0
index b5aacb0..277d500 100644

drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1389

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -1385,6 +1385,14 @@ static int initialize_vpif(void)
        return err;
 }
 
+static void free_vpif_objs(void)
+{

function could be made inline.

drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1392

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +3 -0

+       int i;
+
+       for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)

VPIF_DISPLAY_MAX_DEVICES ? this should be VPIF_CAPTURE_MAX_DEVICES

Quoted 16 lines

drivers/media/platform/davinci/vpif_capture.c +4 -1

+               kfree(vpif_obj.dev[i]);
+}
+
 static int vpif_async_bound(struct v4l2_async_notifier *notifier,
                            struct v4l2_subdev *subdev,
                            struct v4l2_async_subdev *asd)

@@ -1654,7 +1662,7 @@ static __init int vpif_probe(struct platform_device *pdev)
        err = v4l2_device_register(vpif_dev, &vpif_obj.v4l2_dev);
        if (err) {
                v4l2_err(vpif_dev->driver, "Error registering v4l2 device\n");
-               goto cleanup;
+               goto vpif_free;
        }
 
        while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {

drivers/media/platform/davinci/vpif_capture.c static __init int vpif_probe(struct platform_device *pdev) · line 1715

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +4 -1

@@ -1701,7 +1709,10 @@ static __init int vpif_probe(struct platform_device *pdev)
                                  "registered sub device %s\n",
                                   subdevdata->name);
                }
-               vpif_probe_complete();
+               err = vpif_probe_complete();
+               if (err) {
+                       goto probe_subdev_out;
+               }

No need for { and } as per kernel coding style.

Quoted 13 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

        } else {
                vpif_obj.notifier.ops = &vpif_async_ops;
                err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,

@@ -1720,6 +1731,8 @@ static __init int vpif_probe(struct platform_device *pdev)
        kfree(vpif_obj.sd);
 vpif_unregister:
        v4l2_device_unregister(&vpif_obj.v4l2_dev);
+vpif_free:
+       free_vpif_objs();
 cleanup:
        v4l2_async_notifier_cleanup(&vpif_obj.notifier);

drivers/media/platform/davinci/vpif_capture.c static int vpif_remove(struct platform_device *device) · line 1765

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +1 -1

@@ -1748,8 +1761,8 @@ static int vpif_remove(struct platform_device *device)
                ch = vpif_obj.dev[i];
                /* Unregister video device */
                video_unregister_device(&ch->video_dev);
-               kfree(vpif_obj.dev[i]);
        }
+       free_vpif_objs();

no need for this change, leave it as it is.

Cheers,
Prabhakar Lad

Yang Xiao

author

2019-06-04 08:34 UTC #2 lore raw

Quoted 50 lines · Prabhakar

Hi Young,
Thanks for the patch.
Quoted 17 lines · Young Xiao
If vpif_probe() fails on v4l2_device_register() and vpif_probe_complete(), then memory allocated at initialize_vpif() for global vpif_obj.dev[i] become unreleased.
The patch adds deallocation of vpif_obj.dev[i] on the error path.
Signed-off-by Young Xiao <[email protected]>
---
 drivers/media/platform/davinci/vpif_capture.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
drivers/media/platform/davinci/vpif_capture.c +0 -0
index b5aacb0..277d500 100644
drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1389
Quoted 5 lines
drivers/media/platform/davinci/vpif_capture.c +2 -0
@@ -1385,6 +1385,14 @@ static int initialize_vpif(void)
        return err;
 }
 
+static void free_vpif_objs(void)
+{
function could be made inline.
drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1392
Quoted 3 lines
drivers/media/platform/davinci/vpif_capture.c +3 -0
+       int i;
+
+       for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)
VPIF_DISPLAY_MAX_DEVICES ? this should be VPIF_CAPTURE_MAX_DEVICES
Quoted 14 lines
drivers/media/platform/davinci/vpif_capture.c +4 -1
+               kfree(vpif_obj.dev[i]);
+}
+
 static int vpif_async_bound(struct v4l2_async_notifier *notifier,
                            struct v4l2_subdev *subdev,
                            struct v4l2_async_subdev *asd)
@@ -1654,7 +1662,7 @@ static __init int vpif_probe(struct platform_device *pdev)
        err = v4l2_device_register(vpif_dev, &vpif_obj.v4l2_dev);
        if (err) {
                v4l2_err(vpif_dev->driver, "Error registering v4l2 device\n");
-               goto cleanup;
+               goto vpif_free;
        }
 

Quoted 12 lines

drivers/media/platform/davinci/vpif_capture.c static __init int vpif_probe(struct platform_device *pdev) · line 1715

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +4 -1

        while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {

@@ -1701,7 +1709,10 @@ static __init int vpif_probe(struct platform_device *pdev)
                                  "registered sub device %s\n",
                                   subdevdata->name);
                }
-               vpif_probe_complete();
+               err = vpif_probe_complete();
+               if (err) {
+                       goto probe_subdev_out;
+               }

No need for { and } as per kernel coding style.

Sorry, I can not get your point here.
There is no need to check the return value of vpif_probe_complete(), isn't it? So, we just fix the memory leak in the error path of v4l2_device_register()?

Quoted 25 lines

Quoted 14 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

        } else {
                vpif_obj.notifier.ops = &vpif_async_ops;
                err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,

@@ -1720,6 +1731,8 @@ static __init int vpif_probe(struct platform_device *pdev)
        kfree(vpif_obj.sd);
 vpif_unregister:
        v4l2_device_unregister(&vpif_obj.v4l2_dev);
+vpif_free:
+       free_vpif_objs();
 cleanup:
        v4l2_async_notifier_cleanup(&vpif_obj.notifier);

drivers/media/platform/davinci/vpif_capture.c static int vpif_remove(struct platform_device *device) · line 1765

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +1 -1

@@ -1748,8 +1761,8 @@ static int vpif_remove(struct platform_device *device)
                ch = vpif_obj.dev[i];
                /* Unregister video device */
                video_unregister_device(&ch->video_dev);
-               kfree(vpif_obj.dev[i]);
        }
+       free_vpif_objs();

no need for this change, leave it as it is.

Cheers,
Prabhakar Lad

Signature · 4 lines

Best regards!

Young
-----------------------------------------------------------

Lad, Prabhakar

2019-06-04 08:48 UTC #3 lore raw

Cheers,
--Prabhakar Lad

Quoted 66 lines · Yang Xiao

Quoted 50 lines · Prabhakar
Hi Young,
Thanks for the patch.
Quoted 17 lines · Young Xiao
If vpif_probe() fails on v4l2_device_register() and vpif_probe_complete(), then memory allocated at initialize_vpif() for global vpif_obj.dev[i] become unreleased.
The patch adds deallocation of vpif_obj.dev[i] on the error path.
Signed-off-by Young Xiao <[email protected]>
---
 drivers/media/platform/davinci/vpif_capture.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
drivers/media/platform/davinci/vpif_capture.c +0 -0
index b5aacb0..277d500 100644
drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1389
Quoted 5 lines
drivers/media/platform/davinci/vpif_capture.c +2 -0
@@ -1385,6 +1385,14 @@ static int initialize_vpif(void)
        return err;
 }
 
+static void free_vpif_objs(void)
+{
function could be made inline.
drivers/media/platform/davinci/vpif_capture.c static int initialize_vpif(void) · line 1392
Quoted 3 lines
drivers/media/platform/davinci/vpif_capture.c +3 -0
+       int i;
+
+       for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)
VPIF_DISPLAY_MAX_DEVICES ? this should be VPIF_CAPTURE_MAX_DEVICES
Quoted 14 lines
drivers/media/platform/davinci/vpif_capture.c +4 -1
+               kfree(vpif_obj.dev[i]);
+}
+
 static int vpif_async_bound(struct v4l2_async_notifier *notifier,
                            struct v4l2_subdev *subdev,
                            struct v4l2_async_subdev *asd)
@@ -1654,7 +1662,7 @@ static __init int vpif_probe(struct platform_device *pdev)
        err = v4l2_device_register(vpif_dev, &vpif_obj.v4l2_dev);
        if (err) {
                v4l2_err(vpif_dev->driver, "Error registering v4l2 device\n");
-               goto cleanup;
+               goto vpif_free;
        }
 
Quoted 12 lines
drivers/media/platform/davinci/vpif_capture.c static __init int vpif_probe(struct platform_device *pdev) · line 1715
Quoted 10 lines
drivers/media/platform/davinci/vpif_capture.c +4 -1
        while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
@@ -1701,7 +1709,10 @@ static __init int vpif_probe(struct platform_device *pdev)
                                  "registered sub device %s\n",
                                   subdevdata->name);
                }
-               vpif_probe_complete();
+               err = vpif_probe_complete();
+               if (err) {
+                       goto probe_subdev_out;
+               }
No need for { and } as per kernel coding style.

Quoted 4 lines

Sorry, I can not get your point here.
There is no need to check the return value of vpif_probe_complete(), isn't it? So, we just fix the memory leak in the error path of v4l2_device_register()?

Not sure if you got my statement here, it means do not add {} braces around the if statement,
so for example the code should look like something below (refer the link [1] for more reading)

err = vpif_probe_complete();
if (err)

    goto probe_subdev_out;

[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces

Cheers,
--Prabhakar Lad