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

[Bryan O'Donoghue <bod@kernel.org>]

On 19/05/2026 13:51, Guangshuo Li wrote:
> Hi Bryan,
> 
> Thank you for the review and for pointing me to the mxc-jpeg cleanup
> path.
> 
> On Tue, 19 May 2026 at 18:09, Bryan O'Donoghue <bod@kernel.org> wrote:
>>
>> OK so this will get the same feedback as the Iris version which is
>> please fix the cleanup path.
>>
>> If we look at drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c we can see
>>
>>           ret = video_register_device(jpeg->dec_vdev, VFL_TYPE_VIDEO, -1);
>>           if (ret) {
>>                   dev_err(dev, "failed to register video device\n");
>>                   goto err_vdev_register;
>>           }
>> <snip>
>>
>> err_vdev_register:
>>           /* Only release if allocation succeeded but registration failed */
>>           if (jpeg->dec_vdev)
>>                   video_device_release(jpeg->dec_vdev);
>>
>> So for Venus and Iris
>>
>> err_vdev_release:
>>          if(vdev)
>>                  video_device_release(vdev);
>>
>> i.e. only release the video device on the error path if the vdev pointer
>> is non-NULL.
>>
>> ---
>> bod
> 
> I agree that the mxc-jpeg pattern is useful for avoiding a double
> release when a video_device has been registered successfully and a later
> probe step fails. In that case the cleanup path does:
> 
>          video_unregister_device(jpeg->dec_vdev);
>          jpeg->dec_vdev = NULL;
> 
> and then falls through to:
> 
>          if (jpeg->dec_vdev)
>                  video_device_release(jpeg->dec_vdev);
> 
> So the NULL assignment prevents the already unregistered video_device
> from being released again by the later shared error label. That makes
> sense for the "registration succeeded, later cleanup failed" case.
> 
> However, the issue I am trying to fix in Venus happens before
> video_register_device() returns, when video_register_device() itself
> fails after reaching device_register().
> 
> The problematic path is:
> 
>          venc_probe()
>            -> video_register_device(vdev, VFL_TYPE_VIDEO, -1)
>               -> __video_register_device()
>                  -> device_register(&vdev->dev) fails
>                     -> put_device(&vdev->dev)
>                        -> v4l2_device_release()
>                           -> vdev->release(vdev)
>                              -> video_device_release(vdev)
> 
> At this point, video_register_device() returns an error to venc_probe().
> Then the current Venus error path continues with:
> 
>          venc_probe()
>            -> err_vdev_release
>               -> video_device_release(vdev)
> 
> So the same video_device can be released twice.
> 
> In this path, adding only:
> 
>          if (vdev)
>                  video_device_release(vdev);
> 
> would not avoid the double free, because vdev is a local pointer in
> venc_probe(). It is still non-NULL even if the object it points to has
> already been released through put_device(&vdev->dev) inside
> __video_register_device().
> 
> So I think the mxc-jpeg cleanup pattern handles a different ownership
> transition: it avoids releasing a video_device after a successful
> registration has later been undone with video_unregister_device(). The
> Venus issue here is about the ownership state when video_register_device()
> itself fails after device_register() has already taken and dropped the
> device reference.
> 
> This is why the patch temporarily uses video_device_release_empty() while
> calling video_register_device(). With that, if device_register() fails
> and the V4L2 core reaches vdev->release(vdev), it will not free vdev.
> Then the Venus err_vdev_release path can still release vdev exactly once:
> 
>          venc_probe()
>            -> video_register_device()
>               -> __video_register_device()
>                  -> device_register() fails
>                     -> put_device(&vdev->dev)
>                        -> v4l2_device_release()
>                           -> vdev->release(vdev)
>                              -> video_device_release_empty(vdev)
> 
>          venc_probe()
>            -> err_vdev_release
>               -> video_device_release(vdev)
> 
> After video_register_device() succeeds, the patch restores:
> 
>          vdev->release = video_device_release;
> 
> so the successfully registered device keeps the normal lifetime handling.
> 
> Please let me know if you think this should be solved differently, for
> example in the V4L2 core instead of in the Venus driver. I would be happy
> to rework the patch if there is a preferred approach.
> 
> Thanks,
> Guangshuo

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