[PATCH] bluetooth: rfcomm_dev_state_change deadlock fix

[PATCH] bluetooth: rfcomm_dev_state_change 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 |   13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

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-06-02 09:16:31.000000000 +0800
@@ -566,11 +566,22 @@ static void rfcomm_dev_state_change(stru
 	if (dlc->state == BT_CLOSED) {
 		if (!dev->tty) {
 			if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
-				if (rfcomm_dev_get(dev->id) == NULL)
+				/* 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 deadlock if it's
+				 *    the last reference
+				 */
+				rfcomm_dlc_unlock(dlc);
+				if (rfcomm_dev_get(dev->id) == NULL) {
+					rfcomm_dlc_lock(dlc);
 					return;
+				}
 
 				rfcomm_dev_del(dev);
 				rfcomm_dev_put(dev);
+				rfcomm_dlc_lock(dlc);
 			}
 		} else
 			tty_hangup(dev->tty);

Re: [PATCH] bluetooth: rfcomm_dev_state_change deadlock fix

[Marcel Holtmann]

Hi Dave,

> 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>

looks good. Thanks for adding a clear comment why we have to do it  
this way.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH] bluetooth: rfcomm_dev_state_change deadlock fix

[David Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Mon, 2 Jun 2008 08:15:03 +0200

> Acked-by: Marcel Holtmann <marcel@holtmann.org>

Applied, but I had to add the commit header string to the two SHA ID
commit references.  Please always provide the header line text as well
as the SHA ID when referencing commits because when backporting or
putting the patch into a different tree the SHA ID will be different
and people will have a terrible time trying to find the commits you
are referring to.

Thanks.

Re: [PATCH] bluetooth: rfcomm_dev_state_change deadlock fix

[Dave Young]

On Mon, Jun 2, 2008 at 2:50 PM, David Miller <davem@davemloft.net> wrote:
> From: Marcel Holtmann <marcel@holtmann.org>
> Date: Mon, 2 Jun 2008 08:15:03 +0200
>
>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
> Applied, but I had to add the commit header string to the two SHA ID
> commit references.  Please always provide the header line text as well
> as the SHA ID when referencing commits because when backporting or
> putting the patch into a different tree the SHA ID will be different
> and people will have a terrible time trying to find the commits you
> are referring to.

Will do next time, thanks.

>
> Thanks.
>



-- 
---
Regards
dave