[PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP

[PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP

[Mattijs Korpershoek]

Some HCI devices which have the HCI_QUIRK_NON_PERSISTENT_SETUP [1]
require a call to setup() to be ran after every open().

During the setup() stage, these devices expect the chip to acknowledge
its setup() completion via vendor specific frames.

If userspace opens() such HCI device in HCI_USER_CHANNEL [2] mode,
the vendor specific frames are never tranmitted to the driver, as
they are filtered in hci_rx_work().

Allow HCI devices which have HCI_QUIRK_NON_PERSISTENT_SETUP to process
frames if the HCI device is is HCI_INIT state.

[1] https://lore.kernel.org/patchwork/patch/965071/
[2] https://www.spinics.net/lists/linux-bluetooth/msg37345.html

Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings")
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
Some more background on the change follows:

The Android bluetooth stack (Bluedroid) also has a HAL implementation
which follows Linux's standard rfkill interface [1].

This implementation relies on the HCI_CHANNEL_USER feature to get
exclusive access to the underlying bluetooth device.

When testing this along with the btkmtksdio driver, the
chip appeared unresponsive when calling the following from userspace:

    struct sockaddr_hci addr;
    int fd;

    fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);

    memset(&addr, 0, sizeof(addr));
    addr.hci_family = AF_BLUETOOTH;
    addr.hci_dev = 0;
    addr.hci_channel = HCI_CHANNEL_USER;

    bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device hangs

In the case of bluetooth drivers exposing QUIRK_NON_PERSISTENT_SETUP
such as btmtksdio, setup() is called each multiple times.
In particular, when userspace calls bind(), the setup() is called again
and vendor specific commands might be send to re-initialize the chip.

Those commands are filtered out by hci_core in HCI_CHANNEL_USER mode,
preventing setup() from completing successfully.

This has been tested on a 4.19 kernel based on Android Common Kernel.
It has also been compile tested on bluetooth-next.

[1] https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/

 net/bluetooth/hci_core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 04bc79359a17..5f12e8574d54 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -4440,9 +4440,20 @@ static void hci_rx_work(struct work_struct *work)
 			hci_send_to_sock(hdev, skb);
 		}
 
+		/* If the device has been opened in HCI_USER_CHANNEL,
+		 * the userspace has exclusive access to device.
+		 * When HCI_QUIRK_NON_PERSISTENT_SETUP is set and
+		 * device is HCI_INIT,  we still need to process
+		 * the data packets to the driver in order
+		 * to complete its setup().
+		 */
 		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
