[PATCH] [Patch v2] usbtv: Fix refcounting mixup

[PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Oliver Neukum]

The premature free in the error path is blocked by V4L
refcounting, not USB refcounting. Thanks to
Ben Hutchings for review.

[v2] corrected attributions

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
CC: stable@vger.kernel.org
Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
index 5095c380b2c1..4a03c4d66314 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
 
 usbtv_audio_fail:
 	/* we must not free at this point */
-	usb_get_dev(usbtv->udev);
+	v4l2_device_get(&usbtv->v4l2_dev);
+	/* this will undo the v4l2_device_get() */
 	usbtv_video_free(usbtv);
 
 usbtv_video_fail:
-- 
2.13.6

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Hans Verkuil]

On 05/15/18 15:07, Oliver Neukum wrote:
> The premature free in the error path is blocked by V4L
> refcounting, not USB refcounting. Thanks to
> Ben Hutchings for review.
> 
> [v2] corrected attributions
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
> CC: stable@vger.kernel.org
> Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> ---
>  drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
> index 5095c380b2c1..4a03c4d66314 100644
> --- a/drivers/media/usb/usbtv/usbtv-core.c
> +++ b/drivers/media/usb/usbtv/usbtv-core.c
> @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
>  
>  usbtv_audio_fail:
>  	/* we must not free at this point */
> -	usb_get_dev(usbtv->udev);
> +	v4l2_device_get(&usbtv->v4l2_dev);

This is very confusing. I think it is much better to move the
v4l2_device_register() call from usbtv_video_init to this probe function.

The extra v4l2_device_get in the probe() can just be dropped and
usbtv_video_free() no longer needs to call v4l2_device_put().

The only place you need a v4l2_device_put() is in the disconnect()
function at the end.

Regards,

	Hans

> +	/* this will undo the v4l2_device_get() */
>  	usbtv_video_free(usbtv);
>  
>  usbtv_video_fail:
> 

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Oliver Neukum]

Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
> On 05/15/18 15:07, Oliver Neukum wrote:
> > The premature free in the error path is blocked by V4L
> > refcounting, not USB refcounting. Thanks to
> > Ben Hutchings for review.
> > 
> > [v2] corrected attributions
> > 
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
> > CC: stable@vger.kernel.org
> > Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> >  drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
> > index 5095c380b2c1..4a03c4d66314 100644
> > --- a/drivers/media/usb/usbtv/usbtv-core.c
> > +++ b/drivers/media/usb/usbtv/usbtv-core.c
> > @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
> >  
> >  usbtv_audio_fail:
> >  	/* we must not free at this point */
> > -	usb_get_dev(usbtv->udev);
> > +	v4l2_device_get(&usbtv->v4l2_dev);
> 
> This is very confusing. I think it is much better to move the

Yes. It confused me.

> v4l2_device_register() call from usbtv_video_init to this probe function.

Yes, but it is called here. So you want to do it after registering the
audio?

	Regards
		Oliver

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Hans Verkuil]

On 05/15/2018 05:46 PM, Oliver Neukum wrote:
> Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
>> On 05/15/18 15:07, Oliver Neukum wrote:
>>> The premature free in the error path is blocked by V4L
>>> refcounting, not USB refcounting. Thanks to
>>> Ben Hutchings for review.
>>>
>>> [v2] corrected attributions
>>>
>>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>>> Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
>>> CC: stable@vger.kernel.org
>>> Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
>>> ---
>>>  drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
>>> index 5095c380b2c1..4a03c4d66314 100644
>>> --- a/drivers/media/usb/usbtv/usbtv-core.c
>>> +++ b/drivers/media/usb/usbtv/usbtv-core.c
>>> @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
>>>  
>>>  usbtv_audio_fail:
>>>  	/* we must not free at this point */
>>> -	usb_get_dev(usbtv->udev);
>>> +	v4l2_device_get(&usbtv->v4l2_dev);
>>
>> This is very confusing. I think it is much better to move the
> 
> Yes. It confused me.
> 
>> v4l2_device_register() call from usbtv_video_init to this probe function.
> 
> Yes, but it is called here. So you want to do it after registering the
> audio?

No, before. It's a global data structure, so this can be done before the
call to usbtv_video_init() as part of the top-level initialization of the
driver.

Regards,

	Hans

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Oliver Neukum]

Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil:
> On 05/15/2018 05:46 PM, Oliver Neukum wrote:
> > Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
> > > On 05/15/18 15:07, Oliver Neukum wrote:

> > > >  usbtv_audio_fail:
> > > >  	/* we must not free at this point */
> > > > -	usb_get_dev(usbtv->udev);
> > > > +	v4l2_device_get(&usbtv->v4l2_dev);
> > > 
> > > This is very confusing. I think it is much better to move the
> > 
> > Yes. It confused me.
> > 
> > > v4l2_device_register() call from usbtv_video_init to this probe function.
> > 
> > Yes, but it is called here. So you want to do it after registering the
> > audio?
> 
> No, before. It's a global data structure, so this can be done before the
> call to usbtv_video_init() as part of the top-level initialization of the
> driver.

Eh, but we cannot create a V4L device before the first device
is connected and we must certainly create multiple V4L devices if
multiple physical devices are connected.

Maybe I am dense. Please elaborate.
It seem to me that the driver is confusing because it uses
multiple refcounts.

	Regards
		Oliver

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Hans Verkuil]

On 05/16/18 11:23, Oliver Neukum wrote:
> Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil:
>> On 05/15/2018 05:46 PM, Oliver Neukum wrote:
>>> Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
>>>> On 05/15/18 15:07, Oliver Neukum wrote:
> 
>>>>>  usbtv_audio_fail:
>>>>>  	/* we must not free at this point */
>>>>> -	usb_get_dev(usbtv->udev);
>>>>> +	v4l2_device_get(&usbtv->v4l2_dev);
>>>>
>>>> This is very confusing. I think it is much better to move the
>>>
>>> Yes. It confused me.
>>>
>>>> v4l2_device_register() call from usbtv_video_init to this probe function.
>>>
>>> Yes, but it is called here. So you want to do it after registering the
>>> audio?
>>
>> No, before. It's a global data structure, so this can be done before the
>> call to usbtv_video_init() as part of the top-level initialization of the
>> driver.
> 
> Eh, but we cannot create a V4L device before the first device
> is connected and we must certainly create multiple V4L devices if
> multiple physical devices are connected.

v4l2_device_register is a terrible name. It does not create devices
or register with anything, it just initializes a root data structure. I have
proposed renaming this to v4l2_root_init() in the past, but people didn't
want a big rename action.

BTW, with 'global data structure' I meant a data structure in struct usbtv.
All I meant to say is that v4l2_device_register should be called in probe(),
not in usbtv_video_init().

Regards,

	Hans

> 
> Maybe I am dense. Please elaborate.
> It seem to me that the driver is confusing because it uses
> multiple refcounts.
> 
> 	Regards
> 		Oliver
> 

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

Am Mittwoch, den 16.05.2018, 12:27 +0200 schrieb Hans Verkuil:
> On 05/16/18 11:23, Oliver Neukum wrote:
> > Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil:
> > > On 05/15/2018 05:46 PM, Oliver Neukum wrote:
> > > > Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
> > > > > On 05/15/18 15:07, Oliver Neukum wrote:

> > Eh, but we cannot create a V4L device before the first device
> > is connected and we must certainly create multiple V4L devices if
> > multiple physical devices are connected.
> 
> v4l2_device_register is a terrible name. It does not create devices
> or register with anything, it just initializes a root data structure. I have
> proposed renaming this to v4l2_root_init() in the past, but people didn't
> want a big rename action.
> 
> BTW, with 'global data structure' I meant a data structure in struct usbtv.
> All I meant to say is that v4l2_device_register should be called in probe(),
> not in usbtv_video_init().

Hi,

Sorry for thread necromancy I am cleaning up electronically.
This patch has fallen through the cracks. As far as I can see the issue
is still open. I screwed this up. So do you want me to do a major
redesign? If not, what is to be done?

	Regards
		Oliver



[-- Attachment #2: 0001-Patch-v2-usbtv-Fix-refcounting-mixup.patch --]
[-- Type: text/x-patch, Size: 1182 bytes --]

From c6040618651d670b895198096e3fa442b6cf8a26 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 15 May 2018 12:16:26 +0200
Subject: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

The premature free in the error path is blocked by V4L
refcounting, not USB refcounting. Thanks to
Ben Hutchings for review.

[v2] corrected attributions

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
CC: stable@vger.kernel.org
Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
index ee9c656d121f..2308c0b4f5e7 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
 
 usbtv_audio_fail:
 	/* we must not free at this point */
-	usb_get_dev(usbtv->udev);
+	v4l2_device_get(&usbtv->v4l2_dev);
+	/* this will undo the v4l2_device_get() */
 	usbtv_video_free(usbtv);
 
 usbtv_video_fail:
-- 
2.16.4

Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Am Mittwoch, den 16.05.2018, 12:27 +0200 schrieb Hans Verkuil:
> On 05/16/18 11:23, Oliver Neukum wrote:
> > Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil:
> > > On 05/15/2018 05:46 PM, Oliver Neukum wrote:
> > > > Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
> > > > > On 05/15/18 15:07, Oliver Neukum wrote:
> > Eh, but we cannot create a V4L device before the first device
> > is connected and we must certainly create multiple V4L devices if
> > multiple physical devices are connected.
> 
> v4l2_device_register is a terrible name. It does not create devices
> or register with anything, it just initializes a root data structure. I have
> proposed renaming this to v4l2_root_init() in the past, but people didn't
> want a big rename action.
> 
> BTW, with 'global data structure' I meant a data structure in struct usbtv.
> All I meant to say is that v4l2_device_register should be called in probe(),
> not in usbtv_video_init().

Hi,

Sorry for thread necromancy I am cleaning up electronically.
This patch has fallen through the cracks. As far as I can see the issue
is still open. I screwed this up. So do you want me to do a major
redesign? If not, what is to be done?

	Regards
		Oliver



[-- Attachment #2: 0001-Patch-v2-usbtv-Fix-refcounting-mixup.patch --]
[-- Type: text/x-patch, Size: 1182 bytes --]

From c6040618651d670b895198096e3fa442b6cf8a26 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 15 May 2018 12:16:26 +0200
Subject: [PATCH] [Patch v2] usbtv: Fix refcounting mixup

The premature free in the error path is blocked by V4L
refcounting, not USB refcounting. Thanks to
Ben Hutchings for review.

[v2] corrected attributions

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
CC: stable@vger.kernel.org
Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
index ee9c656d121f..2308c0b4f5e7 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
 
 usbtv_audio_fail:
 	/* we must not free at this point */
-	usb_get_dev(usbtv->udev);
+	v4l2_device_get(&usbtv->v4l2_dev);
+	/* this will undo the v4l2_device_get() */
 	usbtv_video_free(usbtv);
 
 usbtv_video_fail:
-- 
2.16.4