[PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[Jaganath Kanakkassery]

mgmt_pending_remove() should be called with hci_dev_lock protection
and this patch fixes the same in the missing places.

Without this patch there is a chance of invalid memory access while
accessing the mgmt_pending list like below

bluetoothd:  392] [0] Backtrace:
bluetoothd:  392] [0] [<c04ec770>] (pending_eir_or_class+0x0/0x68) from [<c04f1830>] (add_uuid+0x34/0x1c4)
bluetoothd:  392] [0] [<c04f17fc>] (add_uuid+0x0/0x1c4) from [<c04f3cc4>] (mgmt_control+0x204/0x274)
bluetoothd:  392] [0] [<c04f3ac0>] (mgmt_control+0x0/0x274) from [<c04f609c>] (hci_sock_sendmsg+0x80/0x308)
bluetoothd:  392] [0] [<c04f601c>] (hci_sock_sendmsg+0x0/0x308) from [<c03d4d68>] (sock_aio_write+0x144/0x174)
bluetoothd:  392] [0]  r8:00000000 r7 7c1be90 r6 7c1be18 r5:00000017 r4 a90ea80
bluetoothd:  392] [0] [<c03d4c24>] (sock_aio_write+0x0/0x174) from [<c00e2d4c>] (do_sync_write+0xb0/0xe0)
bluetoothd:  392] [0] [<c00e2c9c>] (do_sync_write+0x0/0xe0) from [<c00e371c>] (vfs_write+0x134/0x13c)
bluetoothd:  392] [0]  r8:00000000 r7 7c1bf70 r6:beeca5c8 r5:00000017 r4 7c05900
bluetoothd:  392] [0] [<c00e35e8>] (vfs_write+0x0/0x13c) from [<c00e3910>] (sys_write+0x44/0x70)
bluetoothd:  392] [0]  r8:00000000 r7:00000004 r6:00000017 r5:beeca5c8 r4 7c05900
bluetoothd:  392] [0] [<c00e38cc>] (sys_write+0x0/0x70) from [<c000e3c0>] (ret_fast_syscall+0x0/0x30)
bluetoothd:  392] [0]  r9 7c1a000 r8:c000e568 r6:400b5f10 r5:403896d8 r4:beeca604
bluetoothd:  392] [0] Code: e28cc00c e152000c 0a00000f e3a00001 (e1d210b8)
bluetoothd:  392] [0] ---[ end trace 67b6ac67435864c4 ]---
bluetoothd:  392] [0] Kernel panic - not syncing: Fatal exception

Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
---
 net/bluetooth/hci_core.c  |    2 ++
 net/bluetooth/hci_event.c |   22 ++++++++++++++++++++--
 net/bluetooth/mgmt.c      |   18 ++++++++++++------
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 96e7321..ecd7c01 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3083,7 +3083,9 @@ static void hci_power_on(struct work_struct *work)
 
 	err = hci_dev_do_open(hdev);
 	if (err < 0) {
+		hci_dev_lock(hdev);
 		mgmt_set_powered_failed(hdev, err);
+		hci_dev_unlock(hdev);
 		return;
 	}
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 322abbb..baa6c96 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -257,6 +257,8 @@ static void hci_cc_write_auth_enable(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!sent)
 		return;
 
+	hci_dev_lock(hdev);
+
 	if (!status) {
 		__u8 param = *((__u8 *) sent);
 
@@ -268,6 +270,8 @@ static void hci_cc_write_auth_enable(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		mgmt_auth_enable_complete(hdev, status);
+
+	hci_dev_unlock(hdev);
 }
 
 static void hci_cc_write_encrypt_mode(struct hci_dev *hdev, struct sk_buff *skb)
@@ -443,6 +447,8 @@ static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!sent)
 		return;
 
+	hci_dev_lock(hdev);
+
 	if (!status) {
 		if (sent->mode)
 			hdev->features[1][0] |= LMP_HOST_SSP;
@@ -458,6 +464,8 @@ static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
 		else
 			clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
 	}
+
+	hci_dev_unlock(hdev);
 }
 
 static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
@@ -471,6 +479,8 @@ static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!sent)
 		return;
 
+	hci_dev_lock(hdev);
+
 	if (!status) {
 		if (sent->support)
 			hdev->features[1][0] |= LMP_HOST_SC;
@@ -486,6 +496,8 @@ static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
 		else
 			clear_bit(HCI_SC_ENABLED, &hdev->dev_flags);
 	}
+
+	hci_dev_unlock(hdev);
 }
 
 static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1172,9 +1184,11 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		 * re-enable it again if necessary.
 		 */
 		if (test_and_clear_bit(HCI_LE_SCAN_INTERRUPTED,
-				       &hdev->dev_flags))
+				       &hdev->dev_flags)) {
+			hci_dev_lock(hdev);
 			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
-		else if (!test_bit(HCI_LE_ADV, &hdev->dev_flags) &&
+			hci_dev_unlock(hdev);
+		} else if (!test_bit(HCI_LE_ADV, &hdev->dev_flags) &&
 			 hdev->discovery.state == DISCOVERY_FINDING)
 			mgmt_reenable_advertising(hdev);
 
@@ -1278,6 +1292,8 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 	if (!sent)
 		return;
 
+	hci_dev_lock(hdev);
+
 	if (sent->le) {
 		hdev->features[1][0] |= LMP_HOST_LE;
 		set_bit(HCI_LE_ENABLED, &hdev->dev_flags);
@@ -1291,6 +1307,8 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 		hdev->features[1][0] |= LMP_HOST_LE_BREDR;
 	else
 		hdev->features[1][0] &= ~LMP_HOST_LE_BREDR;
+
+	hci_dev_unlock(hdev);
 }
 
 static void hci_cc_set_adv_param(struct hci_dev *hdev, struct sk_buff *skb)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 44b20de..16ac037 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2199,12 +2199,14 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
+	hci_dev_lock(hdev);
+
 	if (status) {
 		u8 mgmt_err = mgmt_status(status);
 
 		mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
 				     &mgmt_err);
-		return;
+		goto unlock;
 	}
 
 	mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
