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

[Laurent Pinchart]

On Wed, May 20, 2026 at 03:02:19PM +0300, Sakari Ailus wrote:
> On Wed, May 20, 2026 at 01:26:30PM +0200, Hans Verkuil wrote:
> > On 20/05/2026 12:48, Laurent Pinchart wrote:
> > > 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/ ?
> > 
> > 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.

I think it's a short term hack at best though :-( If the device core
requires reference-counting for struct device, with a requirement to
call put_device() instead of just freeing the structure, then anything
that embeds a struct device should do the same. That means exposing
video_put() (which should probably be renamed to video_device_put()) and
calling that in the error path, in the caller.

We may also want to split video_register_device() into
video_device_init() and video_device_add(), but that's a separate
question.

> 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,

Laurent Pinchart