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

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

[johan.hedberg]

Hi,

There was recently a bug reported regarding the setup stage getting
called twice (Subject: [PATCH v5 1/2] Bluetooth: btmrvl: add setup
handler). There was an initial patch proposal to get an understanding of
what exactly is happening, but the initial fix itself was not the one we
want upstream.

After considering the various options we decided to simply ensure that
the HCIDEVUP ioctl calling path needs to ensure that the req_workqueue
is flushed before calling hci_dev_open. However, this requires some
refactoring and hence two patches instead of one.

I was able to test this with BlueZ 4.101 and Intel HW to see that the
Intel setup handler doesn't get called twice, but it would still be good
to test this with the setup that was used for the initial report.

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 | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

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

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

[Marcel Holtmann]

Hi Johan,

> 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>
> ---
> net/bluetooth/hci_core.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)

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

Regards

Marcel

[PATCH 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.
---
 net/bluetooth/hci_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fc63e78..a216dcf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1227,6 +1227,11 @@ int hci_dev_open(__u16 dev)
 	if (!hdev)
 		return -ENODEV;
 
+	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 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.
> ---
> net/bluetooth/hci_core.c | 5 +++++
> 1 file changed, 5 insertions(+)

you might want to sign off your patch ;)

> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index fc63e78..a216dcf 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1227,6 +1227,11 @@ int hci_dev_open(__u16 dev)
> 	if (!hdev)
> 		return -ENODEV;
> 
> +	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);

And I think we should have a comment here on why we are doing this. Just to remind ourselves.

Regards

Marcel