[PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar <navinp@cdac.in>
---
 drivers/staging/usbip/stub_dev.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..447a98c 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -198,10 +198,8 @@ static void stub_shutdown_connection(struct usbip_device *ud)
 	 * tcp_socket is freed after threads are killed so that usbip_xmit does
 	 * not touch NULL socket.
 	 */
-	if (ud->tcp_socket) {
+	if (ud->tcp_socket)
 		sock_release(ud->tcp_socket);
-		ud->tcp_socket = NULL;
-	}
 
 	/* 3. free used data */
 	stub_device_cleanup_urbs(sdev);
@@ -233,6 +231,9 @@ static void stub_device_reset(struct usbip_device *ud)
 
 	dev_dbg(&udev->dev, "device reset");
 
+	ud->tcp_socket = NULL;
+	ud->tcp_rx = NULL;
+	ud->tcp_tx = NULL;
 	ret = usb_lock_device_for_reset(udev, sdev->interface);
 	if (ret < 0) {
 		dev_err(&udev->dev, "lock for reset\n");
-- 
1.7.9.5

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Dan Carpenter]

On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
> stub_device_reset should set kernel thread pointers to NULL.
> so that at the time of usbip_host removal stub_shoutdown_connection
> doesn't try to kill kernel threads which are already killed.
> 

If you have the Oops output, that's always nice to put in the commit
message.

Why don't you set the pointers to NULL in stub_shutdown_connection()
since that's where you actually kill the threads.  Setting them to
NULL in stub_device_reset() will (sometimes) solve the problem but
it gives you a new problem of a resource leak.

regards,
dan carpenter

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

On Tue, Sep 18, 2012 at 1:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
>> stub_device_reset should set kernel thread pointers to NULL.
>> so that at the time of usbip_host removal stub_shoutdown_connection
>> doesn't try to kill kernel threads which are already killed.
>>
>
> If you have the Oops output, that's always nice to put in the commit
> message.

i'll surely keep this in mind  before submitting further patches.

> Why don't you set the pointers to NULL in stub_shutdown_connection()
> since that's where you actually kill the threads.  Setting them to
> NULL in stub_device_reset() will (sometimes) solve the problem but
> it gives you a new problem of a resource leak.

 stub_device_reset() always gets executed after
stub_shutdown_connection() , never before.


>
> regards,
> dan carpenter
>

--navin-patidar

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Dan Carpenter]

On Tue, Sep 18, 2012 at 03:02:15PM +0530, navin patidar wrote:
> On Tue, Sep 18, 2012 at 1:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
> >> stub_device_reset should set kernel thread pointers to NULL.
> >> so that at the time of usbip_host removal stub_shoutdown_connection
> >> doesn't try to kill kernel threads which are already killed.
> >>
> >
> > If you have the Oops output, that's always nice to put in the commit
> > message.
> 
> i'll surely keep this in mind  before submitting further patches.
> 
> > Why don't you set the pointers to NULL in stub_shutdown_connection()
> > since that's where you actually kill the threads.  Setting them to
> > NULL in stub_device_reset() will (sometimes) solve the problem but
> > it gives you a new problem of a resource leak.
> 
>  stub_device_reset() always gets executed after
> stub_shutdown_connection() , never before.
> 

No it isn't.  Read event_handler() more carefully.  They can be
executed independently.

In other words, stub_shutdown_connection() can be called without
calling stub_device_reset() and stub_device_reset() can be called
without stub_shutdown_connection().  If either of those happen then
it causes a problem with the patch you have just sent.

regards,
dan carpenter

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