@@ -2222,17 +2224,16 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
 	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
 		struct hci_request req;
 
-		hci_dev_lock(hdev);
-
 		hci_req_init(&req, hdev);
 		update_adv_data(&req);
 		update_scan_rsp_data(&req);
 		hci_req_run(&req, NULL);
 
 		hci_update_background_scan(hdev);
-
-		hci_dev_unlock(hdev);
 	}
+
+unlock:
+	hci_dev_unlock(hdev);
 }
 
 static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
@@ -4279,12 +4280,14 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status)
 {
 	struct cmd_lookup match = { NULL, hdev };
 
+	hci_dev_lock(hdev);
+
 	if (status) {
 		u8 mgmt_err = mgmt_status(status);
 
 		mgmt_pending_foreach(MGMT_OP_SET_ADVERTISING, hdev,
 				     cmd_status_rsp, &mgmt_err);
-		return;
+		goto unlock;
 	}
 
 	if (test_bit(HCI_LE_ADV, &hdev->dev_flags))
@@ -4299,6 +4302,9 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status)
 
 	if (match.sk)
 		sock_put(match.sk);
+
+unlock:
+	hci_dev_unlock(hdev);
 }
 
 static int set_advertising(struct sock *sk, struct hci_dev *hdev, void *data,
-- 
1.7.9.5

Re: [PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[Marcel Holtmann]

Hi Jaganath,

> mgmt_pending_remove() should be called with hci_dev_lock protection
> and this patch fixes the same in the missing places.
> 
> Without this patch there is a chance of invalid memory access while
> accessing the mgmt_pending list like below
> 
> bluetoothd:  392] [0] Backtrace:
> bluetoothd:  392] [0] [<c04ec770>] (pending_eir_or_class+0x0/0x68) from [<c04f1830>] (add_uuid+0x34/0x1c4)
> bluetoothd:  392] [0] [<c04f17fc>] (add_uuid+0x0/0x1c4) from [<c04f3cc4>] (mgmt_control+0x204/0x274)
> bluetoothd:  392] [0] [<c04f3ac0>] (mgmt_control+0x0/0x274) from [<c04f609c>] (hci_sock_sendmsg+0x80/0x308)
> bluetoothd:  392] [0] [<c04f601c>] (hci_sock_sendmsg+0x0/0x308) from [<c03d4d68>] (sock_aio_write+0x144/0x174)
> bluetoothd:  392] [0]  r8:00000000 r7 7c1be90 r6 7c1be18 r5:00000017 r4 a90ea80
> bluetoothd:  392] [0] [<c03d4c24>] (sock_aio_write+0x0/0x174) from [<c00e2d4c>] (do_sync_write+0xb0/0xe0)
> bluetoothd:  392] [0] [<c00e2c9c>] (do_sync_write+0x0/0xe0) from [<c00e371c>] (vfs_write+0x134/0x13c)
> bluetoothd:  392] [0]  r8:00000000 r7 7c1bf70 r6:beeca5c8 r5:00000017 r4 7c05900
> bluetoothd:  392] [0] [<c00e35e8>] (vfs_write+0x0/0x13c) from [<c00e3910>] (sys_write+0x44/0x70)
> bluetoothd:  392] [0]  r8:00000000 r7:00000004 r6:00000017 r5:beeca5c8 r4 7c05900
> bluetoothd:  392] [0] [<c00e38cc>] (sys_write+0x0/0x70) from [<c000e3c0>] (ret_fast_syscall+0x0/0x30)
> bluetoothd:  392] [0]  r9 7c1a000 r8:c000e568 r6:400b5f10 r5:403896d8 r4:beeca604
> bluetoothd:  392] [0] Code: e28cc00c e152000c 0a00000f e3a00001 (e1d210b8)
> bluetoothd:  392] [0] ---[ end trace 67b6ac67435864c4 ]---
> bluetoothd:  392] [0] Kernel panic - not syncing: Fatal exception
> 
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> ---
> net/bluetooth/hci_core.c  |    2 ++
> net/bluetooth/hci_event.c |   22 ++++++++++++++++++++--
> net/bluetooth/mgmt.c      |   18 ++++++++++++------
> 3 files changed, 34 insertions(+), 8 deletions(-)

I would split this into two patches. One for the mgmt.c and hci_core.c changes and one for hci_event.c changes.

> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 96e7321..ecd7c01 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3083,7 +3083,9 @@ static void hci_power_on(struct work_struct *work)
> 
> 	err = hci_dev_do_open(hdev);
> 	if (err < 0) {
> +		hci_dev_lock(hdev);
> 		mgmt_set_powered_failed(hdev, err);
> +		hci_dev_unlock(hdev);
> 		return;
> 	}

I wonder is some of the mgmt_ function should just take the hci_dev lock. Are there cases where we don't want them to take the look?

> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 322abbb..baa6c96 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -257,6 +257,8 @@ static void hci_cc_write_auth_enable(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (!sent)
> 		return;
> 
> +	hci_dev_lock(hdev);
> +
> 	if (!status) {
> 		__u8 param = *((__u8 *) sent);
> 
> @@ -268,6 +270,8 @@ static void hci_cc_write_auth_enable(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	if (test_bit(HCI_MGMT, &hdev->dev_flags))
> 		mgmt_auth_enable_complete(hdev, status);
> +
> +	hci_dev_unlock(hdev);
> }
> 
> static void hci_cc_write_encrypt_mode(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -443,6 +447,8 @@ static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (!sent)
> 		return;
> 
> +	hci_dev_lock(hdev);
> +
> 	if (!status) {
> 		if (sent->mode)
> 			hdev->features[1][0] |= LMP_HOST_SSP;
> @@ -458,6 +464,8 @@ static void hci_cc_write_ssp_mode(struct hci_dev *hdev, struct sk_buff *skb)
> 		else
> 			clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
> 	}
> +
> +	hci_dev_unlock(hdev);
> }
> 
> static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -471,6 +479,8 @@ static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
> 	if (!sent)
> 		return;
> 
> +	hci_dev_lock(hdev);
> +
> 	if (!status) {
> 		if (sent->support)
> 			hdev->features[1][0] |= LMP_HOST_SC;
> @@ -486,6 +496,8 @@ static void hci_cc_write_sc_support(struct hci_dev *hdev, struct sk_buff *skb)
> 		else
> 			clear_bit(HCI_SC_ENABLED, &hdev->dev_flags);
> 	}
> +
> +	hci_dev_unlock(hdev);
> }
> 
> static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -1172,9 +1184,11 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> 		 * re-enable it again if necessary.
> 		 */
> 		if (test_and_clear_bit(HCI_LE_SCAN_INTERRUPTED,
> -				       &hdev->dev_flags))
> +				       &hdev->dev_flags)) {
> +			hci_dev_lock(hdev);
> 			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> -		else if (!test_bit(HCI_LE_ADV, &hdev->dev_flags) &&
> +			hci_dev_unlock(hdev);
> +		} else if (!test_bit(HCI_LE_ADV, &hdev->dev_flags) &&
> 			 hdev->discovery.state == DISCOVERY_FINDING)

This is now a coding style violation. You need to move this line as well.

> 			mgmt_reenable_advertising(hdev);
> 
> @@ -1278,6 +1292,8 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
> 	if (!sent)
> 		return;
> 
> +	hci_dev_lock(hdev);
> +
> 	if (sent->le) {
> 		hdev->features[1][0] |= LMP_HOST_LE;
> 		set_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> @@ -1291,6 +1307,8 @@ static void hci_cc_write_le_host_supported(struct hci_dev *hdev,
> 		hdev->features[1][0] |= LMP_HOST_LE_BREDR;
> 	else
> 		hdev->features[1][0] &= ~LMP_HOST_LE_BREDR;
> +
> +	hci_dev_unlock(hdev);
> }
> 
> static void hci_cc_set_adv_param(struct hci_dev *hdev, struct sk_buff *skb)
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 44b20de..16ac037 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2199,12 +2199,14 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
> {
> 	struct cmd_lookup match = { NULL, hdev };
> 
> +	hci_dev_lock(hdev);
> +
> 	if (status) {
> 		u8 mgmt_err = mgmt_status(status);
> 
> 		mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, cmd_status_rsp,
> 				     &mgmt_err);
> -		return;
> +		goto unlock;
> 	}
> 
> 	mgmt_pending_foreach(MGMT_OP_SET_LE, hdev, settings_rsp, &match);
> @@ -2222,17 +2224,16 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
> 	if (test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> 		struct hci_request req;
> 
> -		hci_dev_lock(hdev);
> -
> 		hci_req_init(&req, hdev);
> 		update_adv_data(&req);
> 		update_scan_rsp_data(&req);
> 		hci_req_run(&req, NULL);
> 
> 		hci_update_background_scan(hdev);
> -
> -		hci_dev_unlock(hdev);
> 	}
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> }
> 
> static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> @@ -4279,12 +4280,14 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status)
> {
> 	struct cmd_lookup match = { NULL, hdev };
> 
> +	hci_dev_lock(hdev);
> +
> 	if (status) {
> 		u8 mgmt_err = mgmt_status(status);
> 
> 		mgmt_pending_foreach(MGMT_OP_SET_ADVERTISING, hdev,
> 				     cmd_status_rsp, &mgmt_err);
> -		return;
> +		goto unlock;
> 	}
> 
> 	if (test_bit(HCI_LE_ADV, &hdev->dev_flags))
> @@ -4299,6 +4302,9 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status)
> 
> 	if (match.sk)
> 		sock_put(match.sk);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> }
> 

Regards

Marcel

