Re: [PATCH] staging: most: video: fix refcount leak in comp_probe_channel()

[Dan Carpenter]

On Thu, Jun 11, 2026 at 07:43:35PM +0800, WenTao Liang wrote:
> If v4l2_device_register() fails in comp_probe_channel(), the
> function frees the allocated mdev with kfree() without releasing the
> reference count held by the embedded v4l2_device.  Because
> v4l2_device_register() initializes a kref in the v4l2_device, the
> reference count is already 1 on failure.  Dropping the last reference
> must be done with v4l2_device_put() so that the release callback can
> unregister the v4l2_device and free mdev.

What are you talking about here?

	kref_init(&v4l2_dev->ref);

This is just a "refcount = 1" assignment.  There is no allocation or
need to free anything.

> 
> Replace the kfree(mdev) with v4l2_device_put(&mdev->v4l2_dev).  The
> error path for comp_register_videodev() failure already does this
> correctly.

This is a weird and confusing to say.  In comp_register_videodev()
we call video_device_release() which is a wrapper around kfree() and
here the original code calls kfree() directly...  The original code
is more similar to comp_register_videodev() than the new code.

> 
> Cc: stable@xxxxxxxxxxxxxxx

CCing stable isn't necessary since v4l2_device_register() can't actually
fail here in real life.

drivers/media/v4l2-core/v4l2-device.c
    17  int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
    18  {
    19          if (v4l2_dev == NULL)

v4l2_dev is non-NULL.

    20                  return -EINVAL;
    21  
    22          INIT_LIST_HEAD(&v4l2_dev->subdevs);
    23          spin_lock_init(&v4l2_dev->lock);
    24          v4l2_prio_init(&v4l2_dev->prio);
    25          kref_init(&v4l2_dev->ref);
    26          get_device(dev);
    27          v4l2_dev->dev = dev;
    28          if (dev == NULL) {

dev is NULL

    29                  /* If dev == NULL, then name must be filled in by the caller */
    30                  if (WARN_ON(!v4l2_dev->name[0]))

The name is filled in.

    31                          return -EINVAL;
    32                  return 0;
                        ^^^^^^^^
We return success.

    33          }

> Fixes: 3d31c0cb6c12 ("Staging: most: add MOST driver's aim-v4l2 module")
> Signed-off-by: WenTao Liang <vulab@xxxxxxxxxxx>

Please put in the commit message if this that this was discovered via AI
and not tested or whatever...

> ---
>  drivers/staging/most/video/video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 04351f8ccccf..aa846959b217 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -491,7 +491,7 @@ static int comp_probe_channel(struct most_interface *iface, int channel_idx,
>  	ret = v4l2_device_register(NULL, &mdev->v4l2_dev);
>  	if (ret) {
>  		pr_err("v4l2_device_register() failed\n");
> -		kfree(mdev);
> +		v4l2_device_put(&mdev->v4l2_dev);

v4l2_device_put() will call comp_v4l2_dev_release() which is calls:

	v4l2_device_unregister(v4l2_dev);
	kfree(mdev);

The call to v4l2_device_unregister() is a no-op since the register
failed (pretending that were possible) so at runtime this is the exact
same as calling kfree(mdev);

So this is not a bug.  The original code is fine.  We could argue
about readability, but I feel like the original code is in some ways
more readable.  I don't like calling unregister() when the device
is not registered.

regards,
dan carpenter

>  		return ret;
>  	}
>  
> -- 
> 2.50.1 (Apple Git-155)