diff mbox series

media: usb: fix memory leak in stk_camera_probe

Message ID	20210714032340.504836-1-mudongliangabcd@gmail.com (mailing list archive)
State	Obsoleted, archived
Delegated to:	Hans Verkuil
Headers	From: Dongliang Mu <mudongliangabcd@gmail.com> To: Mauro Carvalho Chehab <mchehab@kernel.org>, Dongliang Mu <mudongliangabcd@gmail.com> Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] media: usb: fix memory leak in stk_camera_probe Date: Wed, 14 Jul 2021 11:23:28 +0800 Message-Id: <20210714032340.504836-1-mudongliangabcd@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	media: usb: fix memory leak in stk_camera_probe \| media: usb: fix memory leak in stk_camera_probe

Commit Message

Dongliang Mu July 14, 2021, 3:23 a.m. UTC

  stk_camera_probe mistakenly execute usb_get_intf and increase the
refcount of interface->dev.

Fix this by removing the execution of usb_get_intf.

Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Dongliang Mu July 23, 2021, 10:11 a.m. UTC | #1

On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> stk_camera_probe mistakenly execute usb_get_intf and increase the
> refcount of interface->dev.
>
> Fix this by removing the execution of usb_get_intf.

Any idea about this patch?

>
> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index a45d464427c4..5bd8e85b9452 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
>
>         dev->udev = udev;
>         dev->interface = interface;
> -       usb_get_intf(interface);
>
>         if (hflip != -1)
>                 dev->vsettings.hflip = hflip;
> --
> 2.25.1
>

Dongliang Mu Sept. 2, 2021, 10:23 a.m. UTC | #2

On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > refcount of interface->dev.
> >
> > Fix this by removing the execution of usb_get_intf.
>
> Any idea about this patch?

+cc Dan Carpenter, gregkh

There is no reply in this thread in one month. Can someone give some
feedback on this patch?

>
> >
> > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> > index a45d464427c4..5bd8e85b9452 100644
> > --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> > @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
> >
> >         dev->udev = udev;
> >         dev->interface = interface;
> > -       usb_get_intf(interface);
> >
> >         if (hflip != -1)
> >                 dev->vsettings.hflip = hflip;
> > --
> > 2.25.1
> >

Greg Kroah-Hartman Sept. 2, 2021, 10:39 a.m. UTC | #3

On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >
> > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > refcount of interface->dev.
> > >
> > > Fix this by removing the execution of usb_get_intf.
> >
> > Any idea about this patch?
> 
> +cc Dan Carpenter, gregkh
> 
> There is no reply in this thread in one month. Can someone give some
> feedback on this patch?

This is the media developers domain, not much I can do here.

thanks,

greg k-h

Hans Verkuil Sept. 2, 2021, 10:49 a.m. UTC | #4

Hi Dongliang,

On 02/09/2021 12:23, Dongliang Mu wrote:
> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>
>> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>>
>>> stk_camera_probe mistakenly execute usb_get_intf and increase the
>>> refcount of interface->dev.
>>>
>>> Fix this by removing the execution of usb_get_intf.
>>
>> Any idea about this patch?
> 
> +cc Dan Carpenter, gregkh
> 
> There is no reply in this thread in one month. Can someone give some
> feedback on this patch?

I saw that it was marked as Obsoleted in patchwork, but I might have confused
this patch with similar, but not identical, patches for this driver.

I've moved the state back to New.

Comments follow below:

> 
>>
>>>
>>> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
>>> Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
>>> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
>>> ---
>>>  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
>>> index a45d464427c4..5bd8e85b9452 100644
>>> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
>>> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
>>> @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
>>>
>>>         dev->udev = udev;
>>>         dev->interface = interface;
>>> -       usb_get_intf(interface);

Even though this increments the refcount (which might well be unnecessary),
it is also decremented with usb_put_intf. So is there actually a bug here?

And if this is changed, then I expect that both get_intf and put_intf should be
removed, and not just the get.

Regards,

	Hans

>>>
>>>         if (hflip != -1)
>>>                 dev->vsettings.hflip = hflip;
>>> --
>>> 2.25.1
>>>

Mauro Carvalho Chehab Sept. 2, 2021, 10:54 a.m. UTC | #5

Em Thu, 2 Sep 2021 12:39:37 +0200
Greg KH <gregkh@linuxfoundation.org> escreveu:

> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> > On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:  
> > >
> > > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:  
> > > >
> > > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > > refcount of interface->dev.
> > > >
> > > > Fix this by removing the execution of usb_get_intf.  
> > >
> > > Any idea about this patch?  
> > 
> > +cc Dan Carpenter, gregkh
> > 
> > There is no reply in this thread in one month. Can someone give some
> > feedback on this patch?  
> 
> This is the media developers domain, not much I can do here.

There is a high volume of patches for the media subsystem. Anyway,
as your patch is at our patchwork instance:

	https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/

It should be properly tracked, and likely handled after the end of
the merge window.

> > > > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>

If you're the author of the patch, it doesn't make much sense to
add a "Reported-by:" tag there. We only use it in order to give
someone's else credit to report an issue.

Thanks,
Mauro

Dongliang Mu Sept. 2, 2021, 10:59 a.m. UTC | #6

On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
>
> Em Thu, 2 Sep 2021 12:39:37 +0200
> Greg KH <gregkh@linuxfoundation.org> escreveu:
>
> > On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> > > On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > > >
> > > > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > > > >
> > > > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > > > refcount of interface->dev.
> > > > >
> > > > > Fix this by removing the execution of usb_get_intf.
> > > >
> > > > Any idea about this patch?
> > >
> > > +cc Dan Carpenter, gregkh
> > >
> > > There is no reply in this thread in one month. Can someone give some
> > > feedback on this patch?
> >
> > This is the media developers domain, not much I can do here.
>
> There is a high volume of patches for the media subsystem. Anyway,
> as your patch is at our patchwork instance:
>
>         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
>
> It should be properly tracked, and likely handled after the end of
> the merge window.
>
> > > > > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
>
> If you're the author of the patch, it doesn't make much sense to
> add a "Reported-by:" tag there. We only use it in order to give
> someone's else credit to report an issue.

I see. Someone told me this rule in another thread. I will update this
in the next version.

>
> Thanks,
> Mauro

Dongliang Mu Sept. 2, 2021, 11:06 a.m. UTC | #7

On Thu, Sep 2, 2021 at 6:49 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Dongliang,
>
> On 02/09/2021 12:23, Dongliang Mu wrote:
> > On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>
> >> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>>
> >>> stk_camera_probe mistakenly execute usb_get_intf and increase the
> >>> refcount of interface->dev.
> >>>
> >>> Fix this by removing the execution of usb_get_intf.
> >>
> >> Any idea about this patch?
> >
> > +cc Dan Carpenter, gregkh
> >
> > There is no reply in this thread in one month. Can someone give some
> > feedback on this patch?
>
> I saw that it was marked as Obsoleted in patchwork, but I might have confused
> this patch with similar, but not identical, patches for this driver.
>
> I've moved the state back to New.
>
> Comments follow below:
>
> >
> >>
> >>>
> >>> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> >>> Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> >>> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> >>> ---
> >>>  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> >>> index a45d464427c4..5bd8e85b9452 100644
> >>> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> >>> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> >>> @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
> >>>
> >>>         dev->udev = udev;
> >>>         dev->interface = interface;
> >>> -       usb_get_intf(interface);
>
> Even though this increments the refcount (which might well be unnecessary),
> it is also decremented with usb_put_intf. So is there actually a bug here?
>

Yes, if the increment and decrement of refcount are balanced, it's fine.

But this probe function only increases the refcount, but forgets to
decrease the refcount.

> And if this is changed, then I expect that both get_intf and put_intf should be
> removed, and not just the get.
>
> Regards,
>
>         Hans
>
> >>>
> >>>         if (hflip != -1)
> >>>                 dev->vsettings.hflip = hflip;
> >>> --
> >>> 2.25.1
> >>>
>

Dongliang Mu Sept. 2, 2021, 11:10 a.m. UTC | #8

On Thu, Sep 2, 2021 at 6:59 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> >
> > Em Thu, 2 Sep 2021 12:39:37 +0200
> > Greg KH <gregkh@linuxfoundation.org> escreveu:
> >
> > > On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> > > > On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > > > >
> > > > > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > > > > >
> > > > > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > > > > refcount of interface->dev.
> > > > > >
> > > > > > Fix this by removing the execution of usb_get_intf.
> > > > >
> > > > > Any idea about this patch?
> > > >
> > > > +cc Dan Carpenter, gregkh
> > > >
> > > > There is no reply in this thread in one month. Can someone give some
> > > > feedback on this patch?
> > >
> > > This is the media developers domain, not much I can do here.
> >
> > There is a high volume of patches for the media subsystem. Anyway,
> > as your patch is at our patchwork instance:
> >
> >         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
> >
> > It should be properly tracked, and likely handled after the end of
> > the merge window.

Hi Mauro,

I found there is another fix [1] for the same memory leak from Pavel
Skripkin (already cc-ed in this thread).

[1] https://www.spinics.net/lists/stable/msg479628.html

> >
> > > > > > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > > > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > If you're the author of the patch, it doesn't make much sense to
> > add a "Reported-by:" tag there. We only use it in order to give
> > someone's else credit to report an issue.
>
> I see. Someone told me this rule in another thread. I will update this
> in the next version.
>
> >
> > Thanks,
> > Mauro

Hans Verkuil Sept. 2, 2021, 11:15 a.m. UTC | #9

On 02/09/2021 13:10, Dongliang Mu wrote:
> On Thu, Sep 2, 2021 at 6:59 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>
>> On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
>>>
>>> Em Thu, 2 Sep 2021 12:39:37 +0200
>>> Greg KH <gregkh@linuxfoundation.org> escreveu:
>>>
>>>> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
>>>>> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>>>>>>>
>>>>>>> stk_camera_probe mistakenly execute usb_get_intf and increase the
>>>>>>> refcount of interface->dev.
>>>>>>>
>>>>>>> Fix this by removing the execution of usb_get_intf.
>>>>>>
>>>>>> Any idea about this patch?
>>>>>
>>>>> +cc Dan Carpenter, gregkh
>>>>>
>>>>> There is no reply in this thread in one month. Can someone give some
>>>>> feedback on this patch?
>>>>
>>>> This is the media developers domain, not much I can do here.
>>>
>>> There is a high volume of patches for the media subsystem. Anyway,
>>> as your patch is at our patchwork instance:
>>>
>>>         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
>>>
>>> It should be properly tracked, and likely handled after the end of
>>> the merge window.
> 
> Hi Mauro,
> 
> I found there is another fix [1] for the same memory leak from Pavel
> Skripkin (already cc-ed in this thread).
> 
> [1] https://www.spinics.net/lists/stable/msg479628.html

Ah, that's why I marked it as Obsoleted :-)

Regards,

	Hans

> 
>>>
>>>>>>> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
>>>>>>> Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
>>>>>>> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
>>>
>>> If you're the author of the patch, it doesn't make much sense to
>>> add a "Reported-by:" tag there. We only use it in order to give
>>> someone's else credit to report an issue.
>>
>> I see. Someone told me this rule in another thread. I will update this
>> in the next version.
>>
>>>
>>> Thanks,
>>> Mauro

Dongliang Mu Sept. 2, 2021, 11:22 a.m. UTC | #10

On Thu, Sep 2, 2021 at 7:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 02/09/2021 13:10, Dongliang Mu wrote:
> > On Thu, Sep 2, 2021 at 6:59 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>
> >> On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> >>>
> >>> Em Thu, 2 Sep 2021 12:39:37 +0200
> >>> Greg KH <gregkh@linuxfoundation.org> escreveu:
> >>>
> >>>> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> >>>>> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>>>>>
> >>>>>> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >>>>>>>
> >>>>>>> stk_camera_probe mistakenly execute usb_get_intf and increase the
> >>>>>>> refcount of interface->dev.
> >>>>>>>
> >>>>>>> Fix this by removing the execution of usb_get_intf.
> >>>>>>
> >>>>>> Any idea about this patch?
> >>>>>
> >>>>> +cc Dan Carpenter, gregkh
> >>>>>
> >>>>> There is no reply in this thread in one month. Can someone give some
> >>>>> feedback on this patch?
> >>>>
> >>>> This is the media developers domain, not much I can do here.
> >>>
> >>> There is a high volume of patches for the media subsystem. Anyway,
> >>> as your patch is at our patchwork instance:
> >>>
> >>>         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
> >>>
> >>> It should be properly tracked, and likely handled after the end of
> >>> the merge window.
> >
> > Hi Mauro,
> >
> > I found there is another fix [1] for the same memory leak from Pavel
> > Skripkin (already cc-ed in this thread).
> >
> > [1] https://www.spinics.net/lists/stable/msg479628.html
>
> Ah, that's why I marked it as Obsoleted :-)

Oh, I see. If that patch is already merged, please remark my patch as Obsoleted.

Curiously, I did not get an email notification to mark my patch as
Obsoleted before. Why?

>
> Regards,
>
>         Hans
>
> >
> >>>
> >>>>>>> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> >>>>>>> Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> >>>>>>> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> >>>
> >>> If you're the author of the patch, it doesn't make much sense to
> >>> add a "Reported-by:" tag there. We only use it in order to give
> >>> someone's else credit to report an issue.
> >>
> >> I see. Someone told me this rule in another thread. I will update this
> >> in the next version.
> >>
> >>>
> >>> Thanks,
> >>> Mauro
>

Dan Carpenter Sept. 2, 2021, 2:17 p.m. UTC | #11

On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >
> > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > refcount of interface->dev.
> > >
> > > Fix this by removing the execution of usb_get_intf.
> >
> > Any idea about this patch?
> 
> +cc Dan Carpenter, gregkh
> 
> There is no reply in this thread in one month. Can someone give some
> feedback on this patch?
> 
> >
> > >
> > > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > >  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > index a45d464427c4..5bd8e85b9452 100644
> > > --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> > > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
> > >
> > >         dev->udev = udev;
> > >         dev->interface = interface;
> > > -       usb_get_intf(interface);


The patch is wrong.  We're storing a reference to "interface".

	dev->interface = interface;

So we need to boost the refcount of interface.  Pavel Skripkin was on
the right patch but you need to add a:

	usb_put_intf(interface);

to the stk_camera_disconnect() function as you sort of mentioned.
That's the correct fix.

regards,
dan carpenter

Pavel Skripkin Sept. 2, 2021, 6:14 p.m. UTC | #12

On 9/2/21 14:22, Dongliang Mu wrote:
> On Thu, Sep 2, 2021 at 7:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 02/09/2021 13:10, Dongliang Mu wrote:
>> > On Thu, Sep 2, 2021 at 6:59 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>> >>
>> >> On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
>> >>>
>> >>> Em Thu, 2 Sep 2021 12:39:37 +0200
>> >>> Greg KH <gregkh@linuxfoundation.org> escreveu:
>> >>>
>> >>>> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
>> >>>>> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>> >>>>>>
>> >>>>>> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>> >>>>>>>
>> >>>>>>> stk_camera_probe mistakenly execute usb_get_intf and increase the
>> >>>>>>> refcount of interface->dev.
>> >>>>>>>
>> >>>>>>> Fix this by removing the execution of usb_get_intf.
>> >>>>>>
>> >>>>>> Any idea about this patch?
>> >>>>>
>> >>>>> +cc Dan Carpenter, gregkh
>> >>>>>
>> >>>>> There is no reply in this thread in one month. Can someone give some
>> >>>>> feedback on this patch?
>> >>>>
>> >>>> This is the media developers domain, not much I can do here.
>> >>>
>> >>> There is a high volume of patches for the media subsystem. Anyway,
>> >>> as your patch is at our patchwork instance:
>> >>>
>> >>>         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
>> >>>
>> >>> It should be properly tracked, and likely handled after the end of
>> >>> the merge window.
>> >
>> > Hi Mauro,
>> >
>> > I found there is another fix [1] for the same memory leak from Pavel
>> > Skripkin (already cc-ed in this thread).
>> >
>> > [1] https://www.spinics.net/lists/stable/msg479628.html
>>
>> Ah, that's why I marked it as Obsoleted :-)
> 
> Oh, I see. If that patch is already merged, please remark my patch as Obsoleted.
> 
> Curiously, I did not get an email notification to mark my patch as
> Obsoleted before. Why?
> 
>>

Hi, Dongliang!

Yep my patch has been merged already (1 month ago, I guess).




With regards,
Pavel Skripkin

Dongliang Mu Sept. 2, 2021, 11:45 p.m. UTC | #13

