1. Patchew
  2. linux
  3. View series

[PATCH] media: s5p-mfc: avoid double free on video register failure

Guangshuo Li posted 1 patch 6 days, 15 hours ago 
drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH] media: s5p-mfc: avoid double free on video register failure
Posted by Guangshuo Li 6 days, 15 hours ago 
s5p_mfc_probe() allocates video_device instances for both the decoder
and encoder and releases them from the probe error paths if
video_register_device() fails.

This can double free a 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)

  s5p_mfc_probe()
    -> err_dec_reg or err_enc_reg
       -> video_device_release(vdev)

Use video_device_release_empty() while registering the decoder and encoder
video devices so that registration failure paths do not free them through
vdev->release(). s5p_mfc_probe() then releases each video_device exactly
once from its error path. Restore video_device_release() after successful
registration so the registered devices keep their normal lifetime
handling.

This issue was found by a static analysis tool I am developing.

Fixes: d0ce898c39bf ("[media] s5p-mfc: Replaced commas with semicolons")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
index 32eb402d439c..75abb0a8b7a9 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
@@ -1376,7 +1376,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 	}
 	vfd->fops	= &s5p_mfc_fops;
 	vfd->ioctl_ops	= get_dec_v4l2_ioctl_ops();
-	vfd->release	= video_device_release;
+	vfd->release	= video_device_release_empty;
 	vfd->lock	= &dev->mfc_mutex;
 	vfd->v4l2_dev	= &dev->v4l2_dev;
 	vfd->vfl_dir	= VFL_DIR_M2M;
@@ -1395,7 +1395,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 	}
 	vfd->fops	= &s5p_mfc_fops;
 	vfd->ioctl_ops	= get_enc_v4l2_ioctl_ops();
-	vfd->release	= video_device_release;
+	vfd->release	= video_device_release_empty;
 	vfd->lock	= &dev->mfc_mutex;
 	vfd->v4l2_dev	= &dev->v4l2_dev;
 	vfd->vfl_dir	= VFL_DIR_M2M;
@@ -1416,6 +1416,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
 		goto err_dec_reg;
 	}
+
+	dev->vfd_dec->release = video_device_release;
 	v4l2_info(&dev->v4l2_dev,
 		  "decoder registered as /dev/video%d\n", dev->vfd_dec->num);
 
@@ -1424,6 +1426,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
 		goto err_enc_reg;
 	}
+
+	dev->vfd_enc->release = video_device_release;
 	v4l2_info(&dev->v4l2_dev,
 		  "encoder registered as /dev/video%d\n", dev->vfd_enc->num);
 
-- 
2.43.0
Re: [PATCH] media: s5p-mfc: avoid double free on video register failure
Posted by Marek Szyprowski 4 days, 16 hours ago 
On 18.05.2026 15:09, Guangshuo Li wrote:
> s5p_mfc_probe() allocates video_device instances for both the decoder
> and encoder and releases them from the probe error paths if
> video_register_device() fails.
>
> This can double free a 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)
>
>   s5p_mfc_probe()
>     -> err_dec_reg or err_enc_reg
>        -> video_device_release(vdev)
>
> Use video_device_release_empty() while registering the decoder and encoder
> video devices so that registration failure paths do not free them through
> vdev->release(). s5p_mfc_probe() then releases each video_device exactly
> once from its error path. Restore video_device_release() after successful
> registration so the registered devices keep their normal lifetime
> handling.
>
> This issue was found by a static analysis tool I am developing.
>
> Fixes: d0ce898c39bf ("[media] s5p-mfc: Replaced commas with semicolons")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
Frankly speaking I don't like this dancing with video_device_release_empty() and
video_device_release(). I would rather make video_device struct a part of device
state and use common release function.

