This patch lets L2CAP process received S-frames even when socket is
continuously being locked by user process.
This issue was seen when testing with l2test without using "-D" option.
Since the user process does not expect any Rx packets,
it hogs the socket with continuous call to "send()".
When the TxWindow is full Tx stops untill the I-frames are acked by the receiver.
But the Rx S-Frame acknowleding the Tx frames will stay in the backlog queue
because the "sock_owned_by_user()" call in l2cap_data_channel()
will always return true.
The user process does not have an idea about this
mechanism and keep pumping data and locking the socket and cause a deadlock.
Signed-off-by: Suraj Sumangala <[email protected]>
---
net/bluetooth/l2cap.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 1761558..dc22291 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -4336,6 +4336,15 @@ static int l2cap_ertm_data_rcv(struct sock *sk, struct sk_buff *skb)
int len, next_tx_seq_offset, req_seq_offset;
control = get_unaligned_le16(skb->data);
+
+ if (sock_owned_by_user(sk)) {
+ if (__is_iframe(control)) {
+ if (sk_add_backlog(sk, skb))
+ goto drop;
+
+ return 0;
+ }
+ }
skb_pull(skb, 2);
len = skb->len;
@@ -4434,12 +4443,7 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
break;
case L2CAP_MODE_ERTM:
- if (!sock_owned_by_user(sk)) {
- l2cap_ertm_data_rcv(sk, skb);
- } else {
- if (sk_add_backlog(sk, skb))
- goto drop;
- }
+ l2cap_ertm_data_rcv(sk, skb);
goto done;
--
1.7.0.4
Hi Gustavo,
On 2/2/2011 11:11 PM, Gustavo F. Padovan wrote:
> Hi Suraj,
>
> * Suraj Sumangala<[email protected]> [2011-02-02 23:05:19 +0530]:
>
>> Hi Gustavo,
>>
>> On 2/2/2011 10:21 PM, Gustavo F. Padovan wrote:
>>> This one: e454c844644683571617896ab2a4ce0109c1943e
>>>
>>> The issue fixed by this patch is very similar to what you reported
>>
>> Is this commit available in
>> "git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-next-2.6.git"
>> tree?
>
> Yes, it is also available in Linus' tree.
>
> commit e454c844644683571617896ab2a4ce0109c1943e
> Author: Gustavo F. Padovan<[email protected]>
> Date: Tue Sep 21 16:31:11 2010 -0300
>
> Bluetooth: Fix deadlock in the ERTM logic
>
> The Enhanced Retransmission Mode(ERTM) is a realiable mode of operation
> of the Bluetooth L2CAP layer. Think on it like a simplified version of
> TCP.
> The problem we were facing here was a deadlock. ERTM uses a backlog
> queue to queue incomimg packets while the user is helding the lock. At
> some moment the sk_sndbuf can be exceeded and we can't alloc new skbs
> then the code sleep with the lock to wait for memory, that stalls the
> ERTM connection once we can't read the acknowledgements packets in the
> backlog queue to free memory and make the allocation of outcoming skb
> successful.
> successful.
>
> This patch actually affect all users of bt_skb_send_alloc(), i.e., all
> L2CAP modes and SCO.
>
> We are safe against socket states changes or channels deletion while the
> we are sleeping wait memory. Checking for the sk->sk_err and
> sk->sk_shutdown make the code safe, since any action that can leave the
> socket or the channel in a not usable state set one of the struct
> members at least. Then we can check both of them when getting the lock
> again and return with the proper error if something unexpected happens.
>
> Signed-off-by: Gustavo F. Padovan<[email protected]>
> Signed-off-by: Ulisses Furquim<[email protected]>
>
>
>
Thanks,this patch solved my issue.
Regards
Suraj
Hi Suraj,
* Suraj Sumangala <[email protected]> [2011-02-02 23:05:19 +0530]:
> Hi Gustavo,
>
> On 2/2/2011 10:21 PM, Gustavo F. Padovan wrote:
> > This one: e454c844644683571617896ab2a4ce0109c1943e
> >
> > The issue fixed by this patch is very similar to what you reported
>
> Is this commit available in
> "git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-next-2.6.git"
> tree?
Yes, it is also available in Linus' tree.
commit e454c844644683571617896ab2a4ce0109c1943e
Author: Gustavo F. Padovan <[email protected]>
Date: Tue Sep 21 16:31:11 2010 -0300
Bluetooth: Fix deadlock in the ERTM logic
The Enhanced Retransmission Mode(ERTM) is a realiable mode of operation
of the Bluetooth L2CAP layer. Think on it like a simplified version of
TCP.
The problem we were facing here was a deadlock. ERTM uses a backlog
queue to queue incomimg packets while the user is helding the lock. At
some moment the sk_sndbuf can be exceeded and we can't alloc new skbs
then the code sleep with the lock to wait for memory, that stalls the
ERTM connection once we can't read the acknowledgements packets in the
backlog queue to free memory and make the allocation of outcoming skb
successful.
successful.
This patch actually affect all users of bt_skb_send_alloc(), i.e., all
L2CAP modes and SCO.
We are safe against socket states changes or channels deletion while the
we are sleeping wait memory. Checking for the sk->sk_err and
sk->sk_shutdown make the code safe, since any action that can leave the
socket or the channel in a not usable state set one of the struct
members at least. Then we can check both of them when getting the lock
again and return with the proper error if something unexpected happens.
Signed-off-by: Gustavo F. Padovan <[email protected]>
Signed-off-by: Ulisses Furquim <[email protected]>
--
Gustavo F. Padovan
http://profusion.mobi
Hi Gustavo,
On 2/2/2011 10:21 PM, Gustavo F. Padovan wrote:
> This one: e454c844644683571617896ab2a4ce0109c1943e
>
> The issue fixed by this patch is very similar to what you reported
Is this commit available in
"git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-next-2.6.git"
tree?
Sorry, could not find it there.
Regards
Suraj
Hi Suraj,
* Suraj Sumangala <[email protected]> [2011-02-02 22:04:39 +0530]:
> Hi Gustavo,
>
> On 2/2/2011 9:58 PM, Gustavo F. Padovan wrote:
> > Hi Suraj,
> >
> > * Suraj Sumangala<[email protected]> [2011-01-31 18:42:51 +0530]:
> >
> >> This patch lets L2CAP process received S-frames even when socket is
> >> continuously being locked by user process.
> >>
> >> This issue was seen when testing with l2test without using "-D" option.
> >>
> >> Since the user process does not expect any Rx packets,
> >> it hogs the socket with continuous call to "send()".
> >>
> >> When the TxWindow is full Tx stops untill the I-frames are acked by the receiver.
> >>
> >> But the Rx S-Frame acknowleding the Tx frames will stay in the backlog queue
> >> because the "sock_owned_by_user()" call in l2cap_data_channel()
> >> will always return true.
> >>
> >> The user process does not have an idea about this
> >> mechanism and keep pumping data and locking the socket and cause a deadlock.
> >
> > In which kernel are you seeing this error? I think it is already fixed.
> >
> > Regards,
> >
>
> Can you direct me to the patch which fixed it?
This one: e454c844644683571617896ab2a4ce0109c1943e
The issue fixed by this patch is very similar to what you reported.
--
Gustavo F. Padovan
http://profusion.mobi
Hi Gustavo,
On 2/2/2011 9:58 PM, Gustavo F. Padovan wrote:
> Hi Suraj,
>
> * Suraj Sumangala<[email protected]> [2011-01-31 18:42:51 +0530]:
>
>> This patch lets L2CAP process received S-frames even when socket is
>> continuously being locked by user process.
>>
>> This issue was seen when testing with l2test without using "-D" option.
>>
>> Since the user process does not expect any Rx packets,
>> it hogs the socket with continuous call to "send()".
>>
>> When the TxWindow is full Tx stops untill the I-frames are acked by the receiver.
>>
>> But the Rx S-Frame acknowleding the Tx frames will stay in the backlog queue
>> because the "sock_owned_by_user()" call in l2cap_data_channel()
>> will always return true.
>>
>> The user process does not have an idea about this
>> mechanism and keep pumping data and locking the socket and cause a deadlock.
>
> In which kernel are you seeing this error? I think it is already fixed.
>
> Regards,
>
Can you direct me to the patch which fixed it?
I had see this problem when verifying Bluetooth 3.0 in kernel version
2.6.35 and see similar code in the kernel-next tree. That is the reason
why I sent an RFC.
Regards
Suraj
Hi Suraj,
* Suraj Sumangala <[email protected]> [2011-01-31 18:42:51 +0530]:
> This patch lets L2CAP process received S-frames even when socket is
> continuously being locked by user process.
>
> This issue was seen when testing with l2test without using "-D" option.
>
> Since the user process does not expect any Rx packets,
> it hogs the socket with continuous call to "send()".
>
> When the TxWindow is full Tx stops untill the I-frames are acked by the receiver.
>
> But the Rx S-Frame acknowleding the Tx frames will stay in the backlog queue
> because the "sock_owned_by_user()" call in l2cap_data_channel()
> will always return true.
>
> The user process does not have an idea about this
> mechanism and keep pumping data and locking the socket and cause a deadlock.
In which kernel are you seeing this error? I think it is already fixed.
Regards,
--
Gustavo F. Padovan
http://profusion.mobi