On Thu, Sep 2, 2021 at 10:18 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> > On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >
> > > On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > > >
> > > > stk_camera_probe mistakenly execute usb_get_intf and increase the
> > > > refcount of interface->dev.
> > > >
> > > > Fix this by removing the execution of usb_get_intf.
> > >
> > > Any idea about this patch?
> >
> > +cc Dan Carpenter, gregkh
> >
> > There is no reply in this thread in one month. Can someone give some
> > feedback on this patch?
> >
> > >
> > > >
> > > > Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > Fixes: 0aa77f6c2954 ("[media] move the remaining USB drivers to drivers/media/usb")
> > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > ---
> > > >  drivers/media/usb/stkwebcam/stk-webcam.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > index a45d464427c4..5bd8e85b9452 100644
> > > > --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > @@ -1311,7 +1311,6 @@ static int stk_camera_probe(struct usb_interface *interface,
> > > >
> > > >         dev->udev = udev;
> > > >         dev->interface = interface;
> > > > -       usb_get_intf(interface);
>
>
> The patch is wrong.  We're storing a reference to "interface".
>
>         dev->interface = interface;
>
> So we need to boost the refcount of interface.  Pavel Skripkin was on
> the right patch but you need to add a:
>
>         usb_put_intf(interface);
>
> to the stk_camera_disconnect() function as you sort of mentioned.
> That's the correct fix.

Thanks for your explanation, Dan. It's really helpful.

I sent the inquiry email in this thread because I did not receive the
notification of patchwork to mark my patch as obsolete and did not
notice Pavel had sent one patch before.

Now, this patch is marked as obsolete. Let's ignore it now.

>
> regards,
> dan carpenter

Dongliang Mu Sept. 2, 2021, 11:51 p.m. UTC | #14

On Fri, Sep 3, 2021 at 2:14 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 9/2/21 14:22, Dongliang Mu wrote:
> > On Thu, Sep 2, 2021 at 7:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 02/09/2021 13:10, Dongliang Mu wrote:
> >> > On Thu, Sep 2, 2021 at 6:59 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >> >>
> >> >> On Thu, Sep 2, 2021 at 6:54 PM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> >> >>>
> >> >>> Em Thu, 2 Sep 2021 12:39:37 +0200
> >> >>> Greg KH <gregkh@linuxfoundation.org> escreveu:
> >> >>>
> >> >>>> On Thu, Sep 02, 2021 at 06:23:36PM +0800, Dongliang Mu wrote:
> >> >>>>> On Fri, Jul 23, 2021 at 6:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >> >>>>>>
> >> >>>>>> On Wed, Jul 14, 2021 at 11:23 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >> >>>>>>>
> >> >>>>>>> stk_camera_probe mistakenly execute usb_get_intf and increase the
> >> >>>>>>> refcount of interface->dev.
> >> >>>>>>>
> >> >>>>>>> Fix this by removing the execution of usb_get_intf.
> >> >>>>>>
> >> >>>>>> Any idea about this patch?
> >> >>>>>
> >> >>>>> +cc Dan Carpenter, gregkh
> >> >>>>>
> >> >>>>> There is no reply in this thread in one month. Can someone give some
> >> >>>>> feedback on this patch?
> >> >>>>
> >> >>>> This is the media developers domain, not much I can do here.
> >> >>>
> >> >>> There is a high volume of patches for the media subsystem. Anyway,
> >> >>> as your patch is at our patchwork instance:
> >> >>>
> >> >>>         https://patchwork.linuxtv.org/project/linux-media/patch/20210714032340.504836-1-mudongliangabcd@gmail.com/
> >> >>>
> >> >>> It should be properly tracked, and likely handled after the end of
> >> >>> the merge window.
> >> >
> >> > Hi Mauro,
> >> >
> >> > I found there is another fix [1] for the same memory leak from Pavel
> >> > Skripkin (already cc-ed in this thread).
> >> >
> >> > [1] https://www.spinics.net/lists/stable/msg479628.html
> >>
> >> Ah, that's why I marked it as Obsoleted :-)
> >
> > Oh, I see. If that patch is already merged, please remark my patch as Obsoleted.
> >
> > Curiously, I did not get an email notification to mark my patch as
> > Obsoleted before. Why?
> >
> >>
>
> Hi, Dongliang!
>
> Yep my patch has been merged already (1 month ago, I guess).

Yes. When I submit this patch, your patch is still pending. I did not
know someone is already sending a patch with good quality.

So I am curious if there are any methods to notify people there are
already some similar patches in the patchwork or some other resources.

>
>
>
>
> With regards,
> Pavel Skripkin

diff mbox series

Patch

diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index a45d464427c4..5bd8e85b9452 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -1311,7 +1311,6 @@  static int stk_camera_probe(struct usb_interface *interface,
 
 	dev->udev = udev;
 	dev->interface = interface;
-	usb_get_intf(interface);
 
 	if (hflip != -1)
 		dev->vsettings.hflip = hflip;