> ---
>  drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> index 32eb402d439c..75abb0a8b7a9 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> @@ -1376,7 +1376,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  	}
>  	vfd->fops	= &s5p_mfc_fops;
>  	vfd->ioctl_ops	= get_dec_v4l2_ioctl_ops();
> -	vfd->release	= video_device_release;
> +	vfd->release	= video_device_release_empty;
>  	vfd->lock	= &dev->mfc_mutex;
>  	vfd->v4l2_dev	= &dev->v4l2_dev;
>  	vfd->vfl_dir	= VFL_DIR_M2M;
> @@ -1395,7 +1395,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  	}
>  	vfd->fops	= &s5p_mfc_fops;
>  	vfd->ioctl_ops	= get_enc_v4l2_ioctl_ops();
> -	vfd->release	= video_device_release;
> +	vfd->release	= video_device_release_empty;
>  	vfd->lock	= &dev->mfc_mutex;
>  	vfd->v4l2_dev	= &dev->v4l2_dev;
>  	vfd->vfl_dir	= VFL_DIR_M2M;
> @@ -1416,6 +1416,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
>  		goto err_dec_reg;
>  	}
> +
> +	dev->vfd_dec->release = video_device_release;
>  	v4l2_info(&dev->v4l2_dev,
>  		  "decoder registered as /dev/video%d\n", dev->vfd_dec->num);
>  
> @@ -1424,6 +1426,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
>  		goto err_enc_reg;
>  	}
> +
> +	dev->vfd_enc->release = video_device_release;
>  	v4l2_info(&dev->v4l2_dev,
>  		  "encoder registered as /dev/video%d\n", dev->vfd_enc->num);
>  

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Re: [PATCH] media: s5p-mfc: avoid double free on video register failure
Posted by Guangshuo Li 4 days, 12 hours ago 
Hi Marek,

Thanks for your feedback.

On Wed, 20 May 2026 at 19:37, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 18.05.2026 15:09, Guangshuo Li wrote:
> > s5p_mfc_probe() allocates video_device instances for both the decoder
> > and encoder and releases them from the probe error paths if
> > video_register_device() fails.
> >
> > This can double free a 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)
> >
> >   s5p_mfc_probe()
> >     -> err_dec_reg or err_enc_reg
> >        -> video_device_release(vdev)
> >
> > Use video_device_release_empty() while registering the decoder and encoder
> > video devices so that registration failure paths do not free them through
> > vdev->release(). s5p_mfc_probe() then releases each video_device exactly
> > once from its error path. Restore video_device_release() after successful
> > registration so the registered devices keep their normal lifetime
> > handling.
> >
> > This issue was found by a static analysis tool I am developing.
> >
> > Fixes: d0ce898c39bf ("[media] s5p-mfc: Replaced commas with semicolons")
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> Frankly speaking I don't like this dancing with video_device_release_empty() and
> video_device_release(). I would rather make video_device struct a part of device
> state and use common release function.
>
> > ---
> >  drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> > index 32eb402d439c..75abb0a8b7a9 100644
> > --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> > +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
> > @@ -1376,7 +1376,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
> >       }
> >       vfd->fops       = &s5p_mfc_fops;
> >       vfd->ioctl_ops  = get_dec_v4l2_ioctl_ops();
> > -     vfd->release    = video_device_release;
> > +     vfd->release    = video_device_release_empty;
> >       vfd->lock       = &dev->mfc_mutex;
> >       vfd->v4l2_dev   = &dev->v4l2_dev;
> >       vfd->vfl_dir    = VFL_DIR_M2M;
> > @@ -1395,7 +1395,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
> >       }
> >       vfd->fops       = &s5p_mfc_fops;
> >       vfd->ioctl_ops  = get_enc_v4l2_ioctl_ops();
> > -     vfd->release    = video_device_release;
> > +     vfd->release    = video_device_release_empty;
> >       vfd->lock       = &dev->mfc_mutex;
> >       vfd->v4l2_dev   = &dev->v4l2_dev;
> >       vfd->vfl_dir    = VFL_DIR_M2M;
> > @@ -1416,6 +1416,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
> >               v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
> >               goto err_dec_reg;
> >       }
> > +
> > +     dev->vfd_dec->release = video_device_release;
> >       v4l2_info(&dev->v4l2_dev,
> >                 "decoder registered as /dev/video%d\n", dev->vfd_dec->num);
> >
> > @@ -1424,6 +1426,8 @@ static int s5p_mfc_probe(struct platform_device *pdev)
> >               v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
> >               goto err_enc_reg;
> >       }
> > +
> > +     dev->vfd_enc->release = video_device_release;
> >       v4l2_info(&dev->v4l2_dev,
> >                 "encoder registered as /dev/video%d\n", dev->vfd_enc->num);
> >
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

I agree that switching between video_device_release_empty() and
video_device_release() is not a good approach. After looking at this
again, I think the real issue should be fixed in the core error
handling path instead of working around it in the s5p-mfc driver.

Please ignore this patch. I will drop it and look into a proper core-side fix.

Thanks,
Guangshuo

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.