[PATCH] bluetooth: fix shutdown on SCO sockets

[PATCH] bluetooth: fix shutdown on SCO sockets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

shutdown should wait for SCO link to be properly disconnected before
detroying the socket, otherwise an application using the socket may
assume link is properly disconnected before it really happens which
can be a problem when e.g synchronizing profile switch.

Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
---
 net/bluetooth/sco.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 42fdffd..94954c7 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -369,6 +369,15 @@ static void __sco_sock_close(struct sock *sk)
 
 	case BT_CONNECTED:
 	case BT_CONFIG:
+		if (sco_pi(sk)->conn) {
+			sk->sk_state = BT_DISCONN;
+			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
+			hci_conn_put(sco_pi(sk)->conn->hcon);
+			sco_pi(sk)->conn = NULL;
+		} else
+			sco_chan_del(sk, ECONNRESET);
+		break;
+
 	case BT_CONNECT:
 	case BT_DISCONN:
 		sco_chan_del(sk, ECONNRESET);
-- 
1.7.1

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Gustavo F. Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:

> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> 
> shutdown should wait for SCO link to be properly disconnected before
> detroying the socket, otherwise an application using the socket may
> assume link is properly disconnected before it really happens which
> can be a problem when e.g synchronizing profile switch.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

I applied it, but in bluetooth-next. Let's see its behaviour there and if no
problems show up we can move it to bluetooth-2.6

> ---
>  net/bluetooth/sco.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 42fdffd..94954c7 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -369,6 +369,15 @@ static void __sco_sock_close(struct sock *sk)
>  
>  	case BT_CONNECTED:
>  	case BT_CONFIG:
> +		if (sco_pi(sk)->conn) {
> +			sk->sk_state = BT_DISCONN;
> +			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
> +			hci_conn_put(sco_pi(sk)->conn->hcon);
> +			sco_pi(sk)->conn = NULL;
> +		} else
> +			sco_chan_del(sk, ECONNRESET);
> +		break;
> +
>  	case BT_CONNECT:
>  	case BT_DISCONN:
>  		sco_chan_del(sk, ECONNRESET);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Luiz Augusto von Dentz]

Hi Gustavo,

On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
>
>> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>>
>> shutdown should wait for SCO link to be properly disconnected before
>> detroying the socket, otherwise an application using the socket may
>> assume link is properly disconnected before it really happens which
>> can be a problem when e.g synchronizing profile switch.
>>
>> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> I applied it, but in bluetooth-next. Let's see its behaviour there and if no
> problems show up we can move it to bluetooth-2.6

I tested this against Nokia BH-504 and Sony Ericsson W600, both have
problem when switching from hfp to a2dp where the avdtp start is sent
before SCO is fully disconnected, this patch fixes with those
headsets.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Gustavo F. Padovan]

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-17 20:26:53 +0300]:

> Hi Gustavo,
> 
> On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Luiz,
> >
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
> >
> >> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >>
> >> shutdown should wait for SCO link to be properly disconnected before
> >> detroying the socket, otherwise an application using the socket may
> >> assume link is properly disconnected before it really happens which
> >> can be a problem when e.g synchronizing profile switch.
> >>
> >> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >
> > I applied it, but in bluetooth-next. Let's see its behaviour there and if no
> > problems show up we can move it to bluetooth-2.6
> 
> I tested this against Nokia BH-504 and Sony Ericsson W600, both have
> problem when switching from hfp to a2dp where the avdtp start is sent
> before SCO is fully disconnected, this patch fixes with those
> headsets.

Ok, I also pushed it to bluetooth-2.6.


-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Luiz Augusto von Dentz]

Hi Gustavo,

On Mon, Apr 18, 2011 at 8:56 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-17 20:26:53 +0300]:
>
>> Hi Gustavo,
>>
>> On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>> > Hi Luiz,
>> >
>> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
>> >
>> >> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>> >>
>> >> shutdown should wait for SCO link to be properly disconnected before
>> >> detroying the socket, otherwise an application using the socket may
>> >> assume link is properly disconnected before it really happens which
>> >> can be a problem when e.g synchronizing profile switch.
>> >>
>> >> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>> >
>> > I applied it, but in bluetooth-next. Let's see its behaviour there and if no
>> > problems show up we can move it to bluetooth-2.6
>>
>> I tested this against Nokia BH-504 and Sony Ericsson W600, both have
>> problem when switching from hfp to a2dp where the avdtp start is sent
>> before SCO is fully disconnected, this patch fixes with those
>> headsets.
>
> Ok, I also pushed it to bluetooth-2.6.


Apparently this cause a regression, since we set conn to NULL but an
application may not wait for shutdown to complete and call
close/release which will cause sco_chan_del to be called which destroy
the socket without resetting conn->sk to NULL so on disconn_cfm it
will access invalid memory.

To fix this what about the following:

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 94954c7..cb4fb78 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -373,7 +373,7 @@ static void __sco_sock_close(struct sock *sk)
                        sk->sk_state = BT_DISCONN;
                        sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
                        hci_conn_put(sco_pi(sk)->conn->hcon);
-                       sco_pi(sk)->conn = NULL;
+                       sco_pi(sk)->conn->hcon = NULL;
                } else
                        sco_chan_del(sk, ECONNRESET);
                break;
@@ -828,7 +828,9 @@ static void sco_chan_del(struct sock *sk, int err)
                conn->sk = NULL;
                sco_pi(sk)->conn = NULL;
                sco_conn_unlock(conn);
-               hci_conn_put(conn->hcon);
+
+               if (conn->hcon)
+                       hci_conn_put(conn->hcon);
        }

        sk->sk_state = BT_CLOSED;

I tested it with code waiting for shutdown to complete, watch for
POLL_ERR, and without waiting and it seems to work fine, finally.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Gustavo F. Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-05-05 17:50:53 +0300]:

> Hi Gustavo,
> 
> On Mon, Apr 18, 2011 at 8:56 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-17 20:26:53 +0300]:
> >
> >> Hi Gustavo,
> >>
> >> On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
> >> <padovan@profusion.mobi> wrote:
> >> > Hi Luiz,
> >> >
> >> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
> >> >
> >> >> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >> >>
> >> >> shutdown should wait for SCO link to be properly disconnected before
> >> >> detroying the socket, otherwise an application using the socket may
> >> >> assume link is properly disconnected before it really happens which
> >> >> can be a problem when e.g synchronizing profile switch.
> >> >>
> >> >> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >> >
> >> > I applied it, but in bluetooth-next. Let's see its behaviour there and if no
> >> > problems show up we can move it to bluetooth-2.6
> >>
> >> I tested this against Nokia BH-504 and Sony Ericsson W600, both have
> >> problem when switching from hfp to a2dp where the avdtp start is sent
> >> before SCO is fully disconnected, this patch fixes with those
> >> headsets.
> >
> > Ok, I also pushed it to bluetooth-2.6.
> 
> 
> Apparently this cause a regression, since we set conn to NULL but an
> application may not wait for shutdown to complete and call
> close/release which will cause sco_chan_del to be called which destroy
> the socket without resetting conn->sk to NULL so on disconn_cfm it
> will access invalid memory.
> 
> To fix this what about the following:
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 94954c7..cb4fb78 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -373,7 +373,7 @@ static void __sco_sock_close(struct sock *sk)
>                         sk->sk_state = BT_DISCONN;
>                         sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
>                         hci_conn_put(sco_pi(sk)->conn->hcon);
> -                       sco_pi(sk)->conn = NULL;
> +                       sco_pi(sk)->conn->hcon = NULL;
>                 } else
>                         sco_chan_del(sk, ECONNRESET);
>                 break;
> @@ -828,7 +828,9 @@ static void sco_chan_del(struct sock *sk, int err)
>                 conn->sk = NULL;
>                 sco_pi(sk)->conn = NULL;
>                 sco_conn_unlock(conn);
> -               hci_conn_put(conn->hcon);
> +
> +               if (conn->hcon)
> +                       hci_conn_put(conn->hcon);

I think first we need to revert the patch on linus' tree. There isn't time to
a proper fix and test. It may have introduced other bugs too. I don't wanna
take this risk.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Luiz Augusto von Dentz]

