pull-request: bluetooth-2.6 2010-09-27

pull-request: bluetooth-2.6 2010-09-27

[Gustavo F. Padovan]

Hi David,

These are the Bluetooth fixes for 2.6.36, you are used to see Marcel doing this
job, but he has been busy lately so I'm taking the job of the pull request this
time. Since we are a bit late on the pull request I'm skipping the
wireless-2.6 step, tell me if you (including John here) disagree.

In this patch set we have two fixes for regressions in L2CAP due to ERTM code
we added in L2CAP for 2.6.36, a bugfix in the L2CAP Streaming Mode that was
making the kernel crash. And a fix for a deadlock issue between the sk_sndbuf
and the backlog queue in ERTM. The rest are also needed bug fixes.

Please tell me any problem you have pulling this.

PS: Considering that these patches do not go immediately to net-next how do I
run the Bluetooth pull request to net-next? Currently my -next tree is rebased
on net-next but before the bluetooth-next patches I put the bluetooth patches
I have in my bluetooth-2.6 tree, i.e., when making the pull request I have to be
sure that the -next patches will be on top of the patches I'm submitting now
for 2.6.36. Any help on that is welcome. :)

Regards,


---

The following changes since commit b30a3f6257ed2105259b404d419b4964e363928c:

  Linux 2.6.36-rc5 (2010-09-20 16:56:53 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-2.6.git master

Andrei Emeltchenko (1):
      Bluetooth: fix MTU L2CAP configuration parameter

Gustavo F. Padovan (4):
      Bluetooth: Simplify L2CAP Streaming mode sending
      Bluetooth: Fix inconsistent lock state with RFCOMM
      Revert "Bluetooth: Don't accept ConfigReq if we aren't in the BT_CONFIG state"
      Bluetooth: Fix deadlock in the ERTM logic

Mat Martineau (1):
      Bluetooth: Only enable L2CAP FCS for ERTM or streaming

 include/net/bluetooth/bluetooth.h |   11 +++++++
 net/bluetooth/l2cap.c             |   57 +++++++++++++++---------------------
 net/bluetooth/rfcomm/sock.c       |    4 ++
 3 files changed, 39 insertions(+), 33 deletions(-)

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: pull-request: bluetooth-2.6 2010-09-27

[David Miller]

From: "Gustavo F. Padovan" <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
Date: Mon, 27 Sep 2010 23:30:35 -0300

> And a fix for a deadlock issue between the sk_sndbuf and the backlog
> queue in ERTM. The rest are also needed bug fixes.

This fix is still under discussion.

That change effects quite a few code paths.  And when I looked
at them, I was not at all convinced that dropping the socket
lock like that is safe.

Are you sure there are no pieces of socket or socket related state
that might change under us while we drop that lock, which would thus
make the operation suddenly invalid or cause a state corruption or
crash?

You really need to audit this.

Re: pull-request: bluetooth-2.6 2010-09-27

[Gustavo F. Padovan]

* David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> [2010-09-27 20:00:16 -0700]:

> From: "Gustavo F. Padovan" <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
> Date: Mon, 27 Sep 2010 23:30:35 -0300
> 
> > And a fix for a deadlock issue between the sk_sndbuf and the backlog
> > queue in ERTM. The rest are also needed bug fixes.
> 
> This fix is still under discussion.
> 
> That change effects quite a few code paths.  And when I looked
> at them, I was not at all convinced that dropping the socket
> lock like that is safe.
> 
> Are you sure there are no pieces of socket or socket related state
> that might change under us while we drop that lock, which would thus
> make the operation suddenly invalid or cause a state corruption or
> crash?

We can group all the code paths in only two different code paths. One
wirh SCO, L2CAP Basic Mode and L2CAP Streaming Mode once they are very
similar and other for ERTM, a more complicated protocol.
For the first group the only bottom half action we have are incoming data,
which doesn't affect the sk states, and disconnection request, that can
change the sk states. We guarantee that this won't affect by checking the
sk_err after get the lock again. Looking to the code again we might
also want to check the sk->sk_shutdown value like TCP does inside
sk_stream_wait_memory().

Actually sk_stream_wait_memory is another point why it's safe to release
the lock and block waiting for memory. We've been doing that safely in
protocols like TCP, SCTP and DCCP for a long time.

Back to patch, the other code path it affects is the ERTM one, besides
the incoming data we have other bottom halves actions, but in the end the
only action that can affect ERTM flow is closing the channeli, but we are
prepared for that by checking the sk->sk_err and sk->sk_shutdown when we
get the lock back.


---

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.

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 <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
Signed-off-by: Ulisses Furquim <ulisses-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
---
 include/net/bluetooth/bluetooth.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a902d..e8d64ba 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -161,12 +161,30 @@ static inline struct sk_buff *bt_skb_send_alloc(struct sock *sk, unsigned long l
 {
        struct sk_buff *skb;
 
+       release_sock(sk);
        if ((skb = sock_alloc_send_skb(sk, len + BT_SKB_RESERVE, nb, err))) {
                skb_reserve(skb, BT_SKB_RESERVE);
                bt_cb(skb)->incoming  = 0;
        }
+       lock_sock(sk);
+
+       if (!skb && *err)
+               return NULL;
+
+       *err = sock_error(sk);
+       if (*err)
+               goto out;
+
+       if (sk->sk_shutdown) {
+               *err = ECONNRESET;
+               goto out;
+       }
 
        return skb;
+
+out:
+       kfree_skb(skb);
+       return NULL;
 }
 
 int bt_err(__u16 code);
-- 
1.7.3


-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: pull-request: bluetooth-2.6 2010-09-27

[David Miller]

From: "Gustavo F. Padovan" <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
Date: Tue, 28 Sep 2010 19:49:41 -0300

> Actually sk_stream_wait_memory is another point why it's safe to release
> the lock and block waiting for memory. We've been doing that safely in
> protocols like TCP, SCTP and DCCP for a long time.

Do you notice what TCP does when sk_stream_wait_memory() returns?

It reloads all volatile state that might have changed in the socket
while the lock was dropped.

For example, TCP will reload the current MSS that can change
asynchronously while we don't have the socket lock.

Re: pull-request: bluetooth-2.6 2010-09-27

[Gustavo F. Padovan]

Hi Dave,

* David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> [2010-09-30 17:26:57 -0700]:

> From: "Gustavo F. Padovan" <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
> Date: Tue, 28 Sep 2010 19:49:41 -0300
> 
> > Actually sk_stream_wait_memory is another point why it's safe to release
> > the lock and block waiting for memory. We've been doing that safely in
> > protocols like TCP, SCTP and DCCP for a long time.
> 
> Do you notice what TCP does when sk_stream_wait_memory() returns?
> 
> It reloads all volatile state that might have changed in the socket
> while the lock was dropped.
> 
> For example, TCP will reload the current MSS that can change
> asynchronously while we don't have the socket lock.

I got your point. And what I tried to say in the last e-mail is that
ERTM doesn't have such volatile states that need to restore after get
the lock back. The others code path it affect are very simple and also
doesn't have such problem. So we are safe against asynchronous changes.
We obvious have volatiles states, but the code paths where
bt_skb_send_alloc() is used doesn't rely on that states. I'm seeing no
problem on release the lock, alloc memory, and lock it again.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: pull-request: bluetooth-2.6 2010-09-27

[Gustavo F. Padovan]

Hi Dave,

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-09-30 22:22:03 -0300]:

> Hi Dave,
> 
> * David Miller <davem@davemloft.net> [2010-09-30 17:26:57 -0700]:
> 
> > From: "Gustavo F. Padovan" <padovan@profusion.mobi>
> > Date: Tue, 28 Sep 2010 19:49:41 -0300
> > 
> > > Actually sk_stream_wait_memory is another point why it's safe to release
> > > the lock and block waiting for memory. We've been doing that safely in
> > > protocols like TCP, SCTP and DCCP for a long time.
> > 
> > Do you notice what TCP does when sk_stream_wait_memory() returns?
> > 
> > It reloads all volatile state that might have changed in the socket
> > while the lock was dropped.
> > 
> > For example, TCP will reload the current MSS that can change
> > asynchronously while we don't have the socket lock.
> 
> I got your point. And what I tried to say in the last e-mail is that
> ERTM doesn't have such volatile states that need to restore after get
> the lock back. The others code path it affect are very simple and also
> doesn't have such problem. So we are safe against asynchronous changes.
> We obvious have volatiles states, but the code paths where
> bt_skb_send_alloc() is used doesn't rely on that states. I'm seeing no
> problem on release the lock, alloc memory, and lock it again.

I did a proper audit of the code paths:

sco_send_frame(): after bt_skb_send_alloc() returns the function doesn't
touch any sk state anymore, it just picks the skb and sends it to the
HCI.

l2cap_create_connless_pdu(): after bt_skb_send_alloc() returns the
function only reads l2cap_pi(sk)->dcid and l2cap_pi(sk)->psm, those
value are static and don't change with the connection alive

l2cap_create_connless_pdu(): after bt_skb_send_alloc() returns the
function only reads l2cap_pi(sk)->dcid, that value is static and doesn't
change with the connection alive.

l2cap_create_iframe_pdu(): after bt_skb_send_alloc() returns, we only 
reads l2cap_pi(sk)->dcid and l2cap_pi(sk)->fcs which doesn change with 
the connection alive, l2cap_create_iframe_pdu() returns the skb to 
l2cap_sar_segment_sdu(), which changes only the TX_QUEUE(sk), but it 
only appends in the end while the rest of the L2CAP code reads from 
the begin. After that we have the process of sending of ERTM and 
Streaming Mode, this process is independent from the skb alloc process,
so no problem here.

Other point already discussed are the check after get the lock again, 
they guarantee that the socket is still connected, otherwise returns 
error.

An extra check will be added to setsockopt() just to make sure that
l2cap_pi(sk)->fcs won't change while the sk_state is BT_CONNECTED. That
was an issue even before this backlog queue issue.

Follow the output of git show for that change, if we agree on the change I
can append it to the bluetooth pull request.


Author: Gustavo F. Padovan <padovan@profusion.mobi>
Date:   Mon Oct 4 19:28:52 2010 -0300

    Bluetooth: Disallow to change L2CAP_OPTIONS values when connected
    
    L2CAP doesn't permit change like MTU, FCS, TxWindow values while the
    connection is alive, we can only set that before the
    connection/configuration process. That can lead to bugs in the L2CAP
    operation.
    
    Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 44a8fb0..0b54b7d 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1950,6 +1950,11 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 
        switch (optname) {
        case L2CAP_OPTIONS:
+               if (sk->sk_state == BT_CONNECTED) {
+                       err = -EINVAL;
+                       break;
+               }
+
                opts.imtu     = l2cap_pi(sk)->imtu;
                opts.omtu     = l2cap_pi(sk)->omtu;
                opts.flush_to = l2cap_pi(sk)->flush_to;


-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: pull-request: bluetooth-2.6 2010-09-27

[David Miller]

From: "Gustavo F. Padovan" <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
Date: Mon, 4 Oct 2010 19:35:13 -0300

> Follow the output of git show for that change, if we agree on the change I
> can append it to the bluetooth pull request.

That makes sense to me, thanks for doing this audit.

Append that commit and send a new pull request.

Thanks!