[PATCH 0/7] Various core and virtual driver fixes

[PATCH 0/7] Various core and virtual driver fixes

[Hans Verkuil]

Various fixes for bugs that I found while working on the
regression test-media script.

The CONFIG_DEBUG_KOBJECT_RELEASE=y option in particular found
a bunch of bugs where things were not released in the right
order.

Note that the first two patches are not bug fixes, but they
help debugging kobjects. Without this the object name is just
(null), which makes it hard to figure out what the object is.

Regards,

	Hans

Hans Verkuil (7):
  cec: fill in cec chardev kobject to ease debugging
  media-devnode: fill in media chardev kobject to ease debugging
  vivid: use vzalloc for dev->bitmap_out
  media-entity: set ent_enum->bmap to NULL after freeing it
  vim2m: replace devm_kzalloc by kzalloc
  v4l2-device: v4l2_device_release_subdev_node can't reference sd
  vimc: free vimc_cap_device when the last user disappears

 drivers/media/cec/cec-core.c                 |  1 +
 drivers/media/media-devnode.c                |  1 +
 drivers/media/media-entity.c                 |  1 +
 drivers/media/platform/vim2m.c               | 20 +++++++++++++++-----
 drivers/media/platform/vimc/vimc-capture.c   | 13 ++++++++++---
 drivers/media/platform/vivid/vivid-vid-out.c | 14 +++++++++-----
 drivers/media/v4l2-core/v4l2-device.c        | 10 ++--------
 7 files changed, 39 insertions(+), 21 deletions(-)

-- 
2.20.1

[PATCH 1/7] cec: fill in cec chardev kobject to ease debugging

[Hans Verkuil]

The cec chardev kobject has no name, which made it hard to
debug when kobject debugging is turned on.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/cec-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index cc875dabd765..f5d1578e256a 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -126,6 +126,7 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &cec_devnode_fops);
 	devnode->cdev.owner = owner;
