Re: [PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock

[Pete Zaitcev]

On Mon,  2 Nov 2015 10:27:00 +0100
Jiri Slaby  wrote:

> Signed-off-by: Jiri Slaby 
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int 
> nonblock)
>  
>   add_wait_queue(>wwait, );
>   for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
>   if (mutex_lock_interruptible(>mut)) {
>   rc = -EINTR;
>   break;
>   }
> + set_current_state(TASK_INTERRUPTIBLE);
>   rc = usblp_wtest(usblp, nonblock);
>   mutex_unlock(>mut);
>   if (rc <= 0)

I'm fully onboard with this. In the original "big cleanup" 317c67b8f,
I got this right. But then I either missed that mutex_lock_interruptible()
can set the state, or it didn't do it back then (it was in 2007), and
the 7f477358e introduced the existing code.

Acked-By: Pete Zaitcev 

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock

[Pete Zaitcev]

On Mon,  2 Nov 2015 10:27:00 +0100
Jiri Slaby  wrote:

> Signed-off-by: Jiri Slaby 
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int 
> nonblock)
>  
>   add_wait_queue(>wwait, );
>   for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
>   if (mutex_lock_interruptible(>mut)) {
>   rc = -EINTR;
>   break;
>   }
> + set_current_state(TASK_INTERRUPTIBLE);
>   rc = usblp_wtest(usblp, nonblock);
>   mutex_unlock(>mut);
>   if (rc <= 0)

I'm fully onboard with this. In the original "big cleanup" 317c67b8f,
I got this right. But then I either missed that mutex_lock_interruptible()
can set the state, or it didn't do it back then (it was in 2007), and
the 7f477358e introduced the existing code.

Acked-By: Pete Zaitcev 

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock

[Jiri Slaby]

It is not permitted to set task state before lock. usblp_wwait sets
the state to TASK_INTERRUPTIBLE and calls mutex_lock_interruptible.
Upon return from that function, the state will be TASK_RUNNING again.

This is clearly a bug and a warning is generated with LOCKDEP too:
WARNING: CPU: 1 PID: 5109 at kernel/sched/core.c:7404 __might_sleep+0x7d/0x90()
do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] usblp_wwait+0xa0/0x310 [usblp]
Modules linked in: ...
CPU: 1 PID: 5109 Comm: captmon Tainted: GW   
4.2.5-0.gef2823b-default #1
Hardware name: LENOVO 23252SG/23252SG, BIOS G2ET33WW (1.13 ) 07/24/2012
 81a4edce 880236ec7ba8 81716651 
 880236ec7bf8 880236ec7be8 8106e146 0282
 81a50119 028b  8802dab7c508
Call Trace:
...
 [] warn_slowpath_fmt+0x46/0x50
 [] __might_sleep+0x7d/0x90
 [] mutex_lock_interruptible_nested+0x2f/0x4b0
 [] usblp_wwait+0xcc/0x310 [usblp]
 [] usblp_write+0x72/0x350 [usblp]
 [] __vfs_write+0x28/0xf0
...

Commit 7f477358e2384c54b190cc3b6ce28277050a041b (usblp: Implement the
ENOSPC convention) moved the set prior locking. So move it back after
the lock.

Signed-off-by: Jiri Slaby 
Cc: Pete Zaitcev 
Cc: Greg Kroah-Hartman 
Fixes: 7f477358e2 ("usblp: Implement the ENOSPC convention")
---
 drivers/usb/class/usblp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 433bbc34a8a4..071964c7847f 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int nonblock)
 
add_wait_queue(>wwait, );
for (;;) {
-   set_current_state(TASK_INTERRUPTIBLE);
if (mutex_lock_interruptible(>mut)) {
rc = -EINTR;
break;
}
+   set_current_state(TASK_INTERRUPTIBLE);
rc = usblp_wtest(usblp, nonblock);
mutex_unlock(>mut);
if (rc <= 0)
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock

[Jiri Slaby]

It is not permitted to set task state before lock. usblp_wwait sets
the state to TASK_INTERRUPTIBLE and calls mutex_lock_interruptible.
Upon return from that function, the state will be TASK_RUNNING again.

This is clearly a bug and a warning is generated with LOCKDEP too:
WARNING: CPU: 1 PID: 5109 at kernel/sched/core.c:7404 __might_sleep+0x7d/0x90()
do not call blocking ops when !TASK_RUNNING; state=1 set at 
[] usblp_wwait+0xa0/0x310 [usblp]
Modules linked in: ...
CPU: 1 PID: 5109 Comm: captmon Tainted: GW   
4.2.5-0.gef2823b-default #1
Hardware name: LENOVO 23252SG/23252SG, BIOS G2ET33WW (1.13 ) 07/24/2012
 81a4edce 880236ec7ba8 81716651 
 880236ec7bf8 880236ec7be8 8106e146 0282
 81a50119 028b  8802dab7c508
Call Trace:
...
 [] warn_slowpath_fmt+0x46/0x50
 [] __might_sleep+0x7d/0x90
 [] mutex_lock_interruptible_nested+0x2f/0x4b0
 [] usblp_wwait+0xcc/0x310 [usblp]
 [] usblp_write+0x72/0x350 [usblp]
 [] __vfs_write+0x28/0xf0
...

Commit 7f477358e2384c54b190cc3b6ce28277050a041b (usblp: Implement the
ENOSPC convention) moved the set prior locking. So move it back after
the lock.

Signed-off-by: Jiri Slaby 
Cc: Pete Zaitcev 
Cc: Greg Kroah-Hartman 
Fixes: 7f477358e2 ("usblp: Implement the ENOSPC convention")
---
 drivers/usb/class/usblp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 433bbc34a8a4..071964c7847f 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int nonblock)
 
add_wait_queue(>wwait, );
for (;;) {
-   set_current_state(TASK_INTERRUPTIBLE);
if (mutex_lock_interruptible(>mut)) {
rc = -EINTR;
break;
}
+   set_current_state(TASK_INTERRUPTIBLE);
rc = usblp_wtest(usblp, nonblock);
mutex_unlock(>mut);
if (rc <= 0)
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/