[PATCH v2 0/2] Bluetooth: Fix hci_dev_open race condition

[PATCH v2 0/2] Bluetooth: Fix hci_dev_open race condition

[johan.hedberg]

Hi,

Here's an updated set including fixes based on feedback. The patches
should go to bluetooth.git, but I'm not sure how important it is to try
to get them to the stable tree since we haven't had this issue reported
with the Intel setup procedure which (afaik) should be the only one that
could be affected in older kernels.

Johan

----------------------------------------------------------------
Johan Hedberg (2):
      Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
      Bluetooth: Fix workqueue synchronization in hci_dev_open

 net/bluetooth/hci_core.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

[PATCH v2 1/2] Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The requirements of an external call to hci_dev_open from hci_sock.c are
different to that from within hci_core.c. In the former case we want to
flush any pending work in hdev->req_workqueue whereas in the latter we
don't (since there we are already calling from within the workqueue
itself). This patch does the necessary refactoring to a separate
hci_dev_do_open function (analogous to hci_dev_do_close) but does not
yet introduce the synchronizations relating to the workqueue usage.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1b66547..fc63e78 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1126,17 +1126,10 @@ void hci_update_ad(struct hci_request *req)
 	hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
 }
 
-/* ---- HCI ioctl helpers ---- */
-
-int hci_dev_open(__u16 dev)
+static int hci_dev_do_open(struct hci_dev *hdev)
 {
-	struct hci_dev *hdev;
 	int ret = 0;
 
-	hdev = hci_dev_get(dev);
-	if (!hdev)
-		return -ENODEV;
-
 	BT_DBG("%s %p", hdev->name, hdev);
 
 	hci_req_lock(hdev);
@@ -1220,10 +1213,27 @@ int hci_dev_open(__u16 dev)
 
 done:
 	hci_req_unlock(hdev);
-	hci_dev_put(hdev);
 	return ret;
 }
 
+/* ---- HCI ioctl helpers ---- */
+
+int hci_dev_open(__u16 dev)
+{
+	struct hci_dev *hdev;
+	int err;
+
+	hdev = hci_dev_get(dev);
+	if (!hdev)
+		return -ENODEV;
+
+	err = hci_dev_do_open(hdev);
+
+	hci_dev_put(hdev);
+
+	return err;
+}
+
 static int hci_dev_do_close(struct hci_dev *hdev)
 {
 	BT_DBG("%s %p", hdev->name, hdev);
@@ -1592,7 +1602,7 @@ static void hci_power_on(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
-	err = hci_dev_open(hdev->id);
+	err = hci_dev_do_open(hdev);
 	if (err < 0) {
 		mgmt_set_powered_failed(hdev, err);
 		return;
-- 
1.8.3.1

[PATCH v2 2/2] Bluetooth: Fix workqueue synchronization in hci_dev_open

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When hci_sock.c calls hci_dev_open it needs to ensure that there isn't
pending work in progress, such as that which is scheduled for the
initial setup procedure or the one for automatically powering off after
the setup procedure. This adds the necessary calls to ensure that any
previously scheduled work is completed before attempting to call
hci_dev_do_open.

This patch fixes a race with old user space versions where we might
receive a HCIDEVUP ioctl before the setup procedure has been completed.
When that happens the setup procedures callback may fail early and leave
the device in an inconsistent state, causing e.g. the setup callback to
be (incorrectly) called more than once.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fc63e78..a573aa2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1227,6 +1227,16 @@ int hci_dev_open(__u16 dev)
 	if (!hdev)
 		return -ENODEV;
 
+	/* We need to ensure that no other power on/off work is pending
+	 * before proceeding to call hci_dev_do_open. This is
+	 * particularly important if the setup procedure has not yet
+	 * completed.
+	 */
+	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags))
+		cancel_delayed_work(&hdev->power_off);
+
+	flush_workqueue(hdev->req_workqueue);
+
 	err = hci_dev_do_open(hdev);
 
 	hci_dev_put(hdev);
-- 
1.8.3.1

Re: [PATCH v2 2/2] Bluetooth: Fix workqueue synchronization in hci_dev_open

[Marcel Holtmann]

Hi Johan,

> When hci_sock.c calls hci_dev_open it needs to ensure that there isn't
> pending work in progress, such as that which is scheduled for the
> initial setup procedure or the one for automatically powering off after
> the setup procedure. This adds the necessary calls to ensure that any
> previously scheduled work is completed before attempting to call
> hci_dev_do_open.
> 
> This patch fixes a race with old user space versions where we might
> receive a HCIDEVUP ioctl before the setup procedure has been completed.
> When that happens the setup procedures callback may fail early and leave
> the device in an inconsistent state, causing e.g. the setup callback to
> be (incorrectly) called more than once.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)

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

Regards

Marcel

Re: [PATCH v2 0/2] Bluetooth: Fix hci_dev_open race condition

[Marcel Holtmann]

Hi Johan,

> Here's an updated set including fixes based on feedback. The patches
> should go to bluetooth.git, but I'm not sure how important it is to try
> to get them to the stable tree since we haven't had this issue reported
> with the Intel setup procedure which (afaik) should be the only one that
> could be affected in older kernels.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (2):
>      Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
>      Bluetooth: Fix workqueue synchronization in hci_dev_open
> 
> net/bluetooth/hci_core.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)

both patches have been applied to bluetooth-next tree.

Regards

Marcel