Re: [PATCH] media: v4l2-dev: do not fire driver's release on __video_register_device() failure

[Laurent Pinchart]

On Wed, May 20, 2026 at 05:06:24PM +0800, Guangshuo Li wrote:
> video_register_device() / __video_register_device() registers vdev->dev
> with device_register(). Before the call the video core sets
> 
> 	vdev->dev.release = v4l2_device_release;
> 
> v4l2_device_release() invokes vdev->release(vdev) as its last step, and
> the driver's vdev->release hook is commonly video_device_release(), which
> kfree()s the vdev that the driver allocated with video_device_alloc().
> 
> When device_register() fails inside __video_register_device() the core
> does
> 
> 	put_device(&vdev->dev);
> 	return ret;
> 
> which drops the only reference and fires the v4l2_device_release()
> chain:
> 
>   __video_register_device()
>     device_register() -> -E*
>     put_device(&vdev->dev)
>       -> v4l2_device_release()
>          -> vdev->release(vdev)
>             -> video_device_release(vdev)   /* kfree(vdev), free #1 */
> 
> video_register_device() returns the error to the driver. Drivers that
> follow the documented ownership contract release vdev on their own error
> path, e.g.
> 
>   driver_probe()
>     if (video_register_device(vdev, ...))
>       goto err_release_vdev;
>     ...
>   err_release_vdev:
>     video_device_release(vdev);   /* free #2 -- DOUBLE FREE */
> 
> This is the contract documented in
> Documentation/driver-api/media/v4l2-dev.rst: the driver owns vdev and
> is responsible for releasing it if video_register_device() fails. As
> Hans Verkuil pointed out, the right place to fix this is the v4l2 core
> rather than every individual driver, because drivers are expected to
> follow the documented ownership contract.
> 
> Neutralise vdev->release around put_device() in the device_register()
> failure path so the device core cleanup does not run the driver's
> release hook. The driver-supplied release is restored before returning
> so the caller can release vdev according to the documented contract.
> Successful registration is unchanged, so the normal teardown sequence
> continues to call the driver's release hook and free vdev exactly once on
> unregister.
> 
> Fixes: 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()")
> Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 6ce623a1245a..73648549eb2a 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1075,9 +1075,14 @@ int __video_register_device(struct video_device *vdev,
>  	mutex_lock(&videodev_lock);
>  	ret = device_register(&vdev->dev);
>  	if (ret < 0) {
> +		void (*release)(struct video_device *) = vdev->release;
> +
>  		mutex_unlock(&videodev_lock);
>  		pr_err("%s: device_register failed\n", __func__);
> +
> +		vdev->release = video_device_release_empty;
>  		put_device(&vdev->dev);
> +		vdev->release = release;

That looks like a big hack. There must be something wrong somewhere else
in the design.

>  		return ret;
>  	}
>  

-- 
Regards,

Laurent Pinchart