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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
> On 20/05/2026 12:48, Laurent Pinchart wrote:
> > 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@gmail.com>
> >>>> ---
> >>>>  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@hovoldconsulting.com/ ?
> 
> I hadn't seen it. Interesting.
> 
> > 
> > Having an API contract different from device_register() will likely
> > cause issues one way or another.
> 
> Looking closely how the driver core works and what commit 2a934fdb01db changed,
> I think this might fix it:
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 5516b2bbb08f..6ffd385e880f 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1071,25 +1071,27 @@ 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);
> 
> This mostly reverts 2a934fdb01db, except that we keep the put_device.
> But when this is called, vdev->dev.release isn't set yet, so it only
> frees the device-related data (device_release in drivers/base/core.c).
> 
> Am I missing something?

I guess this could be workable. This way the caller knows the release
callback won't be called.

Regarding error handling, the return value from
video_register_media_controller() is ignored. It'd probably be good to fix
that in a separate patch though. I could submit one as well.

-- 
Regards,

Sakari Ailus