* Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2016-11-18 21:58 ` Iago Abal
0 siblings, 0 replies; 6+ messages in thread
From: Iago Abal @ 2016-11-18 21:58 UTC (permalink / raw)
To: Kalle Valo; +Cc: Solomon Peachy, linux-wireless, netdev
Hi,
With the help of a static bug finder (EBA -
https://github.com/models-team/eba) I have found a potential deadlock
in drivers/net/wireless/st/cw1200/
sta.c. This happens due to a recursive mutex_lock on `priv->conf_mutex'.
If this is indeed a bug, I will be happy to help with a patch.
A quick (not elegant) fix could be to unlock before the call to
`cw1200_do_unjoin' in line 1174, and lock again afterwards. It seems
that `cw1200_join_complete' is always called with `priv->conf_mutex'
held. Another option could be to add a Boolean parameter to
`cw1200_do_unjoin' to choose whether this function should take the
lock itself. Yet another option would be to have a
`__cw1200_do_unjoin' that does not lock, and make `cw1200_do_unjoin' a
wrapper over this that adds the locking; `cw1200_join_complete' would
call `__cw1200_do_unjoin' instead.
Someone who is actually familiar with this code may have a better
proposal though.
The trace is as follows:
1. Function `cw1200_join_complete_work' takes the first lock in line 1189:
// see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1189
mutex_lock(& priv->conf_mutex);
2. and subsequently calls `cw1200_join_complete';
3. which calls `cw1200_do_unjoin' in line 1174;
4. and this latter function takes the lock for the second time in line 1387:
// see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1387
mutex_lock(& priv->conf_mutex);
Hope it helps!
--
iago
^ permalink raw reply [flat|nested] 6+ messages in thread* Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2016-11-18 21:58 ` Iago Abal
0 siblings, 0 replies; 6+ messages in thread
From: Iago Abal @ 2016-11-18 21:58 UTC (permalink / raw)
To: Kalle Valo
Cc: Solomon Peachy, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
Hi,
With the help of a static bug finder (EBA -
https://github.com/models-team/eba) I have found a potential deadlock
in drivers/net/wireless/st/cw1200/
sta.c. This happens due to a recursive mutex_lock on `priv->conf_mutex'.
If this is indeed a bug, I will be happy to help with a patch.
A quick (not elegant) fix could be to unlock before the call to
`cw1200_do_unjoin' in line 1174, and lock again afterwards. It seems
that `cw1200_join_complete' is always called with `priv->conf_mutex'
held. Another option could be to add a Boolean parameter to
`cw1200_do_unjoin' to choose whether this function should take the
lock itself. Yet another option would be to have a
`__cw1200_do_unjoin' that does not lock, and make `cw1200_do_unjoin' a
wrapper over this that adds the locking; `cw1200_join_complete' would
call `__cw1200_do_unjoin' instead.
Someone who is actually familiar with this code may have a better
proposal though.
The trace is as follows:
1. Function `cw1200_join_complete_work' takes the first lock in line 1189:
// see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1189
mutex_lock(& priv->conf_mutex);
2. and subsequently calls `cw1200_join_complete';
3. which calls `cw1200_do_unjoin' in line 1174;
4. and this latter function takes the lock for the second time in line 1387:
// see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1387
mutex_lock(& priv->conf_mutex);
Hope it helps!
--
iago
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2018-02-06 20:04 ` Iago Abal
0 siblings, 0 replies; 6+ messages in thread
From: Iago Abal @ 2018-02-06 20:04 UTC (permalink / raw)
To: Johannes Berg; +Cc: Kalle Valo, Solomon Peachy, linux-wireless, netdev
Hi,
This still looks like a deadlock bug to me, could someone take a look
as well and confirm? I will help preparing a patch if needed.
Thanks,
-- iago
On Fri, Nov 18, 2016 at 10:58 PM, Iago Abal <iago.abal@gmail.com> wrote:
> Hi,
>
> With the help of a static bug finder (EBA -
> https://github.com/models-team/eba) I have found a potential deadlock
> in drivers/net/wireless/st/cw1200/
> sta.c. This happens due to a recursive mutex_lock on `priv->conf_mutex'.
>
> If this is indeed a bug, I will be happy to help with a patch.
>
> A quick (not elegant) fix could be to unlock before the call to
> `cw1200_do_unjoin' in line 1174, and lock again afterwards. It seems
> that `cw1200_join_complete' is always called with `priv->conf_mutex'
> held. Another option could be to add a Boolean parameter to
> `cw1200_do_unjoin' to choose whether this function should take the
> lock itself. Yet another option would be to have a
> `__cw1200_do_unjoin' that does not lock, and make `cw1200_do_unjoin' a
> wrapper over this that adds the locking; `cw1200_join_complete' would
> call `__cw1200_do_unjoin' instead.
>
> Someone who is actually familiar with this code may have a better
> proposal though.
>
> The trace is as follows:
>
> 1. Function `cw1200_join_complete_work' takes the first lock in line 1189:
>
> // see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1189
> mutex_lock(& priv->conf_mutex);
>
> 2. and subsequently calls `cw1200_join_complete';
> 3. which calls `cw1200_do_unjoin' in line 1174;
> 4. and this latter function takes the lock for the second time in line 1387:
>
> // see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1387
> mutex_lock(& priv->conf_mutex);
>
> Hope it helps!
>
> --
> iago
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2018-02-06 20:04 ` Iago Abal
0 siblings, 0 replies; 6+ messages in thread
From: Iago Abal @ 2018-02-06 20:04 UTC (permalink / raw)
To: Johannes Berg
Cc: Kalle Valo, Solomon Peachy,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
Hi,
This still looks like a deadlock bug to me, could someone take a look
as well and confirm? I will help preparing a patch if needed.
Thanks,
-- iago
On Fri, Nov 18, 2016 at 10:58 PM, Iago Abal <iago.abal-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi,
>
> With the help of a static bug finder (EBA -
> https://github.com/models-team/eba) I have found a potential deadlock
> in drivers/net/wireless/st/cw1200/
> sta.c. This happens due to a recursive mutex_lock on `priv->conf_mutex'.
>
> If this is indeed a bug, I will be happy to help with a patch.
>
> A quick (not elegant) fix could be to unlock before the call to
> `cw1200_do_unjoin' in line 1174, and lock again afterwards. It seems
> that `cw1200_join_complete' is always called with `priv->conf_mutex'
> held. Another option could be to add a Boolean parameter to
> `cw1200_do_unjoin' to choose whether this function should take the
> lock itself. Yet another option would be to have a
> `__cw1200_do_unjoin' that does not lock, and make `cw1200_do_unjoin' a
> wrapper over this that adds the locking; `cw1200_join_complete' would
> call `__cw1200_do_unjoin' instead.
>
> Someone who is actually familiar with this code may have a better
> proposal though.
>
> The trace is as follows:
>
> 1. Function `cw1200_join_complete_work' takes the first lock in line 1189:
>
> // see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1189
> mutex_lock(& priv->conf_mutex);
>
> 2. and subsequently calls `cw1200_join_complete';
> 3. which calls `cw1200_do_unjoin' in line 1174;
> 4. and this latter function takes the lock for the second time in line 1387:
>
> // see https://github.com/torvalds/linux/blob/v4.9-rc5/drivers/net/wireless/st/cw1200/sta.c#L1387
> mutex_lock(& priv->conf_mutex);
>
> Hope it helps!
>
> --
> iago
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2018-02-15 19:05 ` Solomon Peachy
0 siblings, 0 replies; 6+ messages in thread
From: Solomon Peachy @ 2018-02-15 19:05 UTC (permalink / raw)
To: Iago Abal; +Cc: Johannes Berg, Kalle Valo, linux-wireless, netdev
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
On Tue, Feb 06, 2018 at 09:04:29PM +0100, Iago Abal wrote:
> This still looks like a deadlock bug to me, could someone take a look
> as well and confirm? I will help preparing a patch if needed.
The short answer: yes, it deadlocks.
I'll be glad to review patches (this code is pretty tricky, but I
believe the deadlock got introduced/exposed by different code changes)
but unfortunately my last move killed the hardware capable of
accepting my SDIO CW1200 dongles.
Hmm, might be worth trolling ebay or something.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Coconut Creek, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum videtur.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9)
@ 2018-02-15 19:05 ` Solomon Peachy
0 siblings, 0 replies; 6+ messages in thread
From: Solomon Peachy @ 2018-02-15 19:05 UTC (permalink / raw)
To: Iago Abal
Cc: Johannes Berg, Kalle Valo, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
On Tue, Feb 06, 2018 at 09:04:29PM +0100, Iago Abal wrote:
> This still looks like a deadlock bug to me, could someone take a look
> as well and confirm? I will help preparing a patch if needed.
The short answer: yes, it deadlocks.
I'll be glad to review patches (this code is pretty tricky, but I
believe the deadlock got introduced/exposed by different code changes)
but unfortunately my last move killed the hardware capable of
accepting my SDIO CW1200 dongles.
Hmm, might be worth trolling ebay or something.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Coconut Creek, FL ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum videtur.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-15 20:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-18 21:58 Potential deadlock BUG in drivers/net/wireless/st/cw1200/sta.c (Linux 4.9) Iago Abal
2016-11-18 21:58 ` Iago Abal
2018-02-06 20:04 ` Iago Abal
2018-02-06 20:04 ` Iago Abal
2018-02-15 19:05 ` Solomon Peachy
2018-02-15 19:05 ` Solomon Peachy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.