diff mbox

Input: pxrc - fix leak of usb_device

Message ID 1531512477-31981-1-git-send-email-khoroshilov@ispras.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Alexey Khoroshilov July 13, 2018, 8:07 p.m. UTC
pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
anywhere in the driver.

The patch adds one to error handling code and to pxrc_disconnect().

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

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/input/joystick/pxrc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Marcus Folkesson July 14, 2018, 8:09 a.m. UTC | #1
Hi Alexey,

Good catch!

On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> anywhere in the driver.
> 
> The patch adds one to error handling code and to pxrc_disconnect().
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>

> ---
>  drivers/input/joystick/pxrc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> index 07a0dbd3ced2..0a31de63ac8e 100644
> --- a/drivers/input/joystick/pxrc.c
> +++ b/drivers/input/joystick/pxrc.c
> @@ -221,6 +221,7 @@ static int pxrc_probe(struct usb_interface *intf,
>  	usb_free_urb(pxrc->urb);
>  
>  error:
> +	usb_put_dev(pxrc->udev);
>  	return retval;
>  }
>  
> @@ -229,6 +230,7 @@ static void pxrc_disconnect(struct usb_interface *intf)
>  	struct pxrc *pxrc = usb_get_intfdata(intf);
>  
>  	usb_free_urb(pxrc->urb);
> +	usb_put_dev(pxrc->udev);
>  	usb_set_intfdata(intf, NULL);
>  }
>  
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov July 14, 2018, 8:51 a.m. UTC | #2
On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
> Hi Alexey,
> 
> Good catch!
> 
> On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> > pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> > anywhere in the driver.
> > 
> > The patch adds one to error handling code and to pxrc_disconnect().
> > 
> > Found by Linux Driver Verification project (linuxtesting.org).
> > 
> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> 
> Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Hmm, the biggest question however if we need to "take" the device, as I
do not think interface can outlive the device, and whether we actually
need to store it in pxrc, as we only need it during set up, as far as I
can see.

Thanks.
Marcus Folkesson July 15, 2018, 7:42 a.m. UTC | #3
On Sat, Jul 14, 2018 at 08:51:09AM +0000, Dmitry Torokhov wrote:
> On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
> > Hi Alexey,
> > 
> > Good catch!
> > 
> > On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> > > pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> > > anywhere in the driver.
> > > 
> > > The patch adds one to error handling code and to pxrc_disconnect().
> > > 
> > > Found by Linux Driver Verification project (linuxtesting.org).
> > > 
> > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > 
> > Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> 
> Hmm, the biggest question however if we need to "take" the device, as I
> do not think interface can outlive the device, and whether we actually
> need to store it in pxrc, as we only need it during set up, as far as I
> can see.

Yep, the device is only used during setup.
I interpret the comments for usb_get_dev() as you should take a
reference count on the device even if you only use the interface, but I
could be wrong.

From usb_get_dev()::

	 * usb_get_dev - increments the reference count of the usb device structure
	 * @dev: the device being referenced
	 *
	 * Each live reference to a device should be refcounted.
	 *
	 * Drivers for USB interfaces should normally record such references in
	 * their probe() methods, when they bind to an interface, and release
	 * them by calling usb_put_dev(), in their disconnect() methods.

I can fix the driver to not take the device if that is what we want.
If not Alexey want to fix it of course, it is his catch :-)

> 
> Thanks.
> 
> -- 
> Dmitry