+	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
 	if (ret) {
-- 
2.20.1

Re: [PATCH 1/7] cec: fill in cec chardev kobject to ease debugging

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:42PM +0100, Hans Verkuil wrote:
> The cec chardev kobject has no name, which made it hard to
> debug when kobject debugging is turned on.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/cec/cec-core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index cc875dabd765..f5d1578e256a 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -126,6 +126,7 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
>  	/* Part 2: Initialize and register the character device */
>  	cdev_init(&devnode->cdev, &cec_devnode_fops);
>  	devnode->cdev.owner = owner;
> +	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
>  
>  	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
>  	if (ret) {

-- 
Regards,

Laurent Pinchart

[PATCH 2/7] media-devnode: fill in media chardev kobject to ease debugging

[Hans Verkuil]

The media chardev kobject has no name, which made it hard to
debug when kobject debugging is turned on.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-devnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 6b87a721dc49..61dc05fcc55c 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -251,6 +251,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	/* Part 2: Initialize the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
+	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
 	/* Part 3: Add the media and char device */
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
-- 
2.20.1

Re: [PATCH 2/7] media-devnode: fill in media chardev kobject to ease debugging

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:43PM +0100, Hans Verkuil wrote:
> The media chardev kobject has no name, which made it hard to
> debug when kobject debugging is turned on.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-devnode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 6b87a721dc49..61dc05fcc55c 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -251,6 +251,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
>  	/* Part 2: Initialize the character device */
>  	cdev_init(&devnode->cdev, &media_devnode_fops);
>  	devnode->cdev.owner = owner;
> +	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
>  
>  	/* Part 3: Add the media and char device */
>  	ret = cdev_device_add(&devnode->cdev, &devnode->dev);

-- 
Regards,

Laurent Pinchart

[PATCH 3/7] vivid: use vzalloc for dev->bitmap_out

[Hans Verkuil]

When vivid is unloaded it used vfree to free dev->bitmap_out,
but it was actually allocated using kmalloc. Use vzalloc
instead, conform what vivid-vid-cap.c does.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vivid/vivid-vid-out.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
index e61b91b414f9..9350ca65dd91 100644
--- a/drivers/media/platform/vivid/vivid-vid-out.c
+++ b/drivers/media/platform/vivid/vivid-vid-out.c
@@ -798,7 +798,7 @@ int vivid_vid_out_s_selection(struct file *file, void *fh, struct v4l2_selection
 		s->r.height *= factor;
 		if (dev->bitmap_out && (compose->width != s->r.width ||
 					compose->height != s->r.height)) {
-			kfree(dev->bitmap_out);
+			vfree(dev->bitmap_out);
 			dev->bitmap_out = NULL;
 		}
 		*compose = s->r;
@@ -941,15 +941,19 @@ int vidioc_s_fmt_vid_out_overlay(struct file *file, void *priv,
 		return ret;
 
 	if (win->bitmap) {
-		new_bitmap = memdup_user(win->bitmap, bitmap_size);
+		new_bitmap = vzalloc(bitmap_size);
 
-		if (IS_ERR(new_bitmap))
-			return PTR_ERR(new_bitmap);
+		if (!new_bitmap)
+			return -ENOMEM;
+		if (copy_from_user(new_bitmap, win->bitmap, bitmap_size)) {
+			vfree(new_bitmap);
+			return -EFAULT;
+		}
 	}
 
 	dev->overlay_out_top = win->w.top;
 	dev->overlay_out_left = win->w.left;
-	kfree(dev->bitmap_out);
+	vfree(dev->bitmap_out);
 	dev->bitmap_out = new_bitmap;
 	dev->clipcount_out = win->clipcount;
 	if (dev->clipcount_out)
-- 
2.20.1

Re: [PATCH 3/7] vivid: use vzalloc for dev->bitmap_out

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:44PM +0100, Hans Verkuil wrote:
> When vivid is unloaded it used vfree to free dev->bitmap_out,
> but it was actually allocated using kmalloc. Use vzalloc
> instead, conform what vivid-vid-cap.c does.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/vivid/vivid-vid-out.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
> index e61b91b414f9..9350ca65dd91 100644
> --- a/drivers/media/platform/vivid/vivid-vid-out.c
> +++ b/drivers/media/platform/vivid/vivid-vid-out.c
> @@ -798,7 +798,7 @@ int vivid_vid_out_s_selection(struct file *file, void *fh, struct v4l2_selection
>  		s->r.height *= factor;
>  		if (dev->bitmap_out && (compose->width != s->r.width ||
>  					compose->height != s->r.height)) {
> -			kfree(dev->bitmap_out);
> +			vfree(dev->bitmap_out);
>  			dev->bitmap_out = NULL;
>  		}
>  		*compose = s->r;
> @@ -941,15 +941,19 @@ int vidioc_s_fmt_vid_out_overlay(struct file *file, void *priv,
>  		return ret;
>  
>  	if (win->bitmap) {
> -		new_bitmap = memdup_user(win->bitmap, bitmap_size);
> +		new_bitmap = vzalloc(bitmap_size);
>  
> -		if (IS_ERR(new_bitmap))
> -			return PTR_ERR(new_bitmap);
> +		if (!new_bitmap)
> +			return -ENOMEM;
> +		if (copy_from_user(new_bitmap, win->bitmap, bitmap_size)) {
> +			vfree(new_bitmap);
> +			return -EFAULT;
> +		}
>  	}
>  
>  	dev->overlay_out_top = win->w.top;
>  	dev->overlay_out_left = win->w.left;
> -	kfree(dev->bitmap_out);
> +	vfree(dev->bitmap_out);
>  	dev->bitmap_out = new_bitmap;
>  	dev->clipcount_out = win->clipcount;
>  	if (dev->clipcount_out)

-- 
Regards,

Laurent Pinchart

[PATCH 4/7] media-entity: set ent_enum->bmap to NULL after freeing it

[Hans Verkuil]

Ensure that this pointer is set to NULL after it is freed.
The vimc driver has a static media_entity and after
unbinding and rebinding the vimc device the media code will
try to free this pointer again since it wasn't set to NULL.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-entity.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 0b1cb3559140..7b2a2cc95530 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
 void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
 {
 	kfree(ent_enum->bmap);
+	ent_enum->bmap = NULL;
 }
 EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
 
-- 
2.20.1

Re: [PATCH 4/7] media-entity: set ent_enum->bmap to NULL after freeing it

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:45PM +0100, Hans Verkuil wrote:
> Ensure that this pointer is set to NULL after it is freed.
> The vimc driver has a static media_entity and after
> unbinding and rebinding the vimc device the media code will
> try to free this pointer again since it wasn't set to NULL.

As this fixes a problem in vimc, should you add a Fixes: tag ? To avoid
other similar problems, I think the vimc driver should allocate the
media_device and other device data dynamically at probe time. Bundling
them with the platform_device in struct vimc_device isn't a good idea.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-entity.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 0b1cb3559140..7b2a2cc95530 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
>  void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
>  {
>  	kfree(ent_enum->bmap);
> +	ent_enum->bmap = NULL;
>  }
>  EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] media-entity: set ent_enum->bmap to NULL after freeing it

[Hans Verkuil]

On 2/22/19 12:17 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thu, Feb 21, 2019 at 03:21:45PM +0100, Hans Verkuil wrote:
>> Ensure that this pointer is set to NULL after it is freed.
>> The vimc driver has a static media_entity and after
>> unbinding and rebinding the vimc device the media code will
>> try to free this pointer again since it wasn't set to NULL.
> 
> As this fixes a problem in vimc, should you add a Fixes: tag ? To avoid

The Fixes tag is really for cases where a patch introduces a bug, whereas
this was always wrong.

> other similar problems, I think the vimc driver should allocate the
> media_device and other device data dynamically at probe time. Bundling
> them with the platform_device in struct vimc_device isn't a good idea.

It's not actually wrong as such, just unusual. Which actually makes it a
good test case.

Anyway, the upcoming "vimc: add configfs API to configure the topology" patch
makes this dynamic. Waiting for a v2 of that patch.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/media-entity.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index 0b1cb3559140..7b2a2cc95530 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init);
>>  void media_entity_enum_cleanup(struct media_entity_enum *ent_enum)
>>  {
>>  	kfree(ent_enum->bmap);
>> +	ent_enum->bmap = NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
>>  
> 

[PATCH 5/7] vim2m: replace devm_kzalloc by kzalloc

[Hans Verkuil]

It is not possible to use devm_kzalloc since that memory is
freed immediately when the device instance is unbound.

Various objects like the video device may still be in use
since someone has the device node open, and when that is closed
it expects the memory to be around.

So use kzalloc and release it at the appropriate time.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vim2m.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index a27d3052bb62..bfb3e3eb48d1 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file)
 	return 0;
 }
 
+static void vim2m_device_release(struct video_device *vdev)
+{
+	struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd);
+
+	dprintk(dev, "Releasing last dev\n");
+	v4l2_device_unregister(&dev->v4l2_dev);
+	v4l2_m2m_release(dev->m2m_dev);
+	kfree(dev);
+}
+
 static const struct v4l2_file_operations vim2m_fops = {
 	.owner		= THIS_MODULE,
 	.open		= vim2m_open,
@@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = {
 	.fops		= &vim2m_fops,
 	.ioctl_ops	= &vim2m_ioctl_ops,
 	.minor		= -1,
-	.release	= video_device_release_empty,
+	.release	= vim2m_device_release,
 	.device_caps	= V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING,
 };
 
@@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev)
 	struct video_device *vfd;
 	int ret;
 
-	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
-		return ret;
+		goto unreg_free;
 
 	atomic_set(&dev->num_inst, 0);
 	mutex_init(&dev->dev_mutex);
@@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev)
 	video_unregister_device(&dev->vfd);
 unreg_v4l2:
 	v4l2_device_unregister(&dev->v4l2_dev);
+unreg_free:
+	kfree(dev);
 
 	return ret;
 }
