[PATCH v2 0/2] Bluetooth: Fix race condition with rfkill handling

[PATCH v2 0/2] Bluetooth: Fix race condition with rfkill handling

[johan.hedberg]

Hi,

Here's an updated patch set for the rfkill issue. The only change is the
added explicit initialization of the HCI_RFKILLED flag by checking the
return value of rfkill_blocked() once rfkill_register() has succeeded.

Johan

----------------------------------------------------------------
Johan Hedberg (2):
      Bluetooth: Introduce a new HCI_RFKILLED flag
      Bluetooth: Fix rfkill functionality during the HCI setup stage

 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_core.c    | 25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

[PATCH v2 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag

[johan.hedberg]

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

This makes it more convenient to check for rfkill (no need to check for
dev->rfkill before calling rfkill_blocked()) and also avoids potential
races if the RFKILL state needs to be checked from within the rfkill
callback.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Cc: stable@vger.kernel.org
---
v2: Explicitly intialize HCI_RFKILLED flag after calling rfkill_register

 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_core.c    | 15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 30c88b5..ba008d5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -104,6 +104,7 @@ enum {
 enum {
 	HCI_SETUP,
 	HCI_AUTO_OFF,
+	HCI_RFKILLED,
 	HCI_MGMT,
 	HCI_PAIRABLE,
 	HCI_SERVICE_CACHE,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4dbb6cb..d0d6cf8 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1148,7 +1148,7 @@ int hci_dev_open(__u16 dev)
 		goto done;
 	}
 
-	if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
+	if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
 		ret = -ERFKILL;
 		goto done;
 	}
@@ -1597,10 +1597,12 @@ static int hci_rfkill_set_block(void *data, bool blocked)
 	if (test_bit(HCI_USER_CHANNEL, &hdev->dev_flags))
 		return -EBUSY;
 
-	if (!blocked)
-		return 0;
-
-	hci_dev_do_close(hdev);
+	if (blocked) {
+		set_bit(HCI_RFKILLED, &hdev->dev_flags);
+		hci_dev_do_close(hdev);
+	} else {
+		clear_bit(HCI_RFKILLED, &hdev->dev_flags);
+	}
 
 	return 0;
 }
@@ -2244,6 +2246,9 @@ int hci_register_dev(struct hci_dev *hdev)
 		}
 	}
 
+	if (hdev->rfkill && rfkill_blocked(hdev->rfkill))
+		set_bit(HCI_RFKILLED, &hdev->dev_flags);
+
 	set_bit(HCI_SETUP, &hdev->dev_flags);
 
 	if (hdev->dev_type != HCI_AMP)
-- 
1.8.4.rc3

Re: [PATCH v2 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag

[Marcel Holtmann]

Hi Johan,

> This makes it more convenient to check for rfkill (no need to check for
> dev->rfkill before calling rfkill_blocked()) and also avoids potential
> races if the RFKILL state needs to be checked from within the rfkill
> callback.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> v2: Explicitly intialize HCI_RFKILLED flag after calling rfkill_register
> 
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/hci_core.c    | 15 ++++++++++-----
> 2 files changed, 11 insertions(+), 5 deletions(-)

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

Regards

Marcel

[PATCH v2 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage

[johan.hedberg]

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

We need to let the setup stage complete cleanly even when the HCI device
is rfkilled. Otherwise the HCI device will stay in an undefined state
and never get notified to user space through mgmt (even when it gets
unblocked through rfkill).

This patch makes sure that hci_dev_open() can be called in the HCI_SETUP
stage, that blocking the device doesn't abort the setup stage, and that
the device gets proper powered down as soon as the setup stage completes
in case it was blocked meanwhile.

The bug that this patch fixed can be very easily reproduced using e.g.
the rfkill command line too. By running "rfkill block all" before
inserting a Bluetooth dongle the resulting HCI device goes into a state
where it is never announced over mgmt, not even when "rfkill unblock all"
is run.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Cc: stable@vger.kernel.org
---
 net/bluetooth/hci_core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d0d6cf8..433820a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1148,7 +1148,11 @@ int hci_dev_open(__u16 dev)
 		goto done;
 	}
 
-	if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
+	/* Check for rfkill but allow the HCI setup stage to proceed
+	 * (which in itself doesn't cause any RF activity).
+	 */
+	if (test_bit(HCI_RFKILLED, &hdev->dev_flags) &&
+	    !test_bit(HCI_SETUP, &hdev->dev_flags)) {
 		ret = -ERFKILL;
 		goto done;
 	}
@@ -1599,7 +1603,8 @@ static int hci_rfkill_set_block(void *data, bool blocked)
 
 	if (blocked) {
 		set_bit(HCI_RFKILLED, &hdev->dev_flags);
-		hci_dev_do_close(hdev);
+		if (!test_bit(HCI_SETUP, &hdev->dev_flags))
+			hci_dev_do_close(hdev);
 	} else {
 		clear_bit(HCI_RFKILLED, &hdev->dev_flags);
 	}
@@ -1624,6 +1629,11 @@ static void hci_power_on(struct work_struct *work)
 		return;
 	}
 