-			kfree_skb(skb);
-			continue;
+			if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,
+				      &hdev->quirks) ||
+			    !test_bit(HCI_INIT, &hdev->flags)) {
+				kfree_skb(skb);
+				continue;
+			}
 		}
 
 		if (test_bit(HCI_INIT, &hdev->flags)) {
-- 
2.20.1

Re: [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP

[Marcel Holtmann]

Hi Mattijs,

> Some HCI devices which have the HCI_QUIRK_NON_PERSISTENT_SETUP [1]
> require a call to setup() to be ran after every open().
> 
> During the setup() stage, these devices expect the chip to acknowledge
> its setup() completion via vendor specific frames.
> 
> If userspace opens() such HCI device in HCI_USER_CHANNEL [2] mode,
> the vendor specific frames are never tranmitted to the driver, as
> they are filtered in hci_rx_work().
> 
> Allow HCI devices which have HCI_QUIRK_NON_PERSISTENT_SETUP to process
> frames if the HCI device is is HCI_INIT state.
> 
> [1] https://lore.kernel.org/patchwork/patch/965071/
> [2] https://www.spinics.net/lists/linux-bluetooth/msg37345.html
> 
> Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings")
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> Some more background on the change follows:
> 
> The Android bluetooth stack (Bluedroid) also has a HAL implementation
> which follows Linux's standard rfkill interface [1].
> 
> This implementation relies on the HCI_CHANNEL_USER feature to get
> exclusive access to the underlying bluetooth device.
> 
> When testing this along with the btkmtksdio driver, the
> chip appeared unresponsive when calling the following from userspace:
> 
>    struct sockaddr_hci addr;
>    int fd;
> 
>    fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
> 
>    memset(&addr, 0, sizeof(addr));
>    addr.hci_family = AF_BLUETOOTH;
>    addr.hci_dev = 0;
>    addr.hci_channel = HCI_CHANNEL_USER;
> 
>    bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device hangs
> 
> In the case of bluetooth drivers exposing QUIRK_NON_PERSISTENT_SETUP
> such as btmtksdio, setup() is called each multiple times.
> In particular, when userspace calls bind(), the setup() is called again
> and vendor specific commands might be send to re-initialize the chip.
> 
> Those commands are filtered out by hci_core in HCI_CHANNEL_USER mode,
> preventing setup() from completing successfully.
> 
> This has been tested on a 4.19 kernel based on Android Common Kernel.
> It has also been compile tested on bluetooth-next.
> 
> [1] https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/
> 
> net/bluetooth/hci_core.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 04bc79359a17..5f12e8574d54 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -4440,9 +4440,20 @@ static void hci_rx_work(struct work_struct *work)
> 			hci_send_to_sock(hdev, skb);
> 		}
> 
> +		/* If the device has been opened in HCI_USER_CHANNEL,
> +		 * the userspace has exclusive access to device.
> +		 * When HCI_QUIRK_NON_PERSISTENT_SETUP is set and
> +		 * device is HCI_INIT,  we still need to process
> +		 * the data packets to the driver in order
> +		 * to complete its setup().
> +		 */
> 		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> -			kfree_skb(skb);
> -			continue;
> +			if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,
> +				      &hdev->quirks) ||
> +			    !test_bit(HCI_INIT, &hdev->flags)) {
> +				kfree_skb(skb);
> +				continue;
> +			}
> 		}

	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
	    !test_bit(HCI_INIT, &hdev->flags)) {
		kfree_skb(skb);
		continue;
	}

Wouldn’t it be enough to just add a check for HCI_INIT to this. I mean it makes no difference if ->setup is repeated on each device open or not. We want to process event during HCI_INIT when in user channel mode.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP

[Mattijs Korpershoek]

Hi Marcel,

Marcel Holtmann <marcel@holtmann.org> writes:

> Hi Mattijs,
>
>> Some HCI devices which have the HCI_QUIRK_NON_PERSISTENT_SETUP 
>> [1]
>> require a call to setup() to be ran after every open().
>> 
>> During the setup() stage, these devices expect the chip to 
>> acknowledge
>> its setup() completion via vendor specific frames.
>> 
>> If userspace opens() such HCI device in HCI_USER_CHANNEL [2] 
>> mode,
>> the vendor specific frames are never tranmitted to the driver, 
>> as
>> they are filtered in hci_rx_work().
>> 
>> Allow HCI devices which have HCI_QUIRK_NON_PERSISTENT_SETUP to 
>> process
>> frames if the HCI device is is HCI_INIT state.
>> 
>> [1] https://lore.kernel.org/patchwork/patch/965071/
>> [2] https://www.spinics.net/lists/linux-bluetooth/msg37345.html
>> 
>> Fixes: 740011cfe948 ("Bluetooth: Add new quirk for 
>> non-persistent setup settings")
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> ---
>> Some more background on the change follows:
>> 
>> The Android bluetooth stack (Bluedroid) also has a HAL 
>> implementation
>> which follows Linux's standard rfkill interface [1].
>> 
>> This implementation relies on the HCI_CHANNEL_USER feature to 
>> get
>> exclusive access to the underlying bluetooth device.
>> 
>> When testing this along with the btkmtksdio driver, the
>> chip appeared unresponsive when calling the following from 
>> userspace:
>> 
>>    struct sockaddr_hci addr;
>>    int fd;
>> 
>>    fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
>> 
>>    memset(&addr, 0, sizeof(addr));
>>    addr.hci_family = AF_BLUETOOTH;
>>    addr.hci_dev = 0;
>>    addr.hci_channel = HCI_CHANNEL_USER;
>> 
>>    bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device 
>>    hangs
>> 
>> In the case of bluetooth drivers exposing 
>> QUIRK_NON_PERSISTENT_SETUP
>> such as btmtksdio, setup() is called each multiple times.
>> In particular, when userspace calls bind(), the setup() is 
>> called again
>> and vendor specific commands might be send to re-initialize the 
>> chip.
>> 
>> Those commands are filtered out by hci_core in HCI_CHANNEL_USER 
>> mode,
>> preventing setup() from completing successfully.
>> 
>> This has been tested on a 4.19 kernel based on Android Common 
>> Kernel.
>> It has also been compile tested on bluetooth-next.
>> 
>> [1] 
>> https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/
>> 
>> net/bluetooth/hci_core.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/bluetooth/hci_core.c 
>> b/net/bluetooth/hci_core.c
>> index 04bc79359a17..5f12e8574d54 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -4440,9 +4440,20 @@ static void hci_rx_work(struct 
>> work_struct *work)
>> 			hci_send_to_sock(hdev, skb);
>> 		}
>> 
>> +		/* If the device has been opened in HCI_USER_CHANNEL,
>> +		 * the userspace has exclusive access to device.
>> +		 * When HCI_QUIRK_NON_PERSISTENT_SETUP is set and
>> +		 * device is HCI_INIT,  we still need to process
>> +		 * the data packets to the driver in order
>> +		 * to complete its setup().
>> +		 */
>> 		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
>> -			kfree_skb(skb);
>> -			continue;
>> +			if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,
>> +				      &hdev->quirks) ||
>> +			    !test_bit(HCI_INIT, &hdev->flags)) {
>> +				kfree_skb(skb);
>> +				continue;
>> +			}
>> 		}
>
> 	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> 	    !test_bit(HCI_INIT, &hdev->flags)) {
> 		kfree_skb(skb);
> 		continue;
> 	}
>
> Wouldn’t it be enough to just add a check for HCI_INIT to this. 
> I mean it makes no difference if ->setup is repeated on each 
> device open or not. We want to process event during HCI_INIT 
> when in user channel mode.
Thank you for your review. You are right. We always want to 
process
events during HCI_INIT in user channel mode.

I'll send a v2

Regards,
Mattijs

>
> Regards
>
> Marcel

[PATCH v2] Bluetooth: hci_core: fix init for HCI_USER_CHANNEL

[Mattijs Korpershoek]

During the setup() stage, HCI device drivers expect the chip to
acknowledge its setup() completion via vendor specific frames.

If userspace opens() such HCI device in HCI_USER_CHANNEL [1] mode,
the vendor specific frames are never tranmitted to the driver, as
they are filtered in hci_rx_work().

Allow HCI devices which operate in HCI_USER_CHANNEL mode to receive
frames if the HCI device is is HCI_INIT state.

[1] https://www.spinics.net/lists/linux-bluetooth/msg37345.html

Fixes: 23500189d7e0 ("Bluetooth: Introduce new HCI socket channel for user operation")
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
Changelog:
v2:
* change test logic to transfer packets when in INIT phase
  for user channel mode as recommended by Marcel
* renamed patch from
  "Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP"

v1:
 * https://lkml.org/lkml/2019/10/3/2250

Some more background on the change follows:

The Android bluetooth stack (Bluedroid) also has a HAL implementation
which follows Linux's standard rfkill interface [1].

This implementation relies on the HCI_CHANNEL_USER feature to get
exclusive access to the underlying bluetooth device.

When testing this along with the btkmtksdio driver, the
chip appeared unresponsive when calling the following from userspace:

    struct sockaddr_hci addr;
    int fd;

    fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);

    memset(&addr, 0, sizeof(addr));
    addr.hci_family = AF_BLUETOOTH;
    addr.hci_dev = 0;
    addr.hci_channel = HCI_CHANNEL_USER;

    bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device hangs

