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

[Bryan O'Donoghue]

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@xxxxxxxxxx> 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