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
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<[email protected]>
>>>> ---
>>>> 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
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<[email protected]>
> >>---
> >> 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
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<[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Andrzej.
* Andrzej Kaczmarek <[email protected]> [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 <[email protected]>
> ---
> 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