linux-kernel - Re: [PATCH][resend] rfcomm deadlock fix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 1 Jun 2008 09:06:22 +0200
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Dave Young <hidave.darkstar@...il.com>
Cc:	davem@...emloft.net, arjan@...ux.intel.com,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-bluetooth@...r.kernel.org
Subject: Re: [PATCH][resend] rfcomm deadlock fix

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@...ux.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@...il.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-05-30  
> 17:08:30.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 deaklock if it's
> +				 *    the last reference

I meant this one :)  s/deaklock/deadlock/

Regards

Marcel

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