for usbip_host  event_handler()   handles following events.  defined
in "usbip_common.h"

 1. SDEV_EVENT_REMOVED   (USBIP_EH_SHUTDOWN | USBIP_EH_RESET | USBIP_EH_BYE)
 2. SDEV_EVENT_DOWN		(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 3. SDEV_EVENT_ERROR_TCP	(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 4. SDEV_EVENT_ERROR_SUBMIT	(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 5. VDEV_EVENT_ERROR_MALLOC (USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)

In case of events(1,2,3,4),  stub_shoutdown_connection() gets executed
first and than stub_device_reset() .

In case of event 5, stub_shoutdown_connection()  kills kernel threads
and stub_device_unusable()   changes devices status to
"SDEV_ST_ERROR"(fatal error).

 thus stub_device_reset() can't  be called without
stub_shutdown_connection(), so there is no problem of resource leak .
you are also right, i could have set pointers to  NULL in
stub_shutdown_connection() but i used  stub_device_reset() which is
intended to reset usbip_device stuct member variables.

i'll resend patches, if maintainer ask for that.
thanks

--navin-patidar


On Tue, Sep 18, 2012 at 3:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 18, 2012 at 03:02:15PM +0530, navin patidar wrote:
>> On Tue, Sep 18, 2012 at 1:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
>> >> stub_device_reset should set kernel thread pointers to NULL.
>> >> so that at the time of usbip_host removal stub_shoutdown_connection
>> >> doesn't try to kill kernel threads which are already killed.
>> >>
>> >
>> > If you have the Oops output, that's always nice to put in the commit
>> > message.
>>
>> i'll surely keep this in mind  before submitting further patches.
>>
>> > Why don't you set the pointers to NULL in stub_shutdown_connection()
>> > since that's where you actually kill the threads.  Setting them to
>> > NULL in stub_device_reset() will (sometimes) solve the problem but
>> > it gives you a new problem of a resource leak.
>>
>>  stub_device_reset() always gets executed after
>> stub_shutdown_connection() , never before.
>>
>
> No it isn't.  Read event_handler() more carefully.  They can be
> executed independently.
>
> In other words, stub_shutdown_connection() can be called without
> calling stub_device_reset() and stub_device_reset() can be called
> without stub_shutdown_connection().  If either of those happen then
> it causes a problem with the patch you have just sent.
>
> regards,
> dan carpenter
>

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Dan Carpenter]

On Tue, Sep 18, 2012 at 05:14:41PM +0530, navin patidar wrote:
> for usbip_host  event_handler()   handles following events.  defined
> in "usbip_common.h"
> 
>  1. SDEV_EVENT_REMOVED   (USBIP_EH_SHUTDOWN | USBIP_EH_RESET | USBIP_EH_BYE)
>  2. SDEV_EVENT_DOWN		(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
>  3. SDEV_EVENT_ERROR_TCP	(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
>  4. SDEV_EVENT_ERROR_SUBMIT	(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
>  5. VDEV_EVENT_ERROR_MALLOC (USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)
> 
> In case of events(1,2,3,4),  stub_shoutdown_connection() gets executed
> first and than stub_device_reset() .
> 
> In case of event 5, stub_shoutdown_connection()  kills kernel threads
> and stub_device_unusable()   changes devices status to
> "SDEV_ST_ERROR"(fatal error).
> 

It's case #5 which I would be worried about.  Where did the original
Oops happen?  I feel like it really would be helpful to see it.  I
don't see which check for ->status != SDEV_ST_AVAILABLE you're
talking about here which prevents the pointers from being reused...

>  thus stub_device_reset() can't  be called without
> stub_shutdown_connection(), so there is no problem of resource leak .

Except in the case of #5 obviously.

> you are also right, i could have set pointers to  NULL in
> stub_shutdown_connection() but i used  stub_device_reset() which is
> intended to reset usbip_device stuct member variables.
> 
> i'll resend patches, if maintainer ask for that.
> thanks
> 

Generally, that's normal.  If you want to ensure that a pointer
isn't used again then you clear it immediately.

I'm honestly just trying to figure this out.  When I saw that the
patch, I immediately thought *resource leak*.  I'm sorry that to
take your time up, but it shouldn't be that complicated that I have
to go tracking through the whole driver to understand this.

regards,
dan carpenter

[PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar <navinp@cdac.in>
---
 drivers/staging/usbip/stub_dev.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..447a98c 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -198,10 +198,8 @@ static void stub_shutdown_connection(struct usbip_device *ud)
 	 * tcp_socket is freed after threads are killed so that usbip_xmit does
 	 * not touch NULL socket.
 	 */
-	if (ud->tcp_socket) {
+	if (ud->tcp_socket)
 		sock_release(ud->tcp_socket);
-		ud->tcp_socket = NULL;
-	}
 
 	/* 3. free used data */
 	stub_device_cleanup_urbs(sdev);
@@ -233,6 +231,9 @@ static void stub_device_reset(struct usbip_device *ud)
 
 	dev_dbg(&udev->dev, "device reset");
 
+	ud->tcp_socket = NULL;
+	ud->tcp_rx = NULL;
+	ud->tcp_tx = NULL;
 	ret = usb_lock_device_for_reset(udev, sdev->interface);
 	if (ret < 0) {
 		dev_err(&udev->dev, "lock for reset\n");
-- 
1.7.9.5


-------------------------------------------------------------------------------------------------------------------------------

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
-------------------------------------------------------------------------------------------------------------------------------

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Dan Carpenter]

On Fri, Sep 14, 2012 at 06:51:33PM +0530, navin patidar wrote:
> stub_device_reset should set kernel thread pointers to NULL.
> so that at the time of usbip_host removal stub_shoutdown_connection
> doesn't try to kill kernel threads which are already killed.
> 

If you wanted to put a sample Oops stack trace in here, that would
always be welcome.  It's not something to resend over.  Just an
idea.

> -------------------------------------------------------------------------------------------------------------------------------
> 
> This e-mail is for the sole use of the intended recipient(s) and may
> contain confidential and privileged information. If you are not the
> intended recipient, please contact the sender by reply e-mail and destroy
> all copies and the original message. Any unauthorized review, use,
> disclosure, dissemination, forwarding, printing or copying of this email
> is strictly prohibited and appropriate legal action will be taken.
> -------------------------------------------------------------------------------------------------------------------------------

I don't think this text is allowed on patches.  This is a thing
where you probably will need to resend the patch.

regards,
dan carpenter

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Sebastian Andrzej Siewior]

On Fri, Sep 14, 2012 at 05:16:38PM +0300, Dan Carpenter wrote:
> I don't think this text is allowed on patches.

Oh Oh Oh Dan, I think you crossed a line here. You were not one of the
recipient and you reviewed and forwarded the email but you should have
deleted this email instead. Now legal action will be taken…

> regards,
> dan carpenter

Sebastian

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Dan Carpenter]

On Fri, Sep 14, 2012 at 04:28:33PM +0200, Sebastian Andrzej Siewior wrote:
> On Fri, Sep 14, 2012 at 05:16:38PM +0300, Dan Carpenter wrote:
> > I don't think this text is allowed on patches.
> 
> Oh Oh Oh Dan, I think you crossed a line here. You were not one of the
> recipient and you reviewed and forwarded the email but you should have
> deleted this email instead. Now legal action will be taken…
> 

Oh crap!

Well, it's a good thing I am a fully AWCP certified survivalist.
I'm off to live in the mountains until the statute of limitations
runs out.  If you need me, then print your email out and leave it
under a rock.

regards,
dan carpenter

[PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar <navinp@cdac.in>
---
 drivers/staging/usbip/stub_dev.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..f584af8 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -192,16 +192,13 @@ static void stub_shutdown_connection(struct usbip_device *ud)
 	if (ud->tcp_tx)
 		kthread_stop_put(ud->tcp_tx);
 
-	/*
-	 * 2. close the socket
+	/* 2. close the socket
 	 *
 	 * tcp_socket is freed after threads are killed so that usbip_xmit does
 	 * not touch NULL socket.
 	 */
-	if (ud->tcp_socket) {
+	if (ud->tcp_socket)
 		sock_release(ud->tcp_socket);
-		ud->tcp_socket = NULL;
-	}
 
 	/* 3. free used data */
 	stub_device_cleanup_urbs(sdev);
@@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device *ud)
 
 	dev_dbg(&udev->dev, "device reset");
 
+	/*reset tcp socket*/
+	ud->tcp_socket = NULL;
+
+	/*reset kernel thread pointers */
+	ud->tcp_rx = NULL;
+	ud->tcp_tx = NULL;
+
 	ret = usb_lock_device_for_reset(udev, sdev->interface);
 	if (ret < 0) {
 		dev_err(&udev->dev, "lock for reset\n");
-- 
1.7.9.5


-------------------------------------------------------------------------------------------------------------------------------

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
-------------------------------------------------------------------------------------------------------------------------------

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Sergei Shtylyov]

Hello.

On 14-09-2012 13:53, navin patidar wrote:

> stub_device_reset should set kernel thread pointers to NULL.
> so that at the time of usbip_host removal stub_shoutdown_connection
> doesn't try to kill kernel threads which are already killed.

> Signed-off-by: navin patidar <navinp@cdac.in>
> ---
>   drivers/staging/usbip/stub_dev.c |   14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)

> diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
> index 92ced35..f584af8 100644
> --- a/drivers/staging/usbip/stub_dev.c
> +++ b/drivers/staging/usbip/stub_dev.c
> @@ -192,16 +192,13 @@ static void stub_shutdown_connection(struct usbip_device *ud)
>   	if (ud->tcp_tx)
>   		kthread_stop_put(ud->tcp_tx);
>
> -	/*
> -	 * 2. close the socket
> +	/* 2. close the socket

    It's the preferred comment style -- why modify it?

>   	 *
>   	 * tcp_socket is freed after threads are killed so that usbip_xmit does
>   	 * not touch NULL socket.
>   	 */
> -	if (ud->tcp_socket) {
> +	if (ud->tcp_socket)
>   		sock_release(ud->tcp_socket);
> -		ud->tcp_socket = NULL;
> -	}
>
>   	/* 3. free used data */
>   	stub_device_cleanup_urbs(sdev);
> @@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device *ud)
>
>   	dev_dbg(&udev->dev, "device reset");
>
> +	/*reset tcp socket*/

    Add spaces after /* and before */, please.

> +	ud->tcp_socket = NULL;
> +
> +	/*reset kernel thread pointers */

    Here too.

WBR, Sergei

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Sebastian Andrzej Siewior]

On Fri, Sep 14, 2012 at 03:42:42PM +0400, Sergei Shtylyov wrote:
> >diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
> >index 92ced35..f584af8 100644
> >--- a/drivers/staging/usbip/stub_dev.c
> >+++ b/drivers/staging/usbip/stub_dev.c
> >@@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device *ud)
> >
> >  	dev_dbg(&udev->dev, "device reset");
> >
> >+	/*reset tcp socket*/
> 
>    Add spaces after /* and before */, please.
> 
> >+	ud->tcp_socket = NULL;
> >+
> >+	/*reset kernel thread pointers */
> 
>    Here too.

