Re: [PATCH] media: venus: venc: avoid double free on video register failure

[Hans Verkuil]

On 20/05/2026 04:31, Guangshuo Li wrote:
> On Wed, 20 May 2026 at 00:34, Bryan O'Donoghue <bod@xxxxxxxxxx> wrote:
>>
>> On 19/05/2026 15:58, Guangshuo Li wrote:
>>> Hi Bryan,
>>>
>>> On Tue, 19 May 2026 at 21:20, Bryan O'Donoghue <bod@xxxxxxxxxx> wrote:
>>>>
>>>> Yes I take your point.
>>>>
>>>> So what you are describing is an error in the software contract from
>>>> video_register_device() - if we look throughout the usage of that
>>>> function we see either the pattern we already have - not checking for
>>>> NULL or checking for NULL - not the double free case you are addressing.
>>>>
>>>> So really the fix - the place to litigate this is not in Venus or Iris
>>>> but in video_register_device's cleanup path.
>>>>
>>>> ---
>>>> bod
>>>
>>> Thanks, I agree.
>>>
>>> This should probably be handled in the video_register_device() failure
>>> path rather than in each individual driver.
>>>
>>> I do not have a good idea yet for how to fix that cleanly in the v4l2
>>> core. Do you have any suggestion?
>>
>> So if we look at how video_register_device() is used by drivers we have
>> two different behaviours.
>>
>> 1. Trap the error and release the device
>> 2. Trip the error - check for NULL and release the device
>>
>> Either way the _users_ of video_register_device() right now expect to
>> have to call video_device_release().
>>
>> So... it seems to me video_register_device() also calling
>> video_release() on some but not all of its error path is not the
>> expected software contract.
>>
>> So I suggest two things.
>>
>> 1. Audit all users of video_register_device() and confirm the hypothesis
>>     That is callers expect to own vdev and currently everybody tries
>>     to clean it up.

That's what drivers expect, since that's also how it is documented in
Documentation/driver-api/media/v4l2-dev.rst

This is old code, it's been like that for ages. There may well be drivers
that do not do this, but then it is a driver bug.

>> 2. If 1 is true then fix video_register_device() to not call
>>     video_device_release()

That's the right approach.

>>
>> It either needs to be that or fully delegate ownership of vdev to
>> video_device_register() _and_ update all of the callers.
>>
>> It may be that < 100% of callers if that is low single digits then
>> worthwhile updating those drivers to match the new semantic.
>>
>> €0.02
>>
>> ---
>> bod
> 
> Thanks, I agree with your suggestion.
> 
> I initially considered that some callers might not follow this ownership
> semantic, so I tried to fix the issues reported by my static analysis tool
> driver by driver first.
> 
> I will audit the users of video_register_device() to check the common
> caller expectation, and then look into fixing the core error path if that
> is the right direction.

In patchwork I'm rejecting all your patches that change drivers to 'fix' this.

I'm looking forward to a patch fixing it properly in v4l2-dev.c.

It's a real issue, but this shouldn't be done in drivers.

Regards,

	Hans

> 
> Thanks,
> Guangshuo
>