Best regards
Marcus Folkesson
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov July 15, 2018, 10:06 a.m. UTC | #4
On Sun, Jul 15, 2018 at 10:42 AM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> On Sat, Jul 14, 2018 at 08:51:09AM +0000, Dmitry Torokhov wrote:
> > On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
> > > Hi Alexey,
> > >
> > > Good catch!
> > >
> > > On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> > > > pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> > > > anywhere in the driver.
> > > >
> > > > The patch adds one to error handling code and to pxrc_disconnect().
> > > >
> > > > Found by Linux Driver Verification project (linuxtesting.org).
> > > >
> > > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > >
> > > Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> >
> > Hmm, the biggest question however if we need to "take" the device, as I
> > do not think interface can outlive the device, and whether we actually
> > need to store it in pxrc, as we only need it during set up, as far as I
> > can see.
>
> Yep, the device is only used during setup.
> I interpret the comments for usb_get_dev() as you should take a
> reference count on the device even if you only use the interface, but I
> could be wrong.
>
> From usb_get_dev()::
>
>          * usb_get_dev - increments the reference count of the usb device structure
>          * @dev: the device being referenced
>          *
>          * Each live reference to a device should be refcounted.
>          *
>          * Drivers for USB interfaces should normally record such references in
>          * their probe() methods, when they bind to an interface, and release
>          * them by calling usb_put_dev(), in their disconnect() methods.

Hmm, usb device is a parent of usb interface so our driver model rules
ensure that usb device should not disappear while interface device is
still there. Greg, is this comment still valid?

>
> I can fix the driver to not take the device if that is what we want.
> If not Alexey want to fix it of course, it is his catch :-)

Yeah, I'd prefer doing this if possible.

Thanks.
Greg Kroah-Hartman July 15, 2018, 10:12 a.m. UTC | #5
On Sun, Jul 15, 2018 at 01:06:32PM +0300, Dmitry Torokhov wrote:
> On Sun, Jul 15, 2018 at 10:42 AM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > On Sat, Jul 14, 2018 at 08:51:09AM +0000, Dmitry Torokhov wrote:
> > > On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
> > > > Hi Alexey,
> > > >
> > > > Good catch!
> > > >
> > > > On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> > > > > pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> > > > > anywhere in the driver.
> > > > >
> > > > > The patch adds one to error handling code and to pxrc_disconnect().
> > > > >
> > > > > Found by Linux Driver Verification project (linuxtesting.org).
> > > > >
> > > > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > > >
> > > > Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > >
> > > Hmm, the biggest question however if we need to "take" the device, as I
> > > do not think interface can outlive the device, and whether we actually
> > > need to store it in pxrc, as we only need it during set up, as far as I
> > > can see.
> >
> > Yep, the device is only used during setup.
> > I interpret the comments for usb_get_dev() as you should take a
> > reference count on the device even if you only use the interface, but I
> > could be wrong.
> >
> > From usb_get_dev()::
> >
> >          * usb_get_dev - increments the reference count of the usb device structure
> >          * @dev: the device being referenced
> >          *
> >          * Each live reference to a device should be refcounted.
> >          *
> >          * Drivers for USB interfaces should normally record such references in
> >          * their probe() methods, when they bind to an interface, and release
> >          * them by calling usb_put_dev(), in their disconnect() methods.
> 
> Hmm, usb device is a parent of usb interface so our driver model rules
> ensure that usb device should not disappear while interface device is
> still there. Greg, is this comment still valid?