I'm not sure it is required to comment the obvious things.

> WBR, Sergei

Sebastian

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

hi,

I have sent this patch again with corrections.
thank for reviewing the patch.

--navin-patidar

On 9/14/12, Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
> On Fri, Sep 14, 2012 at 03:42:42PM +0400, Sergei Shtylyov wrote:
>> >diff --git a/drivers/staging/usbip/stub_dev.c
>> > b/drivers/staging/usbip/stub_dev.c
>> >index 92ced35..f584af8 100644
>> >--- a/drivers/staging/usbip/stub_dev.c
>> >+++ b/drivers/staging/usbip/stub_dev.c
>> >@@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device
>> > *ud)
>> >
>> >  	dev_dbg(&udev->dev, "device reset");
>> >
>> >+	/*reset tcp socket*/
>>
>>    Add spaces after /* and before */, please.
>>
>> >+	ud->tcp_socket = NULL;
>> >+
>> >+	/*reset kernel thread pointers */
>>
>>    Here too.
>
> I'm not sure it is required to comment the obvious things.
>
>> WBR, Sergei
>
> Sebastian
>

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Sergei Shtylyov]

Hello.

On 09/14/2012 04:36 PM, navin patidar wrote:

> hi Sergei,
> checkpatch.pl <http://checkpatch.pl> didn't complain any thing about patch
> coding style.

   checkpatch.pl only complains about // comments, IIRC. But see
Documentation/CodingStyle chapter 8 about multi-line comments. As for adding
spaces, it's more for aesthetic reasons...

> Anyway  thanks for reviewing the patch.
> i will send this patch again with corrections.

  Thanks you.

> --navin-patidar

WBR, Sergei

Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[Greg KH]

On Fri, Sep 14, 2012 at 03:23:23PM +0530, navin patidar wrote:
> -------------------------------------------------------------------------------------------------------------------------------
> 
> This e-mail is for the sole use of the intended recipient(s) and may
> contain confidential and privileged information. If you are not the
> intended recipient, please contact the sender by reply e-mail and destroy
> all copies and the original message. Any unauthorized review, use,
> disclosure, dissemination, forwarding, printing or copying of this email
> is strictly prohibited and appropriate legal action will be taken.
> -------------------------------------------------------------------------------------------------------------------------------

As others have pointed out, I can not accept patches sent with this type
of wording in them, sorry.  Please fix your email system and try again.

greg k-h