[PATCH 0/2] media: Revert 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")

[PATCH 0/2] media: Revert 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")

[Hans Verkuil]

That commit exchanged a memory leak with a double-free scenario,
and it is not something that is easily fixable. Revert it, since it
is still better to leak memory than get into a double-free.

See this email discussion for more info:

https://lore.kernel.org/linux-media/20260520090624.1071139-1-lgs201920130244@gmail.com/

I tried to come up with a reliable method to handle this cleanly in the
v4l2 core, but the only way to do this properly would be to split
video_register_device into two parts: an init and a registration part.
And then convert all drivers to use the new functions.

That's a very big job, and so for now just revert the patch and add comments
explaining why it is reverted.

Regards,

	Hans

Hans Verkuil (2):
  Revert "media: v4l2-dev: fix error handling in
    __video_register_device()"
  media: v4l2-core: v4l2-dev: add comments on device_register fail.

 drivers/media/v4l2-core/v4l2-dev.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH 1/2] Revert "media: v4l2-dev: fix error handling in __video_register_device()"

[Hans Verkuil]

This reverts commit 2a934fdb01db6458288fc9386d3d8ceba6dd551a.

The intentions of that patch were good, but it doesn't work.

The idea is that if device_register fails, you have to do a put_device
to let the ref counter release resources.

However, the V4L2 API says that if video_register_device() fails, then
you have to call video_device_release(), which kfree()s the video_device
struct.

But the put_device() will already have freed the struct, so you end
up in a double-free scenario.

There is not really a good way of fixing this without breaking
video_register_device() into two parts, one that initializes everything,
and one that does the actual device_register, and then converting all
V4L2 drivers to this new model.

That is a massive job, and it is very unlikely that device_register
will fail.

So rather than ending up in a double-free scenario, just revert this
patch, and in that case we'll have a small memory leak. Which is a lot
more robust.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/linux-media/20260520090624.1071139-1-lgs201920130244@gmail.com/
Link: https://lore.kernel.org/all/2026042058-charm-storable-4ad8@gregkh/
---
 drivers/media/v4l2-core/v4l2-dev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 5516b2bbb08f..d750bf10febe 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1071,25 +1071,25 @@ int __video_register_device(struct video_device *vdev,
 	vdev->dev.class = &video_class;
 	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
 	vdev->dev.parent = vdev->dev_parent;
-	vdev->dev.release = v4l2_device_release;
 	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
-
-	/* Increase v4l2_device refcount */
-	v4l2_device_get(vdev->v4l2_dev);
-
 	mutex_lock(&videodev_lock);
 	ret = device_register(&vdev->dev);
 	if (ret < 0) {
 		mutex_unlock(&videodev_lock);
 		pr_err("%s: device_register failed\n", __func__);
-		put_device(&vdev->dev);
-		return ret;
+		goto cleanup;
 	}
+	/* Register the release callback that will be called when the last
+	   reference to the device goes away. */
+	vdev->dev.release = v4l2_device_release;
 
 	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
 		pr_warn("%s: requested %s%d, got %s\n", __func__,
 			name_base, nr, video_device_node_name(vdev));
 
+	/* Increase v4l2_device refcount */
+	v4l2_device_get(vdev->v4l2_dev);
+
 	/* Part 5: Register the entity. */
 	ret = video_register_media_controller(vdev);
 
-- 
2.53.0

[PATCH 2/2] media: v4l2-core: v4l2-dev: add comments on device_register fail.

[Hans Verkuil]

If device_register fails, then we are supposed to call put_device.
Explain why we do not do that.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d750bf10febe..fd267fb74905 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1075,6 +1075,20 @@ int __video_register_device(struct video_device *vdev,
 	mutex_lock(&videodev_lock);
 	ret = device_register(&vdev->dev);
 	if (ret < 0) {
+		/*
+		 * We should do a put_device() here, but the problem is that
+		 * the V4L2 API expects drivers to call video_device_release()
+		 * on error, and so both put_device() and video_device_release
+		 * would kfree vdev.
+		 *
+		 * The proper solution would be to split this function into
+		 * two parts: initialization and registration, and then rework
+		 * all drivers.
+		 *
+		 * Until then just skip the put_device and free everything.
+		 * This will result in a small memory leak, which is better
+		 * than a double-free.
+		 */
 		mutex_unlock(&videodev_lock);
 		pr_err("%s: device_register failed\n", __func__);
 		goto cleanup;
-- 
2.53.0