Re: Re: [PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[JAGANATH KANAKKASSERY]

SGkgTWFyY2VsLA0KDQo+PiBtZ210X3BlbmRpbmdfcmVtb3ZlKCkgc2hvdWxkIGJlIGNhbGxlZCB3
aXRoIGhjaV9kZXZfbG9jayBwcm90ZWN0aW9uDQo+PiBhbmQgdGhpcyBwYXRjaCBmaXhlcyB0aGUg
c2FtZSBpbiB0aGUgbWlzc2luZyBwbGFjZXMuDQo+ID4NCj4+V2l0aG91dCB0aGlzIHBhdGNoIHRo
ZXJlIGlzIGEgY2hhbmNlIG9mIGludmFsaWQgbWVtb3J5IGFjY2VzcyB3aGlsZQ0KPj4gYWNjZXNz
aW5nIHRoZSBtZ210X3BlbmRpbmcgbGlzdCBsaWtlIGJlbG93DQo+PiANCj4+IGJsdWV0b290aGQ6
ICAzOTJdIFswXSBCYWNrdHJhY2U6DQo+PiBibHVldG9vdGhkOiAgMzkyXSBbMF0gWzxjMDRlYzc3
MD5dIChwZW5kaW5nX2Vpcl9vcl9jbGFzcysweDAvMHg2OCkgZnJvbSBbPGMwNGYxODMwPl0gKGFk
ZF91dWlkKzB4MzQvMHgxYzQpDQo+PiBibHVldG9vdGhkOiAgMzkyXSBbMF0gWzxjMDRmMTdmYz5d
IChhZGRfdXVpZCsweDAvMHgxYzQpIGZyb20gWzxjMDRmM2NjND5dIChtZ210X2NvbnRyb2wrMHgy
MDQvMHgyNzQpDQo+PiBibHVldG9vdGhkOiAgMzkyXSBbMF0gWzxjMDRmM2FjMD5dIChtZ210X2Nv
bnRyb2wrMHgwLzB4Mjc0KSBmcm9tIFs8YzA0ZjYwOWM+XSAoaGNpX3NvY2tfc2VuZG1zZysweDgw
LzB4MzA4KQ0KPj4gYmx1ZXRvb3RoZDogIDM5Ml0gWzBdIFs8YzA0ZjYwMWM+XSAoaGNpX3NvY2tf
c2VuZG1zZysweDAvMHgzMDgpIGZyb20gWzxjMDNkNGQ2OD5dIChzb2NrX2Fpb193cml0ZSsweDE0
NC8weDE3NCkNCj4+IGJsdWV0b290aGQ6ICAzOTJdIFswXSAgcjg6MDAwMDAwMDAgcjcgN2MxYmU5
MCByNiA3YzFiZTE4IHI1OjAwMDAwMDE3IHI0IGE5MGVhODANCj4+IGJsdWV0b290aGQ6ICAzOTJd
IFswXSBbPGMwM2Q0YzI0Pl0gKHNvY2tfYWlvX3dyaXRlKzB4MC8weDE3NCkgZnJvbSBbPGMwMGUy
ZDRjPl0gKGRvX3N5bmNfd3JpdGUrMHhiMC8weGUwKQ0KPj4gYmx1ZXRvb3RoZDogIDM5Ml0gWzBd
IFs8YzAwZTJjOWM+XSAoZG9fc3luY193cml0ZSsweDAvMHhlMCkgZnJvbSBbPGMwMGUzNzFjPl0g
KHZmc193cml0ZSsweDEzNC8weDEzYykNCj4+IGJsdWV0b290aGQ6ICAzOTJdIFswXSAgcjg6MDAw
MDAwMDAgcjcgN2MxYmY3MCByNjpiZWVjYTVjOCByNTowMDAwMDAxNyByNCA3YzA1OTAwDQo+PiBi
bHVldG9vdGhkOiAgMzkyXSBbMF0gWzxjMDBlMzVlOD5dICh2ZnNfd3JpdGUrMHgwLzB4MTNjKSBm
cm9tIFs8YzAwZTM5MTA+XSAoc3lzX3dyaXRlKzB4NDQvMHg3MCkNCj4+IGJsdWV0b290aGQ6ICAz
OTJdIFswXSAgcjg6MDAwMDAwMDAgcjc6MDAwMDAwMDQgcjY6MDAwMDAwMTcgcjU6YmVlY2E1Yzgg
cjQgN2MwNTkwMA0KPj4gYmx1ZXRvb3RoZDogIDM5Ml0gWzBdIFs8YzAwZTM4Y2M+XSAoc3lzX3dy
aXRlKzB4MC8weDcwKSBmcm9tIFs8YzAwMGUzYzA+XSAocmV0X2Zhc3Rfc3lzY2FsbCsweDAvMHgz
MCkNCj4+IGJsdWV0b290aGQ6ICAzOTJdIFswXSAgcjkgN2MxYTAwMCByODpjMDAwZTU2OCByNjo0
MDBiNWYxMCByNTo0MDM4OTZkOCByNDpiZWVjYTYwNA0KPj4gYmx1ZXRvb3RoZDogIDM5Ml0gWzBd
IENvZGU6IGUyOGNjMDBjIGUxNTIwMDBjIDBhMDAwMDBmIGUzYTAwMDAxIChlMWQyMTBiOCkNCj4+
IGJsdWV0b290aGQ6ICAzOTJdIFswXSAtLS1bIGVuZCB0cmFjZSA2N2I2YWM2NzQzNTg2NGM0IF0t
LS0NCj4+IGJsdWV0b290aGQ6ICAzOTJdIFswXSBLZXJuZWwgcGFuaWMgLSBub3Qgc3luY2luZzog
RmF0YWwgZXhjZXB0aW9uDQo+PiANCj4+U2lnbmVkLW9mZi1ieTogSmFnYW5hdGggS2FuYWtrYXNz
ZXJ5IDxqYWdhbmF0aC5rQHNhbXN1bmcuY29tPg0KPj4gLS0tDQo+PiBuZXQvYmx1ZXRvb3RoL2hj
aV9jb3JlLmMgIHwgICAgMiArKw0KPj4gbmV0L2JsdWV0b290aC9oY2lfZXZlbnQuYyB8ICAgMjIg
KysrKysrKysrKysrKysrKysrKystLQ0KPj4gbmV0L2JsdWV0b290aC9tZ210LmMgICAgICB8ICAg
MTggKysrKysrKysrKysrLS0tLS0tDQo+PiAzIGZpbGVzIGNoYW5nZWQsIDM0IGluc2VydGlvbnMo
KyksIDggZGVsZXRpb25zKC0pDQoNCj5JIHdvdWxkIHNwbGl0IHRoaXMgaW50byB0d28gcGF0Y2hl
cy4gT25lIGZvciB0aGUgbWdtdC5jIGFuZCBoY2lfY29yZS5jIGNoYW5nZXMgYW5kIG9uZSBmb3Ig
aGNpX2V2ZW50LmMgY2hhbmdlcy4NCg0KT2ssIEkgd2lsbCBzcGxpdCB0aGUgcGF0Y2ggaW50byB0
d28gYW5kIHJhaXNlIGFnYWluDQoNCj4+DQo+PiBkaWZmIC0tZ2l0IGEvbmV0L2JsdWV0b290aC9o
Y2lfY29yZS5jIGIvbmV0L2JsdWV0b290aC9oY2lfY29yZS5jDQo+PmluZGV4IDk2ZTczMjEuLmVj
ZDdjMDEgMTAwNjQ0DQo+PiAtLS0gYS9uZXQvYmx1ZXRvb3RoL2hjaV9jb3JlLmMNCj4+ICsrKyBi
L25ldC9ibHVldG9vdGgvaGNpX2NvcmUuYw0KPj4gQEAgLTMwODMsNyArMzA4Myw5IEBAIHN0YXRp
YyB2b2lkIGhjaV9wb3dlcl9vbihzdHJ1Y3Qgd29ya19zdHJ1Y3QgKndvcmspDQo+PiANCj4+IAll
cnIgPSBoY2lfZGV2X2RvX29wZW4oaGRldik7DQo+PiAJaWYgKGVyciA8IDApIHsNCj4+ICsJCWhj
aV9kZXZfbG9jayhoZGV2KTsNCj4+IAkJbWdtdF9zZXRfcG93ZXJlZF9mYWlsZWQoaGRldiwgZXJy
KTsNCj4+ICsJCWhjaV9kZXZfdW5sb2NrKGhkZXYpOw0KPj4gCQlyZXR1cm47DQo+PiAJfQ0KDQo+
SSB3b25kZXIgaXMgc29tZSBvZiB0aGUgbWdtdF8gZnVuY3Rpb24gc2hvdWxkIGp1c3QgdGFrZSB0
aGUgaGNpX2RldiBsb2NrLiBBcmUgdGhlcmUgY2FzZXMgd2hlcmUgd2UgZG9uJ3Qgd2FudCB0aGVt
IHRvIHRha2UgdGhlIGxvb2s/DQoNClRoZXJlIGFyZSBtYW55IG1nbXRfZnVuY3Rpb25zIGNhbGxl
ZCBmcm9tIGhjaV9ldmVudC5jIHdoaWNoIGRvbid0IHJlcXVpcmUgbG9jay4NCllvdSBwcmVmZXIg
bW92aW5nIHRoZSBsb2NrIGluc2lkZSBtZ210X3NldF9wb3dlcmVkX2ZhaWxlZCgpPw0KPj4gDQo+
PiBkaWZmIC0tZ2l0IGEvbmV0L2JsdWV0b290aC9oY2lfZXZlbnQuYyBiL25ldC9ibHVldG9vdGgv
aGNpX2V2ZW50LmMNCj4+IGluZGV4IDMyMmFiYmIuLmJhYTZjOTYgMTAwNjQ0DQo+PiAtLS0gYS9u
ZXQvYmx1ZXRvb3RoL2hjaV9ldmVudC5jDQo+PiArKysgYi9uZXQvYmx1ZXRvb3RoL2hjaV9ldmVu
dC5jDQo+PiBAQCAtMjU3LDYgKzI1Nyw4IEBAIHN0YXRpYyB2b2lkIGhjaV9jY193cml0ZV9hdXRo
X2VuYWJsZShzdHJ1Y3QgaGNpX2RldiAqaGRldiwgc3RydWN0IHNrX2J1ZmYgKnNrYikNCj4+IAlp
ZiAoIXNlbnQpDQo+PiAJCXJldHVybjsNCj4+IA0KPj4gKwloY2lfZGV2X2xvY2soaGRldik7DQo+
PiArDQo+PiAJaWYgKCFzdGF0dXMpIHsNCj4+IAkJX191OCBwYXJhbSA9ICooKF9fdTggKikgc2Vu
dCk7DQo+Pg0KPj4gQEAgLTI2OCw2ICsyNzAsOCBAQCBzdGF0aWMgdm9pZCBoY2lfY2Nfd3JpdGVf
YXV0aF9lbmFibGUoc3RydWN0IGhjaV9kZXYgKmhkZXYsIHN0cnVjdCBza19idWZmICpza2IpDQo+
PiANCj4+IAlpZiAodGVzdF9iaXQoSENJX01HTVQsICZoZGV2LT5kZXZfZmxhZ3MpKQ0KPj4gCQlt
Z210X2F1dGhfZW5hYmxlX2NvbXBsZXRlKGhkZXYsIHN0YXR1cyk7DQo+PiArDQo+PiArCWhjaV9k
ZXZfdW5sb2NrKGhkZXYpOw0KPj4gfQ0KPj4gDQo+PiBzdGF0aWMgdm9pZCBoY2lfY2Nfd3JpdGVf
ZW5jcnlwdF9tb2RlKHN0cnVjdCBoY2lfZGV2ICpoZGV2LCBzdHJ1Y3Qgc2tfYnVmZiAqc2tiKQ0K
Pj4gQEAgLTQ0Myw2ICs0NDcsOCBAQCBzdGF0aWMgdm9pZCBoY2lfY2Nfd3JpdGVfc3NwX21vZGUo
c3RydWN0IGhjaV9kZXYgKmhkZXYsIHN0cnVjdCBza19idWZmICpza2IpDQo+PiAJaWYgKCFzZW50
KQ0KPj4gCQlyZXR1cm47DQo+PiANCj4+ICsJaGNpX2Rldl9sb2NrKGhkZXYpOw0KPj4gKw0KPj4g
CWlmICghc3RhdHVzKSB7DQo+PiAJCWlmIChzZW50LT5tb2RlKQ0KPj4gCQkJaGRldi0+ZmVhdHVy
ZXNbMV1bMF0gfD0gTE1QX0hPU1RfU1NQOw0KPj4gQEAgLTQ1OCw2ICs0NjQsOCBAQCBzdGF0aWMg
dm9pZCBoY2lfY2Nfd3JpdGVfc3NwX21vZGUoc3RydWN0IGhjaV9kZXYgKmhkZXYsIHN0cnVjdCBz
a19idWZmICpza2IpDQo+PiAJCWVsc2UNCj4+IAkJCWNsZWFyX2JpdChIQ0lfU1NQX0VOQUJMRUQs
ICZoZGV2LT5kZXZfZmxhZ3MpOw0KPj4gCX0NCj4+ICsNCj4+ICsJaGNpX2Rldl91bmxvY2soaGRl
dik7DQo+PiB9DQo+PiANCj4+IHN0YXRpYyB2b2lkIGhjaV9jY193cml0ZV9zY19zdXBwb3J0KHN0
cnVjdCBoY2lfZGV2ICpoZGV2LCBzdHJ1Y3Qgc2tfYnVmZiAqc2tiKQ0KPj4gQEAgLTQ3MSw2ICs0
NzksOCBAQCBzdGF0aWMgdm9pZCBoY2lfY2Nfd3JpdGVfc2Nfc3VwcG9ydChzdHJ1Y3QgaGNpX2Rl
diAqaGRldiwgc3RydWN0IHNrX2J1ZmYgKnNrYikNCj4+IAlpZiAoIXNlbnQpDQo+PiAJCXJldHVy
bjsNCj4+IA0KPj4rCWhjaV9kZXZfbG9jayhoZGV2KTsNCj4+ICsNCj4+IAlpZiAoIXN0YXR1cykg
ew0KPj4gCQlpZiAoc2VudC0+c3VwcG9ydCkNCj4+IAkJCWhkZXYtPmZlYXR1cmVzWzFdWzBdIHw9
IExNUF9IT1NUX1NDOw0KPj4gQEAgLTQ4Niw2ICs0OTYsOCBAQCBzdGF0aWMgdm9pZCBoY2lfY2Nf
d3JpdGVfc2Nfc3VwcG9ydChzdHJ1Y3QgaGNpX2RldiAqaGRldiwgc3RydWN0IHNrX2J1ZmYgKnNr
YikNCj4+IAkJZWxzZQ0KPj4gCQkJY2xlYXJfYml0KEhDSV9TQ19FTkFCTEVELCAmaGRldi0+ZGV2
X2ZsYWdzKTsNCj4+IAl9DQo+PiArDQo+PiArCWhjaV9kZXZfdW5sb2NrKGhkZXYpOw0KPj4gfQ0K
Pj4gDQo+PiBzdGF0aWMgdm9pZCBoY2lfY2NfcmVhZF9sb2NhbF92ZXJzaW9uKHN0cnVjdCBoY2lf
ZGV2ICpoZGV2LCBzdHJ1Y3Qgc2tfYnVmZiAqc2tiKQ0KPj4gQEAgLTExNzIsOSArMTE4NCwxMSBA
QCBzdGF0aWMgdm9pZCBoY2lfY2NfbGVfc2V0X3NjYW5fZW5hYmxlKHN0cnVjdCBoY2lfZGV2ICpo
ZGV2LA0KPj4gCQkgKiByZS1lbmFibGUgaXQgYWdhaW4gaWYgbmVjZXNzYXJ5Lg0KPj4gCQkgKi8N
Cj4+IAkJaWYgKHRlc3RfYW5kX2NsZWFyX2JpdChIQ0lfTEVfU0NBTl9JTlRFUlJVUFRFRCwNCj4+
LQkJCQkgICAgICAgJmhkZXYtPmRldl9mbGFncykpDQo+PiArCQkJCSAgICAgICAmaGRldi0+ZGV2
X2ZsYWdzKSkgew0KPj4gKwkJCWhjaV9kZXZfbG9jayhoZGV2KTsNCj4+IAkJCWhjaV9kaXNjb3Zl
cnlfc2V0X3N0YXRlKGhkZXYsIERJU0NPVkVSWV9TVE9QUEVEKTsNCj4+IC0JCWVsc2UgaWYgKCF0
ZXN0X2JpdChIQ0lfTEVfQURWLCAmaGRldi0+ZGV2X2ZsYWdzKSAmJg0KPj4gKwkJCWhjaV9kZXZf
dW5sb2NrKGhkZXYpOw0KPj4gKwkJfSBlbHNlIGlmICghdGVzdF9iaXQoSENJX0xFX0FEViwgJmhk
ZXYtPmRldl9mbGFncykgJiYNCj4+IAkJCSBoZGV2LT5kaXNjb3Zlcnkuc3RhdGUgPT0gRElTQ09W
RVJZX0ZJTkRJTkcpDQoNCj5UaGlzIGlzIG5vdyBhIGNvZGluZyBzdHlsZSB2aW9sYXRpb24uIFlv
dSBuZWVkIHRvIG1vdmUgdGhpcyBsaW5lIGFzIHdlbGwuDQoNCk9rIEkgd2lsbCBmaXggaXQgaW4g
dGhlIG5leHQgcGF0Y2ggc2V0DQoNCj4+IAkJCW1nbXRfcmVlbmFibGVfYWR2ZXJ0aXNpbmcoaGRl
dik7DQo+PiANCj4+IEBAIC0xMjc4LDYgKzEyOTIsOCBAQCBzdGF0aWMgdm9pZCBoY2lfY2Nfd3Jp
dGVfbGVfaG9zdF9zdXBwb3J0ZWQoc3RydWN0IGhjaV9kZXYgKmhkZXYsDQo+PiAJaWYgKCFzZW50
KQ0KPj4gCQlyZXR1cm47DQo+PiANCj4+ICsJaGNpX2Rldl9sb2NrKGhkZXYpOw0KPj4gKw0KPj4g
CWlmIChzZW50LT5sZSkgew0KPj4gCQloZGV2LT5mZWF0dXJlc1sxXVswXSB8PSBMTVBfSE9TVF9M
RTsNCj4+IAkJc2V0X2JpdChIQ0lfTEVfRU5BQkxFRCwgJmhkZXYtPmRldl9mbGFncyk7DQo+PiBA
QCAtMTI5MSw2ICsxMzA3LDggQEAgc3RhdGljIHZvaWQgaGNpX2NjX3dyaXRlX2xlX2hvc3Rfc3Vw
cG9ydGVkKHN0cnVjdCBoY2lfZGV2ICpoZGV2LA0KPj4gCQloZGV2LT5mZWF0dXJlc1sxXVswXSB8
PSBMTVBfSE9TVF9MRV9CUkVEUjsNCj4+IAllbHNlDQo+PiAJCWhkZXYtPmZlYXR1cmVzWzFdWzBd
ICY9IH5MTVBfSE9TVF9MRV9CUkVEUjsNCj4+ICsNCj4+ICsJaGNpX2Rldl91bmxvY2soaGRldik7
DQo+PiB9DQo+PiANCj4+IHN0YXRpYyB2b2lkIGhjaV9jY19zZXRfYWR2X3BhcmFtKHN0cnVjdCBo
Y2lfZGV2ICpoZGV2LCBzdHJ1Y3Qgc2tfYnVmZiAqc2tiKQ0KPj4gZGlmZiAtLWdpdCBhL25ldC9i
bHVldG9vdGgvbWdtdC5jIGIvbmV0L2JsdWV0b290aC9tZ210LmMNCj4+IGluZGV4IDQ0YjIwZGUu
LjE2YWMwMzcgMTAwNjQ0DQo+PiAtLS0gYS9uZXQvYmx1ZXRvb3RoL21nbXQuYw0KPj4gKysrIGIv
bmV0L2JsdWV0b290aC9tZ210LmMNCj4+IEBAIC0yMTk5LDEyICsyMTk5LDE0IEBAIHN0YXRpYyB2
b2lkIGxlX2VuYWJsZV9jb21wbGV0ZShzdHJ1Y3QgaGNpX2RldiAqaGRldiwgdTggc3RhdHVzKQ0K
Pj4gew0KPj4gCXN0cnVjdCBjbWRfbG9va3VwIG1hdGNoID0geyBOVUxMLCBoZGV2IH07DQo+PiAN
Cj4+ICsJaGNpX2Rldl9sb2NrKGhkZXYpOw0KPj4gKw0KPj4gCWlmIChzdGF0dXMpIHsNCj4+IAkJ
dTggbWdtdF9lcnIgPSBtZ210X3N0YXR1cyhzdGF0dXMpOw0KPj4gDQo+PiAJCW1nbXRfcGVuZGlu
Z19mb3JlYWNoKE1HTVRfT1BfU0VUX0xFLCBoZGV2LCBjbWRfc3RhdHVzX3JzcCwNCj4+IAkJCQkg
ICAgICZtZ210X2Vycik7DQo+PiAtCQlyZXR1cm47DQo+PiArCQlnb3RvIHVubG9jazsNCj4+IAl9
DQo+PiANCj4+IAltZ210X3BlbmRpbmdfZm9yZWFjaChNR01UX09QX1NFVF9MRSwgaGRldiwgc2V0
dGluZ3NfcnNwLCAmbWF0Y2gpOw0KPj4gQEAgLTIyMjIsMTcgKzIyMjQsMTYgQEAgc3RhdGljIHZv
aWQgbGVfZW5hYmxlX2NvbXBsZXRlKHN0cnVjdCBoY2lfZGV2ICpoZGV2LCB1OCBzdGF0dXMpDQo+
PiAJaWYgKHRlc3RfYml0KEhDSV9MRV9FTkFCTEVELCAmaGRldi0+ZGV2X2ZsYWdzKSkgew0KPj4g
CQlzdHJ1Y3QgaGNpX3JlcXVlc3QgcmVxOw0KPj4NCj4+LQkJaGNpX2Rldl9sb2NrKGhkZXYpOw0K
Pj4gLQ0KPj4gCQloY2lfcmVxX2luaXQoJnJlcSwgaGRldik7DQo+PiAJCXVwZGF0ZV9hZHZfZGF0
YSgmcmVxKTsNCj4+IAkJdXBkYXRlX3NjYW5fcnNwX2RhdGEoJnJlcSk7DQo+PiAJCWhjaV9yZXFf
cnVuKCZyZXEsIE5VTEwpOw0KPj4gDQo+PiAJCWhjaV91cGRhdGVfYmFja2dyb3VuZF9zY2FuKGhk
ZXYpOw0KPj4gLQ0KPj4gLQkJaGNpX2Rldl91bmxvY2soaGRldik7DQo+PiAJfQ0KPj4gKw0KPj4g
K3VubG9jazoNCj4+ICsJaGNpX2Rldl91bmxvY2soaGRldik7DQo+PiB9DQo+PiANCj4+IHN0YXRp
YyBpbnQgc2V0X2xlKHN0cnVjdCBzb2NrICpzaywgc3RydWN0IGhjaV9kZXYgKmhkZXYsIHZvaWQg
KmRhdGEsIHUxNiBsZW4pDQo+PiBAQCAtNDI3OSwxMiArNDI4MCwxNCBAQCBzdGF0aWMgdm9pZCBz
ZXRfYWR2ZXJ0aXNpbmdfY29tcGxldGUoc3RydWN0IGhjaV9kZXYgKmhkZXYsIHU4IHN0YXR1cykN
Cj4+IHsNCj4+IAlzdHJ1Y3QgY21kX2xvb2t1cCBtYXRjaCA9IHsgTlVMTCwgaGRldiB9Ow0KPj4g
DQo+PiArCWhjaV9kZXZfbG9jayhoZGV2KTsNCj4+ICsNCj4+IAlpZiAoc3RhdHVzKSB7DQo+PiAJ
CXU4IG1nbXRfZXJyID0gbWdtdF9zdGF0dXMoc3RhdHVzKTsNCj4+IA0KPj4gCQltZ210X3BlbmRp
bmdfZm9yZWFjaChNR01UX09QX1NFVF9BRFZFUlRJU0lORywgaGRldiwNCj4+IAkJCQkgICAgIGNt
ZF9zdGF0dXNfcnNwLCAmbWdtdF9lcnIpOw0KPj4gLQkJcmV0dXJuOw0KPj4gKwkJZ290byB1bmxv
Y2s7DQo+PiAJfQ0KPj4gDQo+PiAJaWYgKHRlc3RfYml0KEhDSV9MRV9BRFYsICZoZGV2LT5kZXZf
ZmxhZ3MpKQ0KPj4gQEAgLTQyOTksNiArNDMwMiw5IEBAIHN0YXRpYyB2b2lkIHNldF9hZHZlcnRp
c2luZ19jb21wbGV0ZShzdHJ1Y3QgaGNpX2RldiAqaGRldiwgdTggc3RhdHVzKQ0KPj4gDQo+PiAJ
aWYgKG1hdGNoLnNrKQ0KPj4gCQlzb2NrX3B1dChtYXRjaC5zayk7DQo+PiArDQo+PiArdW5sb2Nr
Og0KPj4gKwloY2lfZGV2X3VubG9jayhoZGV2KTsNCj4+IH0NCj4+IA0KDQpUaGFua3MsDQpKYWdh
bmF0aA==

Re: [PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[Johan Hedberg]

Hi Jaganath & Marcel,

On Wed, Dec 10, 2014, JAGANATH KANAKKASSERY wrote:
> >> 	err = hci_dev_do_open(hdev);
> >> 	if (err < 0) {
> >> +		hci_dev_lock(hdev);
> >> 		mgmt_set_powered_failed(hdev, err);
> >> +		hci_dev_unlock(hdev);
> >> 		return;
> >> 	}
> 
> >I wonder is some of the mgmt_ function should just take the hci_dev
> >lock. Are there cases where we don't want them to take the look?
> 
> There are many mgmt_functions called from hci_event.c which don't
> require lock.  You prefer moving the lock inside
> mgmt_set_powered_failed()?

This should be kept consistent imo. Either all mgmt_* functions called
from hci_core/event.c should be responsible for taking the lock
themselves or none should be. I think right now the general rule is that
hci_core/event.c take the lock first, and this is mainly because a lot
of code that needed calling into mgmt.c was already holding the lock.
I.e. moving the taking into mgmt.c would for many places have required
awkward looking hdev_unlock(); mgmt_foo(); hdev_lock(); constructions.

Johan

Re: [PATCH] Bluetooth: Fix missing hci_dev_lock/unlock

[Marcel Holtmann]

Hi Johan,

>>>> 	err = hci_dev_do_open(hdev);
>>>> 	if (err < 0) {
>>>> +		hci_dev_lock(hdev);
>>>> 		mgmt_set_powered_failed(hdev, err);
>>>> +		hci_dev_unlock(hdev);
>>>> 		return;
>>>> 	}
>> 
>>> I wonder is some of the mgmt_ function should just take the hci_dev
>>> lock. Are there cases where we don't want them to take the look?
>> 
>> There are many mgmt_functions called from hci_event.c which don't
>> require lock.  You prefer moving the lock inside
>> mgmt_set_powered_failed()?
> 
> This should be kept consistent imo. Either all mgmt_* functions called
> from hci_core/event.c should be responsible for taking the lock
> themselves or none should be. I think right now the general rule is that
> hci_core/event.c take the lock first, and this is mainly because a lot
> of code that needed calling into mgmt.c was already holding the lock.
> I.e. moving the taking into mgmt.c would for many places have required
> awkward looking hdev_unlock(); mgmt_foo(); hdev_lock(); constructions.

we could also rename some to __mgmt_foo which means they are not taking the lock and others to mgmt_bar which take the lock by themselves. And then see if they actually have variations where we would need both.

Regards

Marcel