* [PATCH] [Patch v2] usbtv: Fix refcounting mixup
@ 2018-05-15 13:07 Oliver Neukum
2018-05-15 14:28 ` Hans Verkuil
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Neukum @ 2018-05-15 13:07 UTC (permalink / raw)
To: mchehab, ben.hutchings, gregkh, linux-media; +Cc: Oliver Neukum, stable
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-15 13:07 [PATCH] [Patch v2] usbtv: Fix refcounting mixup Oliver Neukum @ 2018-05-15 14:28 ` Hans Verkuil 2018-05-15 15:46 ` Oliver Neukum 0 siblings, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2018-05-15 14:28 UTC (permalink / raw) To: Oliver Neukum, mchehab, ben.hutchings, gregkh, linux-media; +Cc: stable 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: > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-15 14:28 ` Hans Verkuil @ 2018-05-15 15:46 ` Oliver Neukum 2018-05-15 16:01 ` Hans Verkuil 0 siblings, 1 reply; 8+ messages in thread From: Oliver Neukum @ 2018-05-15 15:46 UTC (permalink / raw) To: Hans Verkuil, ben.hutchings, gregkh, mchehab, linux-media; +Cc: stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-15 15:46 ` Oliver Neukum @ 2018-05-15 16:01 ` Hans Verkuil 2018-05-16 9:23 ` Oliver Neukum 0 siblings, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2018-05-15 16:01 UTC (permalink / raw) To: Oliver Neukum, ben.hutchings, gregkh, mchehab, linux-media; +Cc: stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-15 16:01 ` Hans Verkuil @ 2018-05-16 9:23 ` Oliver Neukum 2018-05-16 10:27 ` Hans Verkuil 0 siblings, 1 reply; 8+ messages in thread From: Oliver Neukum @ 2018-05-16 9:23 UTC (permalink / raw) To: Hans Verkuil, ben.hutchings, gregkh, mchehab, linux-media; +Cc: stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-16 9:23 ` Oliver Neukum @ 2018-05-16 10:27 ` Hans Verkuil 2020-09-16 13:47 ` Oliver Neukum 2020-09-21 9:10 ` Oliver Neukum 0 siblings, 2 replies; 8+ messages in thread From: Hans Verkuil @ 2018-05-16 10:27 UTC (permalink / raw) To: Oliver Neukum, ben.hutchings, gregkh, mchehab, linux-media; +Cc: stable 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-16 10:27 ` Hans Verkuil @ 2020-09-16 13:47 ` Oliver Neukum 2020-09-21 9:10 ` Oliver Neukum 1 sibling, 0 replies; 8+ messages in thread From: Oliver Neukum @ 2020-09-16 13:47 UTC (permalink / raw) To: Hans Verkuil, ben.hutchings, gregkh, mchehab, linux-media; +Cc: stable [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup 2018-05-16 10:27 ` Hans Verkuil 2020-09-16 13:47 ` Oliver Neukum @ 2020-09-21 9:10 ` Oliver Neukum 1 sibling, 0 replies; 8+ messages in thread From: Oliver Neukum @ 2020-09-21 9:10 UTC (permalink / raw) To: Hans Verkuil, ben.hutchings, gregkh, linux-media; +Cc: stable [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-21 9:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-15 13:07 [PATCH] [Patch v2] usbtv: Fix refcounting mixup Oliver Neukum 2018-05-15 14:28 ` Hans Verkuil 2018-05-15 15:46 ` Oliver Neukum 2018-05-15 16:01 ` Hans Verkuil 2018-05-16 9:23 ` Oliver Neukum 2018-05-16 10:27 ` Hans Verkuil 2020-09-16 13:47 ` Oliver Neukum 2020-09-21 9:10 ` Oliver Neukum
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.