+	if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
+		clear_bit(HCI_AUTO_OFF, &hdev->dev_flags);
+		hci_dev_do_close(hdev);
+	}
+
 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
 				   HCI_AUTO_OFF_TIMEOUT);
-- 
1.8.4.rc3

Re: [PATCH v2 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage

[Marcel Holtmann]

Hi Johan,

> We need to let the setup stage complete cleanly even when the HCI device
> is rfkilled. Otherwise the HCI device will stay in an undefined state
> and never get notified to user space through mgmt (even when it gets
> unblocked through rfkill).
> 
> This patch makes sure that hci_dev_open() can be called in the HCI_SETUP
> stage, that blocking the device doesn't abort the setup stage, and that
> the device gets proper powered down as soon as the setup stage completes
> in case it was blocked meanwhile.
> 
> The bug that this patch fixed can be very easily reproduced using e.g.
> the rfkill command line too. By running "rfkill block all" before
> inserting a Bluetooth dongle the resulting HCI device goes into a state
> where it is never announced over mgmt, not even when "rfkill unblock all"
> is run.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> net/bluetooth/hci_core.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d0d6cf8..433820a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1148,7 +1148,11 @@ int hci_dev_open(__u16 dev)
> 		goto done;
> 	}
> 
> -	if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
> +	/* Check for rfkill but allow the HCI setup stage to proceed
> +	 * (which in itself doesn't cause any RF activity).
> +	 */
> +	if (test_bit(HCI_RFKILLED, &hdev->dev_flags) &&
> +	    !test_bit(HCI_SETUP, &hdev->dev_flags)) {
> 		ret = -ERFKILL;
> 		goto done;
> 	}
> @@ -1599,7 +1603,8 @@ static int hci_rfkill_set_block(void *data, bool blocked)
> 
> 	if (blocked) {
> 		set_bit(HCI_RFKILLED, &hdev->dev_flags);
> -		hci_dev_do_close(hdev);
> +		if (!test_bit(HCI_SETUP, &hdev->dev_flags))
> +			hci_dev_do_close(hdev);
> 	} else {
> 		clear_bit(HCI_RFKILLED, &hdev->dev_flags);
> 	}
> @@ -1624,6 +1629,11 @@ static void hci_power_on(struct work_struct *work)
> 		return;
> 	}
> 
> +	if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
> +		clear_bit(HCI_AUTO_OFF, &hdev->dev_flags);
> +		hci_dev_do_close(hdev);
> +	}
> +

this might be better extend with

	} else if (test_bit(HCI_AUTO_OFF, ...)

> 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
> 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
> 				   HCI_AUTO_OFF_TIMEOUT);

Makes the code a little bit more readable for the reason that I want to add a return from the function to the HCI_RFKILLED check every time I read it, but that would be actually wrong.

Regards

Marcel