In the case of bluetooth drivers exposing QUIRK_NON_PERSISTENT_SETUP
such as btmtksdio, setup() is called each multiple times.
In particular, when userspace calls bind(), the setup() is called again
and vendor specific commands might be send to re-initialize the chip.

Those commands are filtered out by hci_core in HCI_CHANNEL_USER mode,
preventing setup() from completing successfully.

This has been tested on a 4.19 kernel based on Android Common Kernel.
It has also been compile tested on bluetooth-next.

[1] https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/

 net/bluetooth/hci_core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b2559d4bed81..0cc9ce917222 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -4440,7 +4440,14 @@ static void hci_rx_work(struct work_struct *work)
 			hci_send_to_sock(hdev, skb);
 		}
 
-		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		/* If the device has been opened in HCI_USER_CHANNEL,
+		 * the userspace has exclusive access to device.
+		 * When device is HCI_INIT, we still need to process
+		 * the data packets to the driver in order
+		 * to complete its setup().
+		 */
+		if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+		    !test_bit(HCI_INIT, &hdev->flags)) {
 			kfree_skb(skb);
 			continue;
 		}
-- 
2.20.1

Re: [PATCH v2] Bluetooth: hci_core: fix init for HCI_USER_CHANNEL

[Marcel Holtmann]

Hi Mattijs,

> During the setup() stage, HCI device drivers expect the chip to
> acknowledge its setup() completion via vendor specific frames.
> 
> If userspace opens() such HCI device in HCI_USER_CHANNEL [1] mode,
> the vendor specific frames are never tranmitted to the driver, as
> they are filtered in hci_rx_work().
> 
> Allow HCI devices which operate in HCI_USER_CHANNEL mode to receive
> frames if the HCI device is is HCI_INIT state.
> 
> [1] https://www.spinics.net/lists/linux-bluetooth/msg37345.html
> 
> Fixes: 23500189d7e0 ("Bluetooth: Introduce new HCI socket channel for user operation")
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> Changelog:
> v2:
> * change test logic to transfer packets when in INIT phase
>  for user channel mode as recommended by Marcel
> * renamed patch from
>  "Bluetooth: hci_core: fix init with HCI_QUIRK_NON_PERSISTENT_SETUP"
> 
> v1:
> * https://lkml.org/lkml/2019/10/3/2250
> 
> Some more background on the change follows:
> 
> The Android bluetooth stack (Bluedroid) also has a HAL implementation
> which follows Linux's standard rfkill interface [1].
> 
> This implementation relies on the HCI_CHANNEL_USER feature to get
> exclusive access to the underlying bluetooth device.
> 
> When testing this along with the btkmtksdio driver, the
> chip appeared unresponsive when calling the following from userspace:
> 
>    struct sockaddr_hci addr;
>    int fd;
> 
>    fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
> 
>    memset(&addr, 0, sizeof(addr));
>    addr.hci_family = AF_BLUETOOTH;
>    addr.hci_dev = 0;
>    addr.hci_channel = HCI_CHANNEL_USER;
> 
>    bind(fd, (struct sockaddr *) &addr, sizeof(addr)); # device hangs
> 
> In the case of bluetooth drivers exposing QUIRK_NON_PERSISTENT_SETUP
> such as btmtksdio, setup() is called each multiple times.
> In particular, when userspace calls bind(), the setup() is called again
> and vendor specific commands might be send to re-initialize the chip.
> 
> Those commands are filtered out by hci_core in HCI_CHANNEL_USER mode,
> preventing setup() from completing successfully.
> 
> This has been tested on a 4.19 kernel based on Android Common Kernel.
> It has also been compile tested on bluetooth-next.
> 
> [1] https://android.googlesource.com/platform/system/bt/+/refs/heads/master/vendor_libs/linux/interface/
> 
> net/bluetooth/hci_core.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel