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

[Laurent Pinchart]

Hi Hans,

On Wed, May 20, 2026 at 12:01:46PM +0200, Hans Verkuil wrote:
> On 20/05/2026 11:34, Laurent Pinchart wrote:
> > 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.
> 
> There is, unfortunately the design was wrong since the beginning of V4L2.
> 
> Documentation/driver-api/media/v4l2-dev.rst explicitly says that you should use:
> 
>         err = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>         if (err) {
>                 video_device_release(vdev); /* or kfree(my_vdev); */
>                 return err;
>         }
> 
> So everyone does that. Luckily device_register never fails in practice (you probably have
> bigger problems if it fails then just a double-free).
> 
> The reality is that we don't handle this failure well at all, and this change wouldn't
> help in all cases either. E.g. drivers/media/platform/renesas/renesas-ceu.c actually relies
> on the release() callback in that it doesn't call video_device_release().
> 
> But then it would fail on the v4l2_err(vdev->v4l2_dev, ...) call since vdev would be freed
> already.
> 
> There are probably more drivers like that. (drivers/media/i2c/video-i2c.c)
> 
> I'm not sure what is wisdom here.
> 
> See also commit 2a934fdb01db ("media: v4l2-dev: fix error handling in __video_register_device()"),
> which is where the put_device was introduced in the first place.
> 
> Perhaps that should be reverted instead?

If we want a short term fix I think that would be better.

Have you seen
https://lore.kernel.org/all/aeCOdWLaVpH-5w8s@xxxxxxxxxxxxxxxxxxxx/ ?

Having an API contract different from device_register() will likely
cause issues one way or another.

> >>  		return ret;
> >>  	}
> >>  

-- 
Regards,

Laurent Pinchart