Closing HCI_CHANNEL_USER socket doesn't close HCI device

Closing HCI_CHANNEL_USER socket doesn't close HCI device

[Simon Fels]

Hey all,

I just had a discussion with Johan Hedberg on IRC about problems I am experiencing with
the hci-tester tool.

Basically what I do is:

$ sudo stop bluetooth
$ sudo hciconfig hci0 down

Verifying hci0 device is down

$ sudo hciconfig -a

Now running the hci-tester and getting

$ sudo ./hci-tester

Reset - init
Reset - setup
Reset - setup complete
Reset - run
Reset - test passed
Reset - teardown
Reset - teardown complete
Reset - done

Read Local Version Information - init
Failed to setup upper tester user channel
Read Local Version Information - pre setup failed
Read Local Version Information - done

Read Local Supported Commands - init
Failed to setup upper tester user channel
Read Local Supported Commands - pre setup failed
Read Local Supported Commands - done

Read Local Supported Features - init
Failed to setup upper tester user channel
Read Local Supported Features - pre setup failed
Read Local Supported Features - done

Read Local Extended Features - init
Failed to setup upper tester user channel
Read Local Extended Features - pre setup failed
Read Local Extended Features - done

Read Buffer Size - init
Failed to setup upper tester user channel
Read Buffer Size - pre setup failed
Read Buffer Size - done

[...]

I stripped all the last lines of the output. More detailed is at
http://paste.ubuntu.com/12213127/. The problem here seems to be
that after running the first test the HCI device stays up even
when the HCI_CHANNEL_USER socket gets closed.

Johan pointed out that the problem seems to be that the
HCI_CHANNEL_USER flag doesn't get cleared before calling hci_dev_close
which actively checks for this one being set and if it is it fails
right away to bring down the device.

I am proposing the attached patch here with keeping in mind that
this might still introduce problems somewhere else but wanted to
put this out for discussion.

regards,
Simon

[PATCH] Bluetooth: bring device down when closing HCI_CHANNEL_USER socket

[Simon Fels]

When a HCI_CHANNEL_USER socket is open and should be closed we first try to close the
device which will fail as hci_dev_close checks for HCI_CHANNEL_USER being set and if it is
it just fails to close the device. Clearing the HCI_CHANNEL_USER flag first before trying
to close the device fixes this.

Signed-off-by: Simon Fels <simon.fels@canonical.com>
---
 backports/net/bluetooth/hci_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
index 9a2732f..c84c13e 100644
--- a/backports/net/bluetooth/hci_sock.c
+++ b/backports/net/bluetooth/hci_sock.c
@@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
 
 	if (hdev) {
 		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
-			hci_dev_close(hdev->id);
 			hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
+			hci_dev_close(hdev->id);
 			mgmt_index_added(hdev);
 		}
 
-- 
2.1.4

Re: [PATCH] Bluetooth: bring device down when closing HCI_CHANNEL_USER socket

[Johan Hedberg]

Hi Simon,

On Fri, Aug 28, 2015, Simon Fels wrote:
> When a HCI_CHANNEL_USER socket is open and should be closed we first
> try to close the device which will fail as hci_dev_close checks for
> HCI_CHANNEL_USER being set and if it is it just fails to close the
> device. Clearing the HCI_CHANNEL_USER flag first before trying to
> close the device fixes this.
> 
> Signed-off-by: Simon Fels <simon.fels@canonical.com>
> ---
>  backports/net/bluetooth/hci_sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
> index 9a2732f..c84c13e 100644
> --- a/backports/net/bluetooth/hci_sock.c
> +++ b/backports/net/bluetooth/hci_sock.c
> @@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
>  
>  	if (hdev) {
>  		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
> -			hci_dev_close(hdev->id);
>  			hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> +			hci_dev_close(hdev->id);
>  			mgmt_index_added(hdev);
>  		}

Thanks for catching this and coming up with a patch proposal!

My main concern is that there's code within the hci_dev_close() path
that assumes HCI_USER_CHANNEL may be set. E.g. this in
hci_dev_do_close() (which hci_dev_close calls):

	if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
	    !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
	    test_bit(HCI_UP, &hdev->flags)) {
		/* Execute vendor specific shutdown routine */
		if (hdev->shutdown)
			hdev->shutdown(hdev);
	}

With your change the hdev->shutdown() callback would get called which
seems to be what the above if-statement tries to protect against.

Johan

Re: [PATCH] Bluetooth: bring device down when closing HCI_CHANNEL_USER socket

[Marcel Holtmann]

Hi Johan,

>> When a HCI_CHANNEL_USER socket is open and should be closed we first
>> try to close the device which will fail as hci_dev_close checks for
>> HCI_CHANNEL_USER being set and if it is it just fails to close the
>> device. Clearing the HCI_CHANNEL_USER flag first before trying to
>> close the device fixes this.
>> 
>> Signed-off-by: Simon Fels <simon.fels@canonical.com>
>> ---
>> backports/net/bluetooth/hci_sock.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
>> index 9a2732f..c84c13e 100644
>> --- a/backports/net/bluetooth/hci_sock.c
>> +++ b/backports/net/bluetooth/hci_sock.c
>> @@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
>> 
>> 	if (hdev) {
>> 		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
>> -			hci_dev_close(hdev->id);
>> 			hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
>> +			hci_dev_close(hdev->id);
>> 			mgmt_index_added(hdev);
>> 		}
> 
> Thanks for catching this and coming up with a patch proposal!
> 
> My main concern is that there's code within the hci_dev_close() path
> that assumes HCI_USER_CHANNEL may be set. E.g. this in
> hci_dev_do_close() (which hci_dev_close calls):
> 
> 	if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> 	    !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> 	    test_bit(HCI_UP, &hdev->flags)) {
> 		/* Execute vendor specific shutdown routine */
> 		if (hdev->shutdown)
> 			hdev->shutdown(hdev);
> 	}
> 
> With your change the hdev->shutdown() callback would get called which
> seems to be what the above if-statement tries to protect against.

commit 9380f9eacfbbee701daa416edd6625efcd3e29e1 flipped them around. And I was worried about that at that time, but since I forgot to add a comment, I could not remember why I ordered them in that way in the first place. I just had a hunch that I did it for a reason.

So we might need to look at this one again and make sure this time we get this right and comment on it.

Regards

Marcel