[PATCH] rfcomm deadlock fix

[PATCH] rfcomm deadlock fix

[Dave Young]

There's logic in __rfcomm_dlc_close:
	rfcomm_dlc_lock(d);
	d->state = BT_CLOSED;
	d->state_changed(d, err);
	rfcomm_dlc_unlock(d);

In rfcomm_dev_state_change, it's possible that rfcomm_dev_put try to take the
dlc lock, then we will deadlock.

Here fixed it by unlock dlc before rfcomm_dev_get in rfcomm_dev_state_change.

why not unlock just before rfcomm_dev_put? it's because there's another problem.
rfcomm_dev_get/rfcomm_dev_del will take rfcomm_dev_lock, but in rfcomm_dev_add
the lock order is : rfcomm_dev_lock --> dlc lock

so I unlock dlc before the taken of rfcomm_dev_lock.
 
Actually it's a regression caused by commit
1905f6c736cb618e07eca0c96e60e3c024023428, the dlc state_change could be two
callbacks : rfcomm_sk_state_change and rfcomm_dev_state_change. I missed the rfcomm_sk_state_change that time.

Thanks Arjan van de Ven <arjan@linux.intel.com> for the effort in commit
4c8411f8c115def968820a4df6658ccfd55d7f1a
but he missed the rfcomm_dev_state_change lock issue.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>

---
net/bluetooth/rfcomm/tty.c  |    9 +++++++++
1 files changed, 9 insertions(+)

diff -upr linux/net/bluetooth/rfcomm/tty.c linux.new/net/bluetooth/rfcomm/tty.c
--- linux/net/bluetooth/rfcomm/tty.c	2008-05-30 15:46:33.000000000 +0800
+++ linux.new/net/bluetooth/rfcomm/tty.c	2008-05-30 16:02:38.000000000 +0800
@@ -566,11 +566,20 @@ static void rfcomm_dev_state_change(stru
 	if (dlc->state == BT_CLOSED) {
 		if (!dev->tty) {
 			if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
+				/* Drop DLC lock here to avoid deadlock
+				 * 1. rfcomm_dev_get will take rfcomm_dev_lock
+				 *    but in rfcomm_dev_add there's lock order:
+				 *    rfcomm_dev_lock -> dlc lock
+				 * 2. rfcomm_dev_put will deaklock if it's
+				 *    the last reference
+				 */
+				rfcomm_dlc_unlock(dlc);
 				if (rfcomm_dev_get(dev->id) == NULL)
 					return;
 
 				rfcomm_dev_del(dev);
 				rfcomm_dev_put(dev);
+				rfcomm_dlc_lock(dlc);
 			}
 		} else
 			tty_hangup(dev->tty);

Re: [PATCH] rfcomm deadlock fix

[Dave Young]

Sorry, I sent a wrong version, I will sent again after a while.

On Sat, May 31, 2008 at 5:09 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> There's logic in __rfcomm_dlc_close:
>        rfcomm_dlc_lock(d);
>        d->state = BT_CLOSED;
>        d->state_changed(d, err);
>        rfcomm_dlc_unlock(d);
>
> In rfcomm_dev_state_change, it's possible that rfcomm_dev_put try to take the
> dlc lock, then we will deadlock.
>
> Here fixed it by unlock dlc before rfcomm_dev_get in rfcomm_dev_state_change.
>
> why not unlock just before rfcomm_dev_put? it's because there's another problem.
> rfcomm_dev_get/rfcomm_dev_del will take rfcomm_dev_lock, but in rfcomm_dev_add
> the lock order is : rfcomm_dev_lock --> dlc lock
>
> so I unlock dlc before the taken of rfcomm_dev_lock.
>
> Actually it's a regression caused by commit
> 1905f6c736cb618e07eca0c96e60e3c024023428, the dlc state_change could be two
> callbacks : rfcomm_sk_state_change and rfcomm_dev_state_change. I missed the rfcomm_sk_state_change that time.
>
> Thanks Arjan van de Ven <arjan@linux.intel.com> for the effort in commit
> 4c8411f8c115def968820a4df6658ccfd55d7f1a
> but he missed the rfcomm_dev_state_change lock issue.
>
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>
> ---
> net/bluetooth/rfcomm/tty.c  |    9 +++++++++
> 1 files changed, 9 insertions(+)
>
> diff -upr linux/net/bluetooth/rfcomm/tty.c linux.new/net/bluetooth/rfcomm/tty.c
> --- linux/net/bluetooth/rfcomm/tty.c    2008-05-30 15:46:33.000000000 +0800
> +++ linux.new/net/bluetooth/rfcomm/tty.c        2008-05-30 16:02:38.000000000 +0800
> @@ -566,11 +566,20 @@ static void rfcomm_dev_state_change(stru
>        if (dlc->state == BT_CLOSED) {
>                if (!dev->tty) {
>                        if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
> +                               /* Drop DLC lock here to avoid deadlock
> +                                * 1. rfcomm_dev_get will take rfcomm_dev_lock
> +                                *    but in rfcomm_dev_add there's lock order:
> +                                *    rfcomm_dev_lock -> dlc lock
> +                                * 2. rfcomm_dev_put will deaklock if it's
> +                                *    the last reference
> +                                */
> +                               rfcomm_dlc_unlock(dlc);
>                                if (rfcomm_dev_get(dev->id) == NULL)
>                                        return;
>
>                                rfcomm_dev_del(dev);
>                                rfcomm_dev_put(dev);
> +                               rfcomm_dlc_lock(dlc);
>                        }
>                } else
>                        tty_hangup(dev->tty);
>



-- 
---
Regards
dave

Re: [PATCH] rfcomm deadlock fix

[Marcel Holtmann]

Hi Dave,

> Sorry, I sent a wrong version, I will sent again after a while.

also do s/deaklock/deadlock/ :)

Regards

Marcel

Re: [PATCH] rfcomm deadlock fix

[Dave Young]

On Sun, Jun 1, 2008 at 2:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Dave,
>
>> Sorry, I sent a wrong version, I will sent again after a while.
>
> also do s/deaklock/deadlock/ :)

My typo, will send again.

Regards
dave