[PATCH 0/4] Bluetooth: hci_uart: various fixes

[PATCH 0/4] Bluetooth: hci_uart: various fixes

[Boris Brezillon]

Hi,

We recently faced some problems when using an BT uart chip interfaced
through the H5 proto (rtk_h5). Here are the logs of the 2 different
issues we had when closing the line discipline (actually, restoring
the previous one) [1][2]. I know the kernel is Tainted in those logs,
but after some investigations I found a few potential issues that might
explain what we're seeing.

Patches 1 and 2 are fixing 2 potential 'use after free' bugs: in some
(unlikely) cases the timer and work we try to cancel in the closing
path can be re-scheduled in our back, and since we're releasing the
memory region assigned to those elements at the end of the closing
procedure we can end-up with those invalid pointer exception when the
work or timer handler is called.

Note that this problem is pretty hard to reproduce, so I'm not sure
my patches are fixing all the racy paths.

Patches 3 and 4 are fixing potential issues that I didn't directly
face but may be worth fixing. Path 3 is fixing a potential double
free issue (proto->close() called twice if the hdev registration
failed). Patch 4 is making sure we don't loose some TX events.

Let me know what you think.

Thanks,

Boris

[1]http://code.bulix.org/8qtjly-105082
[2]http://code.bulix.org/qzur9n-105083


Boris Brezillon (4):
  Bluetooth: hci_ldisc: fix a race in the hdev closing path
  Bluetooth: hci_h5: fix a race in the closing path
  Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work()
  Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP
    events

 drivers/bluetooth/hci_h5.c    |  7 ++++++-
 drivers/bluetooth/hci_ldisc.c | 30 ++++++++++++++++++++++++++----
 drivers/bluetooth/hci_uart.h  |  1 +
 3 files changed, 33 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/4] Bluetooth: hci_ldisc: fix a race in the hdev closing path

[Boris Brezillon]

hci_uart_tty_close() is cancelling any pending write work, but some
hci_uart_proto implementations might re-schedule this work after its
cancellation (by calling hci_uart_tx_wakeup()).

Make sure the write work is not re-scheduled in our back while we're
closing the device.

We also cancel any pending init work and prevent the active one (if
any) from registering the hdev if the line discipline is being closed.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_ldisc.c | 15 ++++++++++++++-
 drivers/bluetooth/hci_uart.h  |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda97398c59a..de7f7f1f995c 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -130,7 +130,9 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
 
 	BT_DBG("");
 
-	schedule_work(&hu->write_work);
+	/* Don't schedule the work if the device is being closed. */
+	if (!test_bit(HCI_UART_CLOSING, &hu->flags))
+		schedule_work(&hu->write_work);
 
 	return 0;
 }
@@ -180,6 +182,11 @@ static void hci_uart_init_work(struct work_struct *work)
 	if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
 		return;
 
+	if (test_bit(HCI_UART_CLOSING, &hu->flags)) {
+		BT_DBG("HCI device is being closed, don't register it.");
+		return;
+	}
+
 	err = hci_register_dev(hu->hdev);
 	if (err < 0) {
 		BT_ERR("Can't register HCI device");
@@ -490,7 +497,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (hdev)
 		hci_uart_close(hdev);
 
+	/*
+	 * Set the closing bit to make sure nobody re-schedules the write work
+	 * in our back.
+	 */
+	set_bit(HCI_UART_CLOSING, &hu->flags);
 	cancel_work_sync(&hu->write_work);
+	cancel_work_sync(&hu->init_ready);
 
 	if (test_and_clear_bit(HCI_UART_PROTO_READY, &hu->flags)) {
 		if (hdev) {
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1d8152..c092ad6607e1 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -96,6 +96,7 @@ struct hci_uart {
 #define HCI_UART_PROTO_SET	0
 #define HCI_UART_REGISTERED	1
 #define HCI_UART_PROTO_READY	2
+#define HCI_UART_CLOSING	3
 
 /* TX states  */
 #define HCI_UART_SENDING	1
-- 
2.7.4

[PATCH 2/4] Bluetooth: hci_h5: fix a race in the closing path

[Boris Brezillon]

The H5 timer should not be rescheduled while we are closing the device,
otherwise it's defeating the del_timer_sync() call done in h5_close().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_h5.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 0879d64b1caf..d9720c59cffa 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -140,7 +140,12 @@ static void h5_timed_event(unsigned long arg)
 	}
 
 	if (h5->state != H5_ACTIVE) {
-		mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
+		/*
+		 * Do not re-schedule the timer if the device is being closed.
+		 */
+		if (!test_bit(HCI_UART_CLOSING, &hu->flags))
+			mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
+
 		goto wakeup;
 	}
 
-- 
2.7.4

[PATCH 3/4] Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work()

[Boris Brezillon]

This is done in hci_uart_tty_close() and can lead to double cleanup if
proto->close() is not tracking the closed/open state.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_ldisc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index de7f7f1f995c..27f73294edcb 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -190,9 +190,7 @@ static void hci_uart_init_work(struct work_struct *work)
 	err = hci_register_dev(hu->hdev);
 	if (err < 0) {
 		BT_ERR("Can't register HCI device");
-		hci_free_dev(hu->hdev);
-		hu->hdev = NULL;
-		hu->proto->close(hu);
+		return;
 	}
 
 	set_bit(HCI_UART_REGISTERED, &hu->flags);
-- 
2.7.4

[PATCH 4/4] Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP events

[Boris Brezillon]

The HCI_UART_TX_WAKEUP flag checking is racy and some HCI_UART_TX_WAKEUP
events can be lost.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 27f73294edcb..ee7b25f1c6ce 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -172,6 +172,17 @@ restart:
 		goto restart;
 
 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
+
+	/*
+	 * One last check to make sure hci_uart_tx_wakeup() did not set
+	 * HCI_UART_TX_WAKEUP while we where clearing HCI_UART_SENDING.
+	 * The work might have been scheduled by someone else in the
+	 * meantime, in this case we return directly.
+	 */
+	if (test_bit(HCI_UART_TX_WAKEUP, &hu->tx_state) &&
+	    !test_and_set_bit(HCI_UART_SENDING, &hu->tx_state))
+		goto restart;
+
 }
 
 static void hci_uart_init_work(struct work_struct *work)
-- 
2.7.4

Re: [PATCH 0/4] Bluetooth: hci_uart: various fixes

[Boris Brezillon]

On Fri, 19 Aug 2016 09:38:43 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi,
> 
> We recently faced some problems when using an BT uart chip interfaced
> through the H5 proto (rtk_h5). Here are the logs of the 2 different
> issues we had when closing the line discipline (actually, restoring
> the previous one) [1][2]. I know the kernel is Tainted in those logs,
> but after some investigations I found a few potential issues that might
> explain what we're seeing.
> 
> Patches 1 and 2 are fixing 2 potential 'use after free' bugs: in some
> (unlikely) cases the timer and work we try to cancel in the closing
> path can be re-scheduled in our back, and since we're releasing the
> memory region assigned to those elements at the end of the closing
> procedure we can end-up with those invalid pointer exception when the
> work or timer handler is called.
> 
> Note that this problem is pretty hard to reproduce, so I'm not sure
> my patches are fixing all the racy paths.
> 
> Patches 3 and 4 are fixing potential issues that I didn't directly
> face but may be worth fixing. Path 3 is fixing a potential double
> free issue (proto->close() called twice if the hdev registration
> failed). Patch 4 is making sure we don't loose some TX events.
> 
> Let me know what you think.

Ping.

> 
> Thanks,
> 
> Boris
> 
> [1]http://code.bulix.org/8qtjly-105082
> [2]http://code.bulix.org/qzur9n-105083
> 
> 
> Boris Brezillon (4):
>   Bluetooth: hci_ldisc: fix a race in the hdev closing path
>   Bluetooth: hci_h5: fix a race in the closing path
>   Bluetooth: hci_ldisc: don't release resources in hci_uart_init_work()
>   Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP
>     events
> 
>  drivers/bluetooth/hci_h5.c    |  7 ++++++-
>  drivers/bluetooth/hci_ldisc.c | 30 ++++++++++++++++++++++++++----
>  drivers/bluetooth/hci_uart.h  |  1 +
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 

Re: [PATCH 0/4] Bluetooth: hci_uart: various fixes

[Marcel Holtmann]

Hi Boris,

> We recently faced some problems when using an BT uart chip interfaced
> through the H5 proto (rtk_h5). Here are the logs of the 2 different
> issues we had when closing the line discipline (actually, restoring
> the previous one) [1][2]. I know the kernel is Tainted in those logs,
> but after some investigations I found a few potential issues that might
> explain what we're seeing.

while I can look through these patches, but I wonder when we are finally getting a full and proper RTK support that doesn't use these hackish hciattach code I have seen.

I mean the only thing userspace should be doing is attaching the line discipline and then everything else should run inside the kernel. Attaching the line discipline is the same as plugging in an USB dongle. Detaching it is the same as unplugging the dongle. That is how we should treat it. So for the lifetime of a system it should never detach. All the power control etc. should be done inside the kernel. Same as how we have done it for Broadcom and Intel devices.

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP events

[Marcel Holtmann]

Hi Boris,

> The HCI_UART_TX_WAKEUP flag checking is racy and some HCI_UART_TX_WAKEUP
> events can be lost.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 27f73294edcb..ee7b25f1c6ce 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -172,6 +172,17 @@ restart:
> 		goto restart;
> 
> 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
> +
> +	/*
> +	 * One last check to make sure hci_uart_tx_wakeup() did not set
> +	 * HCI_UART_TX_WAKEUP while we where clearing HCI_UART_SENDING.
> +	 * The work might have been scheduled by someone else in the
> +	 * meantime, in this case we return directly.
> +	 */
> +	if (test_bit(HCI_UART_TX_WAKEUP, &hu->tx_state) &&
> +	    !test_and_set_bit(HCI_UART_SENDING, &hu->tx_state))
> +		goto restart;
> +

I know this is correct, but I would actually make it visually different.

	if (test_bit(UART_TX_WAKEUP, ..) {
		/* comment goes here
		 */
		if (!test_and_set_bit(UART_SENDING, ..)
			goto restart;
	}

For me with a proper comment that is a lot easier to read and grok that it is correct.

Regards

Marcel

Re: [PATCH 1/4] Bluetooth: hci_ldisc: fix a race in the hdev closing path

[Marcel Holtmann]

Hi Boris,

> hci_uart_tty_close() is cancelling any pending write work, but some
> hci_uart_proto implementations might re-schedule this work after its
> cancellation (by calling hci_uart_tx_wakeup()).
> 
> Make sure the write work is not re-scheduled in our back while we're
> closing the device.
> 
> We also cancel any pending init work and prevent the active one (if
> any) from registering the hdev if the line discipline is being closed.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 15 ++++++++++++++-
> drivers/bluetooth/hci_uart.h  |  1 +
> 2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index dda97398c59a..de7f7f1f995c 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -130,7 +130,9 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
> 
> 	BT_DBG("");
> 
> -	schedule_work(&hu->write_work);
> +	/* Don't schedule the work if the device is being closed. */
> +	if (!test_bit(HCI_UART_CLOSING, &hu->flags))
> +		schedule_work(&hu->write_work);
> 
> 	return 0;
> }
> @@ -180,6 +182,11 @@ static void hci_uart_init_work(struct work_struct *work)
> 	if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> 		return;
> 
> +	if (test_bit(HCI_UART_CLOSING, &hu->flags)) {
> +		BT_DBG("HCI device is being closed, don't register it.");
> +		return;
> +	}
> +
> 	err = hci_register_dev(hu->hdev);
> 	if (err < 0) {
> 		BT_ERR("Can't register HCI device");
> @@ -490,7 +497,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> 	if (hdev)
> 		hci_uart_close(hdev);
> 
> +	/*
> +	 * Set the closing bit to make sure nobody re-schedules the write work
> +	 * in our back.
> +	 */

please use the network subsystem comment style here.

Regards

Marcel

Re: [PATCH 2/4] Bluetooth: hci_h5: fix a race in the closing path

[Marcel Holtmann]

Hi Boris,

> The H5 timer should not be rescheduled while we are closing the device,
> otherwise it's defeating the del_timer_sync() call done in h5_close().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/bluetooth/hci_h5.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 0879d64b1caf..d9720c59cffa 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -140,7 +140,12 @@ static void h5_timed_event(unsigned long arg)
> 	}
> 
> 	if (h5->state != H5_ACTIVE) {
> -		mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
> +		/*
> +		 * Do not re-schedule the timer if the device is being closed.
> +		 */

same here as the other patch. Use network subsystem comment style.

Regards

Marcel

Re: [PATCH 1/4] Bluetooth: hci_ldisc: fix a race in the hdev closing path

[Boris Brezillon]

On Tue, 30 Aug 2016 09:53:59 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Boris,
> 
> > hci_uart_tty_close() is cancelling any pending write work, but some
> > hci_uart_proto implementations might re-schedule this work after its
> > cancellation (by calling hci_uart_tx_wakeup()).
> > 
> > Make sure the write work is not re-scheduled in our back while we're
> > closing the device.
> > 
> > We also cancel any pending init work and prevent the active one (if
> > any) from registering the hdev if the line discipline is being closed.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/bluetooth/hci_ldisc.c | 15 ++++++++++++++-
> > drivers/bluetooth/hci_uart.h  |  1 +
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index dda97398c59a..de7f7f1f995c 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -130,7 +130,9 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
> > 
> > 	BT_DBG("");
> > 
> > -	schedule_work(&hu->write_work);
> > +	/* Don't schedule the work if the device is being closed. */
> > +	if (!test_bit(HCI_UART_CLOSING, &hu->flags))
> > +		schedule_work(&hu->write_work);
> > 
> > 	return 0;
> > }
> > @@ -180,6 +182,11 @@ static void hci_uart_init_work(struct work_struct *work)
> > 	if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> > 		return;
> > 
> > +	if (test_bit(HCI_UART_CLOSING, &hu->flags)) {
> > +		BT_DBG("HCI device is being closed, don't register it.");
> > +		return;
> > +	}
> > +
> > 	err = hci_register_dev(hu->hdev);
> > 	if (err < 0) {
> > 		BT_ERR("Can't register HCI device");
> > @@ -490,7 +497,13 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> > 	if (hdev)
> > 		hci_uart_close(hdev);
> > 
> > +	/*
> > +	 * Set the closing bit to make sure nobody re-schedules the write work
> > +	 * in our back.
> > +	 */  
> 
> please use the network subsystem comment style here.

Sure, I'll comply to the net subsystem coding style in my v2.

Re: [PATCH 4/4] Bluetooth: hci_ldisc: make sure we don't loose HCI_UART_TX_WAKEUP events

[Boris Brezillon]

On Tue, 30 Aug 2016 09:53:53 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Boris,
> 
> > The HCI_UART_TX_WAKEUP flag checking is racy and some HCI_UART_TX_WAKEUP
> > events can be lost.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index 27f73294edcb..ee7b25f1c6ce 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -172,6 +172,17 @@ restart:
> > 		goto restart;
> > 
> > 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
> > +
> > +	/*
> > +	 * One last check to make sure hci_uart_tx_wakeup() did not set
> > +	 * HCI_UART_TX_WAKEUP while we where clearing HCI_UART_SENDING.
> > +	 * The work might have been scheduled by someone else in the
> > +	 * meantime, in this case we return directly.
> > +	 */
> > +	if (test_bit(HCI_UART_TX_WAKEUP, &hu->tx_state) &&
> > +	    !test_and_set_bit(HCI_UART_SENDING, &hu->tx_state))
> > +		goto restart;
> > +  
> 
> I know this is correct, but I would actually make it visually different.
> 
> 	if (test_bit(UART_TX_WAKEUP, ..) {
> 		/* comment goes here
> 		 */
> 		if (!test_and_set_bit(UART_SENDING, ..)
> 			goto restart;
> 	}
> 
> For me with a proper comment that is a lot easier to read and grok that it is correct.

Sure, I'll address that.

Re: [PATCH 0/4] Bluetooth: hci_uart: various fixes

[Boris Brezillon]

Hi Marcel,

On Tue, 30 Aug 2016 09:48:13 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Boris,
> 
> > We recently faced some problems when using an BT uart chip interfaced
> > through the H5 proto (rtk_h5). Here are the logs of the 2 different
> > issues we had when closing the line discipline (actually, restoring
> > the previous one) [1][2]. I know the kernel is Tainted in those logs,
> > but after some investigations I found a few potential issues that might
> > explain what we're seeing.  
> 
> while I can look through these patches, but I wonder when we are finally getting a full and proper RTK support that doesn't use these hackish hciattach code I have seen.
> 
> I mean the only thing userspace should be doing is attaching the line discipline and then everything else should run inside the kernel. Attaching the line discipline is the same as plugging in an USB dongle. Detaching it is the same as unplugging the dongle. That is how we should treat it. So for the lifetime of a system it should never detach. All the power control etc. should be done inside the kernel. Same as how we have done it for Broadcom and Intel devices.

Well, I'm completely new to the bluetooth stack, and while the
'hciattach/in-kernel driver' interaction does seem weird to me, I
definitely can't tell whether it's good or bad.

I just sent those patches because I was facing kernel panics.

Still, I don't get your argument about allowing a user to attach the
line discipline, but preventing him from detaching it. The races exist,
so even if you decide that the line discipline cannot be detached, how
do you plan to handle module unloading (maybe you don't want to allow
that either)?

One last comment. Even after applying those fixes I'm still facing
panics [1], so there are probably other racy portions in the code.

Anyway, tanks for your review.

Boris

[1]http://code.bulix.org/b5td9x-105502

Re: [PATCH 0/4] Bluetooth: hci_uart: various fixes

[Marcel Holtmann]

Hi Boris,

>>> We recently faced some problems when using an BT uart chip interfaced
>>> through the H5 proto (rtk_h5). Here are the logs of the 2 different
>>> issues we had when closing the line discipline (actually, restoring
>>> the previous one) [1][2]. I know the kernel is Tainted in those logs,
>>> but after some investigations I found a few potential issues that might
>>> explain what we're seeing.  
>> 
>> while I can look through these patches, but I wonder when we are finally getting a full and proper RTK support that doesn't use these hackish hciattach code I have seen.
>> 
>> I mean the only thing userspace should be doing is attaching the line discipline and then everything else should run inside the kernel. Attaching the line discipline is the same as plugging in an USB dongle. Detaching it is the same as unplugging the dongle. That is how we should treat it. So for the lifetime of a system it should never detach. All the power control etc. should be done inside the kernel. Same as how we have done it for Broadcom and Intel devices.
> 
> Well, I'm completely new to the bluetooth stack, and while the
> 'hciattach/in-kernel driver' interaction does seem weird to me, I
> definitely can't tell whether it's good or bad.

if it is the hciattach_rtk.c that uses kernel struct sk_buff in userspace, then it is hackish like no tomorrow. I am still surprised that this works for anybody.

> I just sent those patches because I was facing kernel panics.
> 
> Still, I don't get your argument about allowing a user to attach the
> line discipline, but preventing him from detaching it. The races exist,
> so even if you decide that the line discipline cannot be detached, how
> do you plan to handle module unloading (maybe you don't want to allow
> that either)?

The point is that btattach (which we like to use now) is suppose to be plug/unplug action. You should be able to keep the line discipline attached over hciconfig hci0 up/down interaction. Meaning the kernel can do its proper work without having to resort to detach the line discipline to bring the device down or in low power mode.

> One last comment. Even after applying those fixes I'm still facing
> panics [1], so there are probably other racy portions in the code.

I am not fully surprised. It would be great if you have time to add full RTK support to the kernel side that just uses btattach in userspace to open the TTY and attach the line discipline. And then drive vendor setup + firmware loading from the kernel.

Regards

Marcel