drivers/media/platform/qcom/venus/venc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
venc_probe() allocates a video_device with video_device_alloc() and
releases it from the err_vdev_release error path if
video_register_device() fails.
This can double free the video_device when __video_register_device()
reaches device_register() and that call fails:
video_register_device()
-> __video_register_device()
-> device_register() fails
-> put_device(&vdev->dev)
-> v4l2_device_release()
-> vdev->release(vdev)
-> video_device_release(vdev)
venc_probe()
-> err_vdev_release
-> video_device_release(vdev)
Use video_device_release_empty() while registering the device so that
registration failure paths do not free vdev through vdev->release().
venc_probe() then releases vdev exactly once from err_vdev_release.
Restore video_device_release() after successful registration so the
registered device keeps its normal lifetime handling.
This issue was found by a static analysis tool I am developing.
Fixes: aaaa93eda64b ("[media] media: venus: venc: add video encoder files")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
drivers/media/platform/qcom/venus/venc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index bf53267cb68d..9a5a025607fb 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1579,7 +1579,7 @@ static int venc_probe(struct platform_device *pdev)
return -ENOMEM;
strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name));
- vdev->release = video_device_release;
+ vdev->release = video_device_release_empty;
vdev->fops = &venc_fops;
vdev->ioctl_ops = &venc_ioctl_ops;
vdev->vfl_dir = VFL_DIR_M2M;
@@ -1590,6 +1590,7 @@ static int venc_probe(struct platform_device *pdev)
if (ret)
goto err_vdev_release;
+ vdev->release = video_device_release;
core->vdev_enc = vdev;
core->dev_enc = dev;
--
2.43.0On 19/05/2026 10:08, Guangshuo Li wrote:
> venc_probe() allocates a video_device with video_device_alloc() and
> releases it from the err_vdev_release error path if
> video_register_device() fails.
>
> This can double free the video_device when __video_register_device()
> reaches device_register() and that call fails:
>
> video_register_device()
> -> __video_register_device()
> -> device_register() fails
> -> put_device(&vdev->dev)
> -> v4l2_device_release()
> -> vdev->release(vdev)
> -> video_device_release(vdev)
>
> venc_probe()
> -> err_vdev_release
> -> video_device_release(vdev)
>
> Use video_device_release_empty() while registering the device so that
> registration failure paths do not free vdev through vdev->release().
> venc_probe() then releases vdev exactly once from err_vdev_release.
> Restore video_device_release() after successful registration so the
> registered device keeps its normal lifetime handling.
>
> This issue was found by a static analysis tool I am developing.
>
> Fixes: aaaa93eda64b ("[media] media: venus: venc: add video encoder files")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> drivers/media/platform/qcom/venus/venc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index bf53267cb68d..9a5a025607fb 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1579,7 +1579,7 @@ static int venc_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name));
> - vdev->release = video_device_release;
> + vdev->release = video_device_release_empty;
> vdev->fops = &venc_fops;
> vdev->ioctl_ops = &venc_ioctl_ops;
> vdev->vfl_dir = VFL_DIR_M2M;
> @@ -1590,6 +1590,7 @@ static int venc_probe(struct platform_device *pdev)
> if (ret)
> goto err_vdev_release;
>
> + vdev->release = video_device_release;
> core->vdev_enc = vdev;
> core->dev_enc = dev;
>
> --
> 2.43.0
>
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
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
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
Hi Bryan, On Tue, 19 May 2026 at 21:20, Bryan O'Donoghue <bod@kernel.org> 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?
On 19/05/2026 15:58, Guangshuo Li wrote:
> Hi Bryan,
>
> On Tue, 19 May 2026 at 21:20, Bryan O'Donoghue <bod@kernel.org> 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.
2. If 1 is true then fix video_register_device() to not call
video_device_release()
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
On Wed, 20 May 2026 at 00:34, Bryan O'Donoghue <bod@kernel.org> wrote: > > On 19/05/2026 15:58, Guangshuo Li wrote: > > Hi Bryan, > > > > On Tue, 19 May 2026 at 21:20, Bryan O'Donoghue <bod@kernel.org> 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. > > 2. If 1 is true then fix video_register_device() to not call > video_device_release() > > 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. Thanks, Guangshuo
On 20/05/2026 04:31, Guangshuo Li wrote: > On Wed, 20 May 2026 at 00:34, Bryan O'Donoghue <bod@kernel.org> wrote: >> >> On 19/05/2026 15:58, Guangshuo Li wrote: >>> Hi Bryan, >>> >>> On Tue, 19 May 2026 at 21:20, Bryan O'Donoghue <bod@kernel.org> 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 >
Hi Hans, On Wed, 20 May 2026 at 14:10, Hans Verkuil <hverkuil+cisco@kernel.org> wrote: > > > In patchwork I'm rejecting all your patches that change drivers to 'fix' this. > Thanks for the clarification, and thanks for rejecting those driver-specific patches in patchwork. > 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. > I agree that this should be fixed properly in v4l2-dev.c instead. I will look into the __video_register_device() error path and try to prepare a proper core fix. Thanks, Guangshuo
© 2016 - 2026 Red Hat, Inc.