Hi Gustavo,

On Wed, May 11, 2011 at 8:09 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-05-05 17:50:53 +0300]:
>
>> Hi Gustavo,
>>
>> On Mon, Apr 18, 2011 at 8:56 PM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-17 20:26:53 +0300]:
>> >
>> >> Hi Gustavo,
>> >>
>> >> On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
>> >> <padovan@profusion.mobi> wrote:
>> >> > Hi Luiz,
>> >> >
>> >> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
>> >> >
>> >> >> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>> >> >>
>> >> >> shutdown should wait for SCO link to be properly disconnected before
>> >> >> detroying the socket, otherwise an application using the socket may
>> >> >> assume link is properly disconnected before it really happens which
>> >> >> can be a problem when e.g synchronizing profile switch.
>> >> >>
>> >> >> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>> >> >
>> >> > I applied it, but in bluetooth-next. Let's see its behaviour there and if no
>> >> > problems show up we can move it to bluetooth-2.6
>> >>
>> >> I tested this against Nokia BH-504 and Sony Ericsson W600, both have
>> >> problem when switching from hfp to a2dp where the avdtp start is sent
>> >> before SCO is fully disconnected, this patch fixes with those
>> >> headsets.
>> >
>> > Ok, I also pushed it to bluetooth-2.6.
>>
>>
>> Apparently this cause a regression, since we set conn to NULL but an
>> application may not wait for shutdown to complete and call
>> close/release which will cause sco_chan_del to be called which destroy
>> the socket without resetting conn->sk to NULL so on disconn_cfm it
>> will access invalid memory.
>>
>> To fix this what about the following:
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index 94954c7..cb4fb78 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -373,7 +373,7 @@ static void __sco_sock_close(struct sock *sk)
>>                         sk->sk_state = BT_DISCONN;
>>                         sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
>>                         hci_conn_put(sco_pi(sk)->conn->hcon);
>> -                       sco_pi(sk)->conn = NULL;
>> +                       sco_pi(sk)->conn->hcon = NULL;
>>                 } else
>>                         sco_chan_del(sk, ECONNRESET);
>>                 break;
>> @@ -828,7 +828,9 @@ static void sco_chan_del(struct sock *sk, int err)
>>                 conn->sk = NULL;
>>                 sco_pi(sk)->conn = NULL;
>>                 sco_conn_unlock(conn);
>> -               hci_conn_put(conn->hcon);
>> +
>> +               if (conn->hcon)
>> +                       hci_conn_put(conn->hcon);
>
> I think first we need to revert the patch on linus' tree. There isn't time to
> a proper fix and test. It may have introduced other bugs too. I don't wanna
> take this risk.

Ville and I tested the suggested changes and see no regression
anymore, so I can either create a new patch or update the other in
case we revert it.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Gustavo F. Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-05-11 23:49:51 +0300]:

> Hi Gustavo,
> 
> On Wed, May 11, 2011 at 8:09 PM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Hi Luiz,
> >
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-05-05 17:50:53 +0300]:
> >
> >> Hi Gustavo,
> >>
> >> On Mon, Apr 18, 2011 at 8:56 PM, Gustavo F. Padovan
> >> <padovan@profusion.mobi> wrote:
> >> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-17 20:26:53 +0300]:
> >> >
> >> >> Hi Gustavo,
> >> >>
> >> >> On Fri, Apr 15, 2011 at 9:58 PM, Gustavo F. Padovan
> >> >> <padovan@profusion.mobi> wrote:
> >> >> > Hi Luiz,
> >> >> >
> >> >> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-04-08 17:10:41 +0300]:
> >> >> >
> >> >> >> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >> >> >>
> >> >> >> shutdown should wait for SCO link to be properly disconnected before
> >> >> >> detroying the socket, otherwise an application using the socket may
> >> >> >> assume link is properly disconnected before it really happens which
> >> >> >> can be a problem when e.g synchronizing profile switch.
> >> >> >>
> >> >> >> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> >> >> >
> >> >> > I applied it, but in bluetooth-next. Let's see its behaviour there and if no
> >> >> > problems show up we can move it to bluetooth-2.6
> >> >>
> >> >> I tested this against Nokia BH-504 and Sony Ericsson W600, both have
> >> >> problem when switching from hfp to a2dp where the avdtp start is sent
> >> >> before SCO is fully disconnected, this patch fixes with those
> >> >> headsets.
> >> >
> >> > Ok, I also pushed it to bluetooth-2.6.
> >>
> >>
> >> Apparently this cause a regression, since we set conn to NULL but an
> >> application may not wait for shutdown to complete and call
> >> close/release which will cause sco_chan_del to be called which destroy
> >> the socket without resetting conn->sk to NULL so on disconn_cfm it
> >> will access invalid memory.
> >>
> >> To fix this what about the following:
> >>
> >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> >> index 94954c7..cb4fb78 100644
> >> --- a/net/bluetooth/sco.c
> >> +++ b/net/bluetooth/sco.c
> >> @@ -373,7 +373,7 @@ static void __sco_sock_close(struct sock *sk)
> >>                         sk->sk_state = BT_DISCONN;
> >>                         sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
> >>                         hci_conn_put(sco_pi(sk)->conn->hcon);
> >> -                       sco_pi(sk)->conn = NULL;
> >> +                       sco_pi(sk)->conn->hcon = NULL;
> >>                 } else
> >>                         sco_chan_del(sk, ECONNRESET);
> >>                 break;
> >> @@ -828,7 +828,9 @@ static void sco_chan_del(struct sock *sk, int err)
> >>                 conn->sk = NULL;
> >>                 sco_pi(sk)->conn = NULL;
> >>                 sco_conn_unlock(conn);
> >> -               hci_conn_put(conn->hcon);
> >> +
> >> +               if (conn->hcon)
> >> +                       hci_conn_put(conn->hcon);
> >
> > I think first we need to revert the patch on linus' tree. There isn't time to
> > a proper fix and test. It may have introduced other bugs too. I don't wanna
> > take this risk.
> 
> Ville and I tested the suggested changes and see no regression
> anymore, so I can either create a new patch or update the other in
> case we revert it.

So update the patch and resend, I already asked Linus to revert it.

-- 
Gustavo F. Padovan
http://profusion.mobi

[PATCH] bluetooth: fix shutdown on SCO sockets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

shutdown should wait for SCO link to be properly disconnected before
detroying the socket, otherwise an application using the socket may
assume link is properly disconnected before it really happens which
can be a problem when e.g synchronizing profile switch.

Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
---
 net/bluetooth/sco.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 42fdffd..cb4fb78 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -369,6 +369,15 @@ static void __sco_sock_close(struct sock *sk)
 
 	case BT_CONNECTED:
 	case BT_CONFIG:
+		if (sco_pi(sk)->conn) {
+			sk->sk_state = BT_DISCONN;
+			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
+			hci_conn_put(sco_pi(sk)->conn->hcon);
+			sco_pi(sk)->conn->hcon = NULL;
+		} else
+			sco_chan_del(sk, ECONNRESET);
+		break;
+
 	case BT_CONNECT:
 	case BT_DISCONN:
 		sco_chan_del(sk, ECONNRESET);
@@ -819,7 +828,9 @@ static void sco_chan_del(struct sock *sk, int err)
 		conn->sk = NULL;
 		sco_pi(sk)->conn = NULL;
 		sco_conn_unlock(conn);
-		hci_conn_put(conn->hcon);
+
+		if (conn->hcon)
+			hci_conn_put(conn->hcon);
 	}
 
 	sk->sk_state = BT_CLOSED;
-- 
1.7.1

Re: [PATCH] bluetooth: fix shutdown on SCO sockets

[Gustavo F. Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-05-12 11:13:15 +0300]:

> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> 
> shutdown should wait for SCO link to be properly disconnected before
> detroying the socket, otherwise an application using the socket may
> assume link is properly disconnected before it really happens which
> can be a problem when e.g synchronizing profile switch.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> ---
>  net/bluetooth/sco.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)

Applied, thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi