[PATCH] USB: input: gtco.c: fix usb_dev leak

[PATCH] USB: input: gtco.c: fix usb_dev leak

[Alexey Khoroshilov]

There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
anywhere in the driver.

The patch adds usb_get_dev() to failure handling code of gtco_probe()
and to gtco_disconnect(().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/input/tablet/gtco.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index 29e01ab6859f..6ec8a3a04672 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -858,7 +858,7 @@ static int gtco_probe(struct usb_interface *usbinterface,
 	if (!gtco->buffer) {
 		dev_err(&usbinterface->dev, "No more memory for us buffers\n");
 		error = -ENOMEM;
-		goto err_free_devs;
+		goto err_put_usb;
 	}
 
 	/* Allocate URB for reports */
@@ -990,6 +990,8 @@ static int gtco_probe(struct usb_interface *usbinterface,
  err_free_buf:
 	usb_free_coherent(gtco->usbdev, REPORT_MAX_SIZE,
 			  gtco->buffer, gtco->buf_dma);
+ err_put_usb:
+	usb_put_dev(gtco->usbdev);
  err_free_devs:
 	input_free_device(input_dev);
 	kfree(gtco);
@@ -1013,6 +1015,7 @@ static void gtco_disconnect(struct usb_interface *interface)
 		usb_free_urb(gtco->urbinfo);
 		usb_free_coherent(gtco->usbdev, REPORT_MAX_SIZE,
 				  gtco->buffer, gtco->buf_dma);
+		usb_put_dev(gtco->usbdev);
 		kfree(gtco);
 	}
 
-- 
1.8.3.2

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Dmitry Torokhov]

On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote:
> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
> anywhere in the driver.
> 
> The patch adds usb_get_dev() to failure handling code of gtco_probe()
> and to gtco_disconnect(().

Hmm, I think gtco should simply not use usb_get_dev() in the first
place.

Thanks.

-- 
Dmitry

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Alexey Khoroshilov]

On 21.01.2014 23:59, Dmitry Torokhov wrote:
> On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote:
>> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
>> anywhere in the driver.
>>
>> The patch adds usb_get_dev() to failure handling code of gtco_probe()
>> and to gtco_disconnect(().
> Hmm, I think gtco should simply not use usb_get_dev() in the first
> place.
>
> Thanks.
Dear Dmitry,

Could you please clarify why usb_get_dev() not needed here?
We store reference to usb_dev in gtco structure, so we should refcount it.
What is wrong in this reasoning?

Thanks,
Alexey

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Dmitry Torokhov]

Hi Alexey,

On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote:
> On 21.01.2014 23:59, Dmitry Torokhov wrote:
> > On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote:
> >> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
> >> anywhere in the driver.
> >>
> >> The patch adds usb_get_dev() to failure handling code of gtco_probe()
> >> and to gtco_disconnect(().
> > Hmm, I think gtco should simply not use usb_get_dev() in the first
> > place.
> >
> > Thanks.
> Dear Dmitry,
> 
> Could you please clarify why usb_get_dev() not needed here?
> We store reference to usb_dev in gtco structure, so we should refcount it.
> What is wrong in this reasoning?

The lifetime of gtco structure is already directly tied to lifetime of
usb_dev: when destroying usb_dev driver core will call remove() function
of currently bound driver (in our case gtco) which will destroy gtco
memory.

Taking additional reference is not needed here.

Hope this helps.

-- 
Dmitry

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Alexey Khoroshilov]

On 27.01.2014 10:54, Dmitry Torokhov wrote:
> Hi Alexey,
>
> On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote:
>> On 21.01.2014 23:59, Dmitry Torokhov wrote:
>>> On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote:
>>>> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
>>>> anywhere in the driver.
>>>>
>>>> The patch adds usb_get_dev() to failure handling code of gtco_probe()
>>>> and to gtco_disconnect(().
>>> Hmm, I think gtco should simply not use usb_get_dev() in the first
>>> place.
>>>
>>> Thanks.
>> Dear Dmitry,
>>
>> Could you please clarify why usb_get_dev() not needed here?
>> We store reference to usb_dev in gtco structure, so we should refcount it.
>> What is wrong in this reasoning?
> The lifetime of gtco structure is already directly tied to lifetime of
> usb_dev: when destroying usb_dev driver core will call remove() function
> of currently bound driver (in our case gtco) which will destroy gtco
> memory.
>
> Taking additional reference is not needed here.
>
> Hope this helps.
Thank you, that helps a lot.

By the way, usb_skeleton suggests to use usb_get_dev()/usb_put_dev()
nevertheless.

Greg, may be it makes sense to fix usb_skeleton as well?

--
Alexey

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Greg Kroah-Hartman]

On Mon, Jan 27, 2014 at 02:29:23PM +0400, Alexey Khoroshilov wrote:
> On 27.01.2014 10:54, Dmitry Torokhov wrote:
> > Hi Alexey,
> >
> > On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote:
> >> On 21.01.2014 23:59, Dmitry Torokhov wrote:
> >>> On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote:
> >>>> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
> >>>> anywhere in the driver.
> >>>>
> >>>> The patch adds usb_get_dev() to failure handling code of gtco_probe()
> >>>> and to gtco_disconnect(().
> >>> Hmm, I think gtco should simply not use usb_get_dev() in the first
> >>> place.
> >>>
> >>> Thanks.
> >> Dear Dmitry,
> >>
> >> Could you please clarify why usb_get_dev() not needed here?
> >> We store reference to usb_dev in gtco structure, so we should refcount it.
> >> What is wrong in this reasoning?
> > The lifetime of gtco structure is already directly tied to lifetime of
> > usb_dev: when destroying usb_dev driver core will call remove() function
> > of currently bound driver (in our case gtco) which will destroy gtco
> > memory.
> >
> > Taking additional reference is not needed here.
> >
> > Hope this helps.
> Thank you, that helps a lot.
> 
> By the way, usb_skeleton suggests to use usb_get_dev()/usb_put_dev()
> nevertheless.
> 
> Greg, may be it makes sense to fix usb_skeleton as well?

Patches are always welcome.

[PATCH] USB: input: gtco.c: fix usb_dev leak

[Alexey Khoroshilov]

There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
anywhere in the driver.

As pointed out by Dmitry Torokhov:
The lifetime of gtco structure is already directly tied to lifetime of
usb_dev: when destroying usb_dev driver core will call remove() function
of currently bound driver (in our case gtco) which will destroy gtco
memory. Taking additional reference is not needed here.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/input/tablet/gtco.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index caecffe8caff..858045694e9d 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -848,7 +848,7 @@ static int gtco_probe(struct usb_interface *usbinterface,
 	gtco->inputdevice = input_dev;
 
 	/* Save interface information */
-	gtco->usbdev = usb_get_dev(interface_to_usbdev(usbinterface));
+	gtco->usbdev = interface_to_usbdev(usbinterface);
 	gtco->intf = usbinterface;
 
 	/* Allocate some data for incoming reports */
-- 
1.8.3.2

Re: [PATCH] USB: input: gtco.c: fix usb_dev leak

[Dmitry Torokhov]

On Tue, Jan 28, 2014 at 12:10:54AM +0400, Alexey Khoroshilov wrote:
> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev()
> anywhere in the driver.
> 
> As pointed out by Dmitry Torokhov:
> The lifetime of gtco structure is already directly tied to lifetime of
> usb_dev: when destroying usb_dev driver core will call remove() function
> of currently bound driver (in our case gtco) which will destroy gtco
> memory. Taking additional reference is not needed here.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

Applied, thank you.

> ---
>  drivers/input/tablet/gtco.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
> index caecffe8caff..858045694e9d 100644
> --- a/drivers/input/tablet/gtco.c
> +++ b/drivers/input/tablet/gtco.c
> @@ -848,7 +848,7 @@ static int gtco_probe(struct usb_interface *usbinterface,
>  	gtco->inputdevice = input_dev;
>  
>  	/* Save interface information */
> -	gtco->usbdev = usb_get_dev(interface_to_usbdev(usbinterface));
> +	gtco->usbdev = interface_to_usbdev(usbinterface);
>  	gtco->intf = usbinterface;
>  
>  	/* Allocate some data for incoming reports */
> -- 
> 1.8.3.2
> 

-- 
Dmitry