@@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev)
 	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
 	media_device_cleanup(&dev->mdev);
 #endif
-	v4l2_m2m_release(dev->m2m_dev);
 	video_unregister_device(&dev->vfd);
-	v4l2_device_unregister(&dev->v4l2_dev);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 5/7] vim2m: replace devm_kzalloc by kzalloc

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:46PM +0100, Hans Verkuil wrote:
> It is not possible to use devm_kzalloc since that memory is
> freed immediately when the device instance is unbound.
> 
> Various objects like the video device may still be in use
> since someone has the device node open, and when that is closed
> it expects the memory to be around.
> 
> So use kzalloc and release it at the appropriate time.

You're opening a can of worms, we have tons of drivers that use
devm_kzalloc() :-) I however believe this is the right course of action.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/vim2m.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
> index a27d3052bb62..bfb3e3eb48d1 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file)
>  	return 0;
>  }
>  
> +static void vim2m_device_release(struct video_device *vdev)
> +{
> +	struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd);
> +
> +	dprintk(dev, "Releasing last dev\n");

Do we really need a debug printk here ?

> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	v4l2_m2m_release(dev->m2m_dev);
> +	kfree(dev);
> +}
> +
>  static const struct v4l2_file_operations vim2m_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= vim2m_open,
> @@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = {
>  	.fops		= &vim2m_fops,
>  	.ioctl_ops	= &vim2m_ioctl_ops,
>  	.minor		= -1,
> -	.release	= video_device_release_empty,
> +	.release	= vim2m_device_release,
>  	.device_caps	= V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING,
>  };
>  
> @@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev)
>  	struct video_device *vfd;
>  	int ret;
>  
> -	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev)
>  		return -ENOMEM;
>  
>  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>  	if (ret)
> -		return ret;
> +		goto unreg_free;
>  
>  	atomic_set(&dev->num_inst, 0);
>  	mutex_init(&dev->dev_mutex);
> @@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev)
>  	video_unregister_device(&dev->vfd);
>  unreg_v4l2:
>  	v4l2_device_unregister(&dev->v4l2_dev);
> +unreg_free:

I'd call the label error_free, and rename the other ones with an error_
prefix, as you don't register anything here.

With these two small issues fixes,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	kfree(dev);
>  
>  	return ret;
>  }
> @@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev)
>  	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
>  	media_device_cleanup(&dev->mdev);
>  #endif
> -	v4l2_m2m_release(dev->m2m_dev);
>  	video_unregister_device(&dev->vfd);
> -	v4l2_device_unregister(&dev->v4l2_dev);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] vim2m: replace devm_kzalloc by kzalloc

[Hans Verkuil]

On 2/22/19 12:20 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thu, Feb 21, 2019 at 03:21:46PM +0100, Hans Verkuil wrote:
>> It is not possible to use devm_kzalloc since that memory is
>> freed immediately when the device instance is unbound.
>>
>> Various objects like the video device may still be in use
>> since someone has the device node open, and when that is closed
>> it expects the memory to be around.
>>
>> So use kzalloc and release it at the appropriate time.
> 
> You're opening a can of worms, we have tons of drivers that use
> devm_kzalloc() :-) I however believe this is the right course of action.
> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/platform/vim2m.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
>> index a27d3052bb62..bfb3e3eb48d1 100644
>> --- a/drivers/media/platform/vim2m.c
>> +++ b/drivers/media/platform/vim2m.c
>> @@ -1087,6 +1087,16 @@ static int vim2m_release(struct file *file)
>>  	return 0;
>>  }
>>  
>> +static void vim2m_device_release(struct video_device *vdev)
>> +{
>> +	struct vim2m_dev *dev = container_of(vdev, struct vim2m_dev, vfd);
>> +
>> +	dprintk(dev, "Releasing last dev\n");
> 
> Do we really need a debug printk here ?

Oops, that's a left-over debug message. I'll remove it.

> 
>> +	v4l2_device_unregister(&dev->v4l2_dev);
>> +	v4l2_m2m_release(dev->m2m_dev);
>> +	kfree(dev);
>> +}
>> +
>>  static const struct v4l2_file_operations vim2m_fops = {
>>  	.owner		= THIS_MODULE,
>>  	.open		= vim2m_open,
>> @@ -1102,7 +1112,7 @@ static const struct video_device vim2m_videodev = {
>>  	.fops		= &vim2m_fops,
>>  	.ioctl_ops	= &vim2m_ioctl_ops,
>>  	.minor		= -1,
>> -	.release	= video_device_release_empty,
>> +	.release	= vim2m_device_release,
>>  	.device_caps	= V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING,
>>  };
>>  
>> @@ -1123,13 +1133,13 @@ static int vim2m_probe(struct platform_device *pdev)
>>  	struct video_device *vfd;
>>  	int ret;
>>  
>> -	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>  	if (!dev)
>>  		return -ENOMEM;
>>  
>>  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>>  	if (ret)
>> -		return ret;
>> +		goto unreg_free;
>>  
>>  	atomic_set(&dev->num_inst, 0);
>>  	mutex_init(&dev->dev_mutex);
>> @@ -1192,6 +1202,8 @@ static int vim2m_probe(struct platform_device *pdev)
>>  	video_unregister_device(&dev->vfd);
>>  unreg_v4l2:
>>  	v4l2_device_unregister(&dev->v4l2_dev);
>> +unreg_free:
> 
> I'd call the label error_free, and rename the other ones with an error_
> prefix, as you don't register anything here.

OK

> 
> With these two small issues fixes,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Regards,

	Hans

> 
>> +	kfree(dev);
>>  
>>  	return ret;
>>  }
>> @@ -1207,9 +1219,7 @@ static int vim2m_remove(struct platform_device *pdev)
>>  	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
>>  	media_device_cleanup(&dev->mdev);
>>  #endif
>> -	v4l2_m2m_release(dev->m2m_dev);
>>  	video_unregister_device(&dev->vfd);
>> -	v4l2_device_unregister(&dev->v4l2_dev);
>>  
>>  	return 0;
>>  }
> 

[PATCH 6/7] v4l2-device: v4l2_device_release_subdev_node can't reference sd

[Hans Verkuil]

When the v4l-subdev device node is released it calls the
v4l2_device_release_subdev_node() function which sets sd->devnode
to NULL.

However, the v4l2_subdev struct may already be released causing this
to write in freed memory.

Instead just use the regular video_device_release release function
(just calls kfree) and set sd->devnode to NULL right after the
video_unregister_device() call.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-device.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index e0ddb9a52bd1..57a7b220fa4d 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 }
 EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
 
-static void v4l2_device_release_subdev_node(struct video_device *vdev)
-{
-	struct v4l2_subdev *sd = video_get_drvdata(vdev);
-	sd->devnode = NULL;
-	kfree(vdev);
-}
-
 int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 {
 	struct video_device *vdev;
@@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		vdev->dev_parent = sd->dev;
 		vdev->v4l2_dev = v4l2_dev;
 		vdev->fops = &v4l2_subdev_fops;
-		vdev->release = v4l2_device_release_subdev_node;
+		vdev->release = video_device_release;
 		vdev->ctrl_handler = sd->ctrl_handler;
 		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
 					      sd->owner);
@@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	}
 #endif
 	video_unregister_device(sd->devnode);
+	sd->devnode = NULL;
 	if (!sd->owner_v4l2_dev)
 		module_put(sd->owner);
 }
-- 
2.20.1

Re: [PATCH 6/7] v4l2-device: v4l2_device_release_subdev_node can't reference sd

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote:
> When the v4l-subdev device node is released it calls the
> v4l2_device_release_subdev_node() function which sets sd->devnode
> to NULL.
> 
> However, the v4l2_subdev struct may already be released causing this
> to write in freed memory.
> 
> Instead just use the regular video_device_release release function
> (just calls kfree) and set sd->devnode to NULL right after the
> video_unregister_device() call.

This seems a bit of a workaround. The devnode can access the subdev in
multiple ways, it should really keep a reference to the subdev to ensure
it doesn't get freed early.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-device.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index e0ddb9a52bd1..57a7b220fa4d 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>  
> -static void v4l2_device_release_subdev_node(struct video_device *vdev)
> -{
> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
> -	sd->devnode = NULL;
> -	kfree(vdev);
> -}
> -
>  int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>  {
>  	struct video_device *vdev;
> @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>  		vdev->dev_parent = sd->dev;
>  		vdev->v4l2_dev = v4l2_dev;
>  		vdev->fops = &v4l2_subdev_fops;
> -		vdev->release = v4l2_device_release_subdev_node;
> +		vdev->release = video_device_release;
>  		vdev->ctrl_handler = sd->ctrl_handler;
>  		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
>  					      sd->owner);
> @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  	}
>  #endif
>  	video_unregister_device(sd->devnode);
> +	sd->devnode = NULL;
>  	if (!sd->owner_v4l2_dev)
>  		module_put(sd->owner);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] v4l2-device: v4l2_device_release_subdev_node can't reference sd

[Hans Verkuil]

On 2/22/19 12:32 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote:
>> When the v4l-subdev device node is released it calls the
>> v4l2_device_release_subdev_node() function which sets sd->devnode
>> to NULL.
>>
>> However, the v4l2_subdev struct may already be released causing this
>> to write in freed memory.
>>
>> Instead just use the regular video_device_release release function
>> (just calls kfree) and set sd->devnode to NULL right after the
>> video_unregister_device() call.
> 
> This seems a bit of a workaround. The devnode can access the subdev in
> multiple ways, it should really keep a reference to the subdev to ensure
> it doesn't get freed early.

It's not the link from the devnode to the subdev (that's done through
video_get_drvdata()), it's the link from the subdev to the devnode.

As soon as the video device is unregistered sd->devnode should be set
to NULL. It is in fact how sd->devnode is used: as a check if the devnode
was registered.

The only other place where it is used is in v4l2_subdev_notify_event to
send an event to the devnode, and after unregistering the video device
you no longer want to do that, so setting sd->devnode to NULL after it
is unregistered is the right thing to do.

FYI, I'll post a v2 of this series soon.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-device.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>> index e0ddb9a52bd1..57a7b220fa4d 100644
>> --- a/drivers/media/v4l2-core/v4l2-device.c
>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>> @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>>  
>> -static void v4l2_device_release_subdev_node(struct video_device *vdev)
>> -{
>> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
>> -	sd->devnode = NULL;
>> -	kfree(vdev);
>> -}
>> -
>>  int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>>  {
>>  	struct video_device *vdev;
>> @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>>  		vdev->dev_parent = sd->dev;
>>  		vdev->v4l2_dev = v4l2_dev;
>>  		vdev->fops = &v4l2_subdev_fops;
>> -		vdev->release = v4l2_device_release_subdev_node;
>> +		vdev->release = video_device_release;
>>  		vdev->ctrl_handler = sd->ctrl_handler;
>>  		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
>>  					      sd->owner);
>> @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>>  	}
>>  #endif
>>  	video_unregister_device(sd->devnode);
>> +	sd->devnode = NULL;
>>  	if (!sd->owner_v4l2_dev)
>>  		module_put(sd->owner);
>>  }
> 

Re: [PATCH 6/7] v4l2-device: v4l2_device_release_subdev_node can't reference sd

[Laurent Pinchart]

On Fri, Mar 01, 2019 at 12:46:52PM +0100, Hans Verkuil wrote:
> On 2/22/19 12:32 PM, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote:
> >> When the v4l-subdev device node is released it calls the
> >> v4l2_device_release_subdev_node() function which sets sd->devnode
> >> to NULL.
> >>
> >> However, the v4l2_subdev struct may already be released causing this
> >> to write in freed memory.
> >>
> >> Instead just use the regular video_device_release release function
> >> (just calls kfree) and set sd->devnode to NULL right after the
> >> video_unregister_device() call.
> > 
> > This seems a bit of a workaround. The devnode can access the subdev in
> > multiple ways, it should really keep a reference to the subdev to ensure
> > it doesn't get freed early.
> 
> It's not the link from the devnode to the subdev (that's done through
> video_get_drvdata()), it's the link from the subdev to the devnode.

Right, my bad.

> As soon as the video device is unregistered sd->devnode should be set
> to NULL. It is in fact how sd->devnode is used: as a check if the devnode
> was registered.
> 
> The only other place where it is used is in v4l2_subdev_notify_event to
> send an event to the devnode, and after unregistering the video device
> you no longer want to do that, so setting sd->devnode to NULL after it
> is unregistered is the right thing to do.

Is there a risk the two function could race each other ?

> FYI, I'll post a v2 of this series soon.
> 
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-device.c | 10 ++--------
> >>  1 file changed, 2 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> >> index e0ddb9a52bd1..57a7b220fa4d 100644
> >> --- a/drivers/media/v4l2-core/v4l2-device.c
> >> +++ b/drivers/media/v4l2-core/v4l2-device.c
> >> @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
> >>  }
> >>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
> >>  
> >> -static void v4l2_device_release_subdev_node(struct video_device *vdev)
> >> -{
> >> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
> >> -	sd->devnode = NULL;
> >> -	kfree(vdev);
> >> -}
> >> -
> >>  int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> >>  {
> >>  	struct video_device *vdev;
> >> @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> >>  		vdev->dev_parent = sd->dev;
> >>  		vdev->v4l2_dev = v4l2_dev;
> >>  		vdev->fops = &v4l2_subdev_fops;
> >> -		vdev->release = v4l2_device_release_subdev_node;
> >> +		vdev->release = video_device_release;
> >>  		vdev->ctrl_handler = sd->ctrl_handler;
> >>  		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
> >>  					      sd->owner);
> >> @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> >>  	}
> >>  #endif
> >>  	video_unregister_device(sd->devnode);
> >> +	sd->devnode = NULL;
> >>  	if (!sd->owner_v4l2_dev)
> >>  		module_put(sd->owner);
> >>  }

-- 
Regards,

Laurent Pinchart

[PATCH 7/7] vimc: free vimc_cap_device when the last user disappears

[Hans Verkuil]

Don't free vimc_cap_device immediately, instead do this
in the video_device release function which is called when the
last user closes the video device. Only then is it safe to
free the memory.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vimc/vimc-capture.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index ea869631a3f6..3d433361d297 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -338,6 +338,15 @@ static const struct media_entity_operations vimc_cap_mops = {
 	.link_validate		= vimc_link_validate,
 };
 
+static void vimc_cap_release(struct video_device *vdev)
+{
+	struct vimc_cap_device *vcap = container_of(vdev, struct vimc_cap_device,
+						    vdev);
+
+	vimc_pads_cleanup(vcap->ved.pads);
+	kfree(vcap);
+}
+
 static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 				 void *master_data)
 {
@@ -348,8 +357,6 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
 	vb2_queue_release(&vcap->queue);
 	media_entity_cleanup(ved->ent);
 	video_unregister_device(&vcap->vdev);
-	vimc_pads_cleanup(vcap->ved.pads);
-	kfree(vcap);
 }
 
 static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
@@ -467,7 +474,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	vdev = &vcap->vdev;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
 	vdev->entity.ops = &vimc_cap_mops;
-	vdev->release = video_device_release_empty;
+	vdev->release = vimc_cap_release;
 	vdev->fops = &vimc_cap_fops;
 	vdev->ioctl_ops = &vimc_cap_ioctl_ops;
 	vdev->lock = &vcap->lock;
-- 
2.20.1

Re: [PATCH 7/7] vimc: free vimc_cap_device when the last user disappears

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Feb 21, 2019 at 03:21:48PM +0100, Hans Verkuil wrote:
> Don't free vimc_cap_device immediately, instead do this
> in the video_device release function which is called when the
> last user closes the video device. Only then is it safe to
> free the memory.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index ea869631a3f6..3d433361d297 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -338,6 +338,15 @@ static const struct media_entity_operations vimc_cap_mops = {
>  	.link_validate		= vimc_link_validate,
>  };
>  
> +static void vimc_cap_release(struct video_device *vdev)
> +{
> +	struct vimc_cap_device *vcap = container_of(vdev, struct vimc_cap_device,
> +						    vdev);
> +
> +	vimc_pads_cleanup(vcap->ved.pads);
> +	kfree(vcap);
> +}
> +
>  static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>  				 void *master_data)
>  {
> @@ -348,8 +357,6 @@ static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>  	vb2_queue_release(&vcap->queue);
>  	media_entity_cleanup(ved->ent);
>  	video_unregister_device(&vcap->vdev);
> -	vimc_pads_cleanup(vcap->ved.pads);
> -	kfree(vcap);
>  }
>  
>  static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> @@ -467,7 +474,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	vdev = &vcap->vdev;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>  	vdev->entity.ops = &vimc_cap_mops;
> -	vdev->release = video_device_release_empty;
> +	vdev->release = vimc_cap_release;

Another use of video_device_release_empty gone, only 80+ to go ;-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	vdev->fops = &vimc_cap_fops;
>  	vdev->ioctl_ops = &vimc_cap_ioctl_ops;
>  	vdev->lock = &vcap->lock;

-- 
Regards,

Laurent Pinchart