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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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?

Regards,

	Hans

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