Yes, that is true.  But remember that interface devices can go away
while the parent is still present, so if you need the interface pointer,
you have to grab a reference on it.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov July 15, 2018, 10:18 a.m. UTC | #6
On Sun, Jul 15, 2018 at 12:12:44PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 15, 2018 at 01:06:32PM +0300, Dmitry Torokhov wrote:
> > On Sun, Jul 15, 2018 at 10:42 AM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
> > >
> > > On Sat, Jul 14, 2018 at 08:51:09AM +0000, Dmitry Torokhov wrote:
> > > > On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
> > > > > Hi Alexey,
> > > > >
> > > > > Good catch!
> > > > >
> > > > > On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
> > > > > > pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
> > > > > > anywhere in the driver.
> > > > > >
> > > > > > The patch adds one to error handling code and to pxrc_disconnect().
> > > > > >
> > > > > > Found by Linux Driver Verification project (linuxtesting.org).
> > > > > >
> > > > > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > > > >
> > > > > Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > >
> > > > Hmm, the biggest question however if we need to "take" the device, as I
> > > > do not think interface can outlive the device, and whether we actually
> > > > need to store it in pxrc, as we only need it during set up, as far as I
> > > > can see.
> > >
> > > Yep, the device is only used during setup.
> > > I interpret the comments for usb_get_dev() as you should take a
> > > reference count on the device even if you only use the interface, but I
> > > could be wrong.
> > >
> > > From usb_get_dev()::
> > >
> > >          * usb_get_dev - increments the reference count of the usb device structure
> > >          * @dev: the device being referenced
> > >          *
> > >          * Each live reference to a device should be refcounted.
> > >          *
> > >          * Drivers for USB interfaces should normally record such references in
> > >          * their probe() methods, when they bind to an interface, and release
> > >          * them by calling usb_put_dev(), in their disconnect() methods.
> > 
> > Hmm, usb device is a parent of usb interface so our driver model rules
> > ensure that usb device should not disappear while interface device is
> > still there. Greg, is this comment still valid?
> 
> Yes, that is true.  But remember that interface devices can go away
> while the parent is still present, so if you need the interface pointer,
> you have to grab a reference on it.

But not in a simple interface driver, as we'll unbind the driver before
destroying the interface... IOW we need to record the reference only if
we are doing something unusual.

Thanks.
Alexey Khoroshilov July 15, 2018, 7:58 p.m. UTC | #7
Dear Marcus,

On 15.07.2018 10:42, Marcus Folkesson wrote:
> On Sat, Jul 14, 2018 at 08:51:09AM +0000, Dmitry Torokhov wrote:
>> On Sat, Jul 14, 2018 at 10:09:20AM +0200, Marcus Folkesson wrote:
>>> Hi Alexey,
>>>
>>> Good catch!
>>>
>>> On Fri, Jul 13, 2018 at 11:07:57PM +0300, Alexey Khoroshilov wrote:
>>>> pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev()
>>>> anywhere in the driver.
>>>>
>>>> The patch adds one to error handling code and to pxrc_isconnect().
>>>>
>>>> Found by Linux Driver Verification project (linuxtesting.org).
>>>>
>>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>>>
>>> Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>>
>> Hmm, the biggest question however if we need to "take" the device, as I
>> do not think interface can outlive the device, and whether we actually
>> need to store it in pxrc, as we only need it during set up, as far as I
>> can see.
> 
> Yep, the device is only used during setup.
> I interpret the comments for usb_get_dev() as you should take a
> reference count on the device even if you only use the interface, but I
> could be wrong.
> 
>>From usb_get_dev()::
> 
> 	 * usb_get_dev - increments the reference count of the usb device structure
> 	 * @dev: the device being referenced
> 	 *
> 	 * Each live reference to a device should be refcounted.
> 	 *
> 	 * Drivers for USB interfaces should normally record such references in
> 	 * their probe() methods, when they bind to an interface, and release
> 	 * them by calling usb_put_dev(), in their disconnect() methods.
> 
> I can fix the driver to not take the device if that is what we want.
> If not Alexey want to fix it of course, it is his catch :-)

As far as I can see the proposed solution requires some refactoring of
the init code. So, I believe the author is in the better position to do
that.

Best regards,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 07a0dbd3ced2..0a31de63ac8e 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -221,6 +221,7 @@  static int pxrc_probe(struct usb_interface *intf,
 	usb_free_urb(pxrc->urb);
 
 error:
+	usb_put_dev(pxrc->udev);
 	return retval;
 }
 
@@ -229,6 +230,7 @@  static void pxrc_disconnect(struct usb_interface *intf)
 	struct pxrc *pxrc = usb_get_intfdata(intf);
 
 	usb_free_urb(pxrc->urb);
+	usb_put_dev(pxrc->udev);
 	usb_set_intfdata(intf, NULL);
 }