[PATCH 0/2] Fix usage of sk_sndtimeo value in l2cap

[PATCH 0/2] Fix usage of sk_sndtimeo value in l2cap

[Andrzej Kaczmarek]

Hi,

It seems sk_sndtimeo is initialized incorrectly in l2cap_sock.c to
miliseconds, this will cause invalid value to be set or read using
SO_SNDTIMEO sockopt since it is assumed this value is specified in
jiffies.

Initial value of sk_sndtimeo was changed in 6be6b11f006840ba7d8d4b
and I belive this was because of regression introduced by commit
942ecc9c4643db5ce071562e0a23f99464d6b461.

My two patches revert mentioned commit and solve issue in proper way.
Regression introduced by 942ecc9c4643db5ce071562e0a23f99464d6b461 is
already fixed by other commit.


Andrzej Kaczmarek (2):
  Buetooth: Revert "Fixed wrong L2CAP Sock timer value"
  Bluetooth: Fix usage of sk_sndtimeo value

 net/bluetooth/l2cap_core.c |    8 ++++----
 net/bluetooth/l2cap_sock.c |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
on behalf of ST-Ericsson

[PATCH 1/2] Buetooth: Revert "Fixed wrong L2CAP Sock timer value"

[Andrzej Kaczmarek]

This reverts commit 6be6b11f006840ba7d8d4b959b3fa0c522f8468a.

Commit changed sk_sndtimeo value unit to miliseconds while jiffies
should be used. This will cause invalid behaviour if timeout is
read or set via SO_SNDTIMEO sockopt which uses jiffies.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 net/bluetooth/l2cap_sock.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 664762e..ae7dbe9 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1039,7 +1039,7 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock, int p
 	INIT_LIST_HEAD(&bt_sk(sk)->accept_q);
 
 	sk->sk_destruct = l2cap_sock_destruct;
-	sk->sk_sndtimeo = L2CAP_CONN_TIMEOUT;
+	sk->sk_sndtimeo = msecs_to_jiffies(L2CAP_CONN_TIMEOUT);
 
 	sock_reset_flag(sk, SOCK_ZAPPED);
 
-- 
on behalf of ST-Ericsson

[PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Andrzej Kaczmarek]

sk_sndtimeo timeout value is specified in jiffes and should be
converted to miliseconds when used as input to __set_chan_timer.

Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
---
 net/bluetooth/l2cap_core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f850684..3b0f807 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -446,7 +446,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 		if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED &&
 					conn->hcon->type == ACL_LINK) {
 			__clear_chan_timer(chan);
-			__set_chan_timer(chan, sk->sk_sndtimeo);
+			__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
 			l2cap_send_disconn_req(conn, chan, reason);
 		} else
 			l2cap_chan_del(chan, reason);
@@ -899,7 +899,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	__l2cap_chan_add(conn, chan);
 
-	__set_chan_timer(chan, sk->sk_sndtimeo);
+	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
 
 	l2cap_state_change(chan, BT_CONNECTED);
 	parent->sk_data_ready(parent, 0);
@@ -1176,7 +1176,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan)
 	l2cap_chan_add(conn, chan);
 
 	l2cap_state_change(chan, BT_CONNECT);
-	__set_chan_timer(chan, sk->sk_sndtimeo);
+	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
 
 	if (hcon->state == BT_CONNECTED) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
@@ -2601,7 +2601,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	dcid = chan->scid;
 
-	__set_chan_timer(chan, sk->sk_sndtimeo);
+	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
 
 	chan->ident = cmd->ident;
 
-- 
on behalf of ST-Ericsson

Re: [PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Andrei Emeltchenko]

Hi Andrzej,

On Thu, Nov 10, 2011 at 10:37:25AM +0100, Andrzej Kaczmarek wrote:
> sk_sndtimeo timeout value is specified in jiffes and should be
> converted to miliseconds when used as input to __set_chan_timer.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
>  net/bluetooth/l2cap_core.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index f850684..3b0f807 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -446,7 +446,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>  		if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED &&
>  					conn->hcon->type == ACL_LINK) {
>  			__clear_chan_timer(chan);
> -			__set_chan_timer(chan, sk->sk_sndtimeo);
> +			__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));

Then __set_chan_timer do reverse conversion:
mod_timer(timer, jiffies + msecs_to_jiffies(timeout))

Look ugly :-(

Best regards 
Andrei Emeltchenko 

>  			l2cap_send_disconn_req(conn, chan, reason);
>  		} else
>  			l2cap_chan_del(chan, reason);
> @@ -899,7 +899,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  
>  	__l2cap_chan_add(conn, chan);
>  
> -	__set_chan_timer(chan, sk->sk_sndtimeo);
> +	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
>  
>  	l2cap_state_change(chan, BT_CONNECTED);
>  	parent->sk_data_ready(parent, 0);
> @@ -1176,7 +1176,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan)
>  	l2cap_chan_add(conn, chan);
>  
>  	l2cap_state_change(chan, BT_CONNECT);
> -	__set_chan_timer(chan, sk->sk_sndtimeo);
> +	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
>  
>  	if (hcon->state == BT_CONNECTED) {
>  		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> @@ -2601,7 +2601,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  
>  	dcid = chan->scid;
>  
> -	__set_chan_timer(chan, sk->sk_sndtimeo);
> +	__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
>  
>  	chan->ident = cmd->ident;
>  
> -- 
> on behalf of ST-Ericsson
> 
> --
> 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

Re: [PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Andrzej Kaczmarek]

Hi Andrei,

On 10.11.2011 10:57, Andrei Emeltchenko wrote:
> Hi Andrzej,
>
> On Thu, Nov 10, 2011 at 10:37:25AM +0100, Andrzej Kaczmarek wrote:
>> sk_sndtimeo timeout value is specified in jiffes and should be
>> converted to miliseconds when used as input to __set_chan_timer.
>>
>> Signed-off-by: Andrzej Kaczmarek<andrzej.kaczmarek@tieto.com>
>> ---
>>   net/bluetooth/l2cap_core.c |    8 ++++----
>>   1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index f850684..3b0f807 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -446,7 +446,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>>   		if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED&&
>>   					conn->hcon->type == ACL_LINK) {
>>   			__clear_chan_timer(chan);
>> -			__set_chan_timer(chan, sk->sk_sndtimeo);
>> +			__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
>
> Then __set_chan_timer do reverse conversion:
> mod_timer(timer, jiffies + msecs_to_jiffies(timeout))
>
> Look ugly :-(

Well, I agree. But problem here is that __set_chan_timer has miliseconds 
as input and sk_sndtimeo has to be specified in jiffies thus ugly double 
conversion.

But what about adding __set_chan_timer_jiffies macro and use it here?

BR,
Andrzej

Re: [PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Andrei Emeltchenko]

Hi Andrzej,

On Thu, Nov 10, 2011 at 11:13:53AM +0100, Andrzej Kaczmarek wrote:
> Hi Andrei,
> 
> On 10.11.2011 10:57, Andrei Emeltchenko wrote:
> >Hi Andrzej,
> >
> >On Thu, Nov 10, 2011 at 10:37:25AM +0100, Andrzej Kaczmarek wrote:
> >>sk_sndtimeo timeout value is specified in jiffes and should be
> >>converted to miliseconds when used as input to __set_chan_timer.
> >>
> >>Signed-off-by: Andrzej Kaczmarek<andrzej.kaczmarek@tieto.com>
> >>---
> >>  net/bluetooth/l2cap_core.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >>index f850684..3b0f807 100644
> >>--- a/net/bluetooth/l2cap_core.c
> >>+++ b/net/bluetooth/l2cap_core.c
> >>@@ -446,7 +446,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >>  		if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED&&
> >>  					conn->hcon->type == ACL_LINK) {
> >>  			__clear_chan_timer(chan);
> >>-			__set_chan_timer(chan, sk->sk_sndtimeo);
> >>+			__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
> >
> >Then __set_chan_timer do reverse conversion:
> >mod_timer(timer, jiffies + msecs_to_jiffies(timeout))
> >
> >Look ugly :-(
> 
> Well, I agree. But problem here is that __set_chan_timer has
> miliseconds as input and sk_sndtimeo has to be specified in jiffies
> thus ugly double conversion.

Maybe we can just use jiffies in  __set_chan_timer ?

Best regards 
Andrei Emeltchenko 

> 
> But what about adding __set_chan_timer_jiffies macro and use it here?
> 
> BR,
> Andrzej

Re: [PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Andrzej Kaczmarek]

Hi Andrei,

On 10.11.2011 11:35, Andrei Emeltchenko wrote:
> Hi Andrzej,
>
> On Thu, Nov 10, 2011 at 11:13:53AM +0100, Andrzej Kaczmarek wrote:
>> Hi Andrei,
>>
>> On 10.11.2011 10:57, Andrei Emeltchenko wrote:
>>> Hi Andrzej,
>>>
>>> On Thu, Nov 10, 2011 at 10:37:25AM +0100, Andrzej Kaczmarek wrote:
>>>> sk_sndtimeo timeout value is specified in jiffes and should be
>>>> converted to miliseconds when used as input to __set_chan_timer.
>>>>
>>>> Signed-off-by: Andrzej Kaczmarek<andrzej.kaczmarek@tieto.com>
>>>> ---
>>>>   net/bluetooth/l2cap_core.c |    8 ++++----
>>>>   1 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>>> index f850684..3b0f807 100644
>>>> --- a/net/bluetooth/l2cap_core.c
>>>> +++ b/net/bluetooth/l2cap_core.c
>>>> @@ -446,7 +446,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>>>>   		if (chan->chan_type == L2CAP_CHAN_CONN_ORIENTED&&
>>>>   					conn->hcon->type == ACL_LINK) {
>>>>   			__clear_chan_timer(chan);
>>>> -			__set_chan_timer(chan, sk->sk_sndtimeo);
>>>> +			__set_chan_timer(chan, jiffies_to_msecs(sk->sk_sndtimeo));
>>>
>>> Then __set_chan_timer do reverse conversion:
>>> mod_timer(timer, jiffies + msecs_to_jiffies(timeout))
>>>
>>> Look ugly :-(
>>
>> Well, I agree. But problem here is that __set_chan_timer has
>> miliseconds as input and sk_sndtimeo has to be specified in jiffies
>> thus ugly double conversion.
>
> Maybe we can just use jiffies in  __set_chan_timer ?

This will actually move conversion one step further to underlying 
l2cap_set_timer :-) And moreover we'll need to change other 
__set_chan_timer calls to use jiffies again and put msecs_to_jiffies 
"everywhere".

Of course we can make also l2cap_set_timer to accept jiffies and set all 
timers (except sk_sndtimeo) with msecs_to_jiffies, but I think this will 
be even more ugly :-(

BR,
Andrzej

Re: [PATCH 2/2] Bluetooth: Fix usage of sk_sndtimeo value

[Gustavo Padovan]

Hi Andrzej.

* Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com> [2011-11-10 10:37:25 +0100]:

> sk_sndtimeo timeout value is specified in jiffes and should be
> converted to miliseconds when used as input to __set_chan_timer.
> 
> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>
> ---
>  net/bluetooth/l2cap_core.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)

I think those one doesn't make sense anymore after the move to workqueues.

	Gustavo