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:
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:
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Hans Verkuil -
Oliver Neukum -
Oliver Neukum