All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Bluetooth: Various L2CAP fixes
@ 2013-09-13  8:43 johan.hedberg
  2013-09-13  8:43 ` [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID johan.hedberg
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: johan.hedberg @ 2013-09-13  8:43 UTC (permalink / raw)
  To: linux-bluetooth

Hi,

This patch set includes various fixes to L2CAP signaling and socket
handling.

Johan

----------------------------------------------------------------
Johan Hedberg (3):
      Bluetooth: Fix L2CAP Disconnect response for unknown CID
      Bluetooth: Fix responding to invalid L2CAP signaling commands
      Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag

 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/af_bluetooth.c      | 38 +++++++++++++++++++++++++++++++++++++
 net/bluetooth/l2cap_core.c        | 10 +++++++++-
 net/bluetooth/l2cap_sock.c        |  6 ++++++
 net/bluetooth/rfcomm/sock.c       |  7 +++++--
 5 files changed, 59 insertions(+), 3 deletions(-)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID
  2013-09-13  8:43 [PATCH 0/3] Bluetooth: Various L2CAP fixes johan.hedberg
@ 2013-09-13  8:43 ` johan.hedberg
  2013-09-14  1:40   ` Marcel Holtmann
  2013-09-13  8:43 ` [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands johan.hedberg
  2013-09-13  8:43 ` [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag johan.hedberg
  2 siblings, 1 reply; 12+ messages in thread
From: johan.hedberg @ 2013-09-13  8:43 UTC (permalink / raw)
  To: linux-bluetooth

From: Johan Hedberg <johan.hedberg@intel.com>

If we receive an L2CAP Disconnect Request for an unknown CID we should
not just silently drop it but reply with a proper Command Reject
response. This patch fixes this by ensuring that the disconnect handler
returns a proper error instead of 0 and will cause the function caller
to send the right response.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b3bb7bc..ea3792f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
 	chan = __l2cap_get_chan_by_scid(conn, dcid);
 	if (!chan) {
 		mutex_unlock(&conn->chan_lock);
-		return 0;
+		return -EINVAL;
 	}
 
 	l2cap_chan_lock(chan);
-- 
1.8.4.rc3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands
  2013-09-13  8:43 [PATCH 0/3] Bluetooth: Various L2CAP fixes johan.hedberg
  2013-09-13  8:43 ` [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID johan.hedberg
@ 2013-09-13  8:43 ` johan.hedberg
  2013-09-14  1:42   ` Marcel Holtmann
  2013-09-13  8:43 ` [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag johan.hedberg
  2 siblings, 1 reply; 12+ messages in thread
From: johan.hedberg @ 2013-09-13  8:43 UTC (permalink / raw)
  To: linux-bluetooth

From: Johan Hedberg <johan.hedberg@intel.com>

When we have an LE link we should not respond to any data on the BR/EDR
L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
This patch fixes this issue by checking for a valid link type and
ignores data if it is wrong.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ea3792f..1d03644 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5297,6 +5297,7 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
 static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 					struct sk_buff *skb)
 {
+	struct hci_conn *hcon = conn->hcon;
 	u8 *data = skb->data;
 	int len = skb->len;
 	struct l2cap_cmd_hdr cmd;
@@ -5304,6 +5305,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 
 	l2cap_raw_recv(conn, skb);
 
+	if (hcon->type != LE_LINK)
+		return;
+
 	while (len >= L2CAP_CMD_HDR_SIZE) {
 		u16 cmd_len;
 		memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
@@ -5342,6 +5346,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
 static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 				     struct sk_buff *skb)
 {
+	struct hci_conn *hcon = conn->hcon;
 	u8 *data = skb->data;
 	int len = skb->len;
 	struct l2cap_cmd_hdr cmd;
@@ -5349,6 +5354,9 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
 
 	l2cap_raw_recv(conn, skb);
 
+	if (hcon->type != ACL_LINK)
+		return;
+
 	while (len >= L2CAP_CMD_HDR_SIZE) {
 		u16 cmd_len;
 		memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
-- 
1.8.4.rc3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
  2013-09-13  8:43 [PATCH 0/3] Bluetooth: Various L2CAP fixes johan.hedberg
  2013-09-13  8:43 ` [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID johan.hedberg
  2013-09-13  8:43 ` [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands johan.hedberg
@ 2013-09-13  8:43 ` johan.hedberg
  2013-09-13 10:41   ` Anderson Lizardo
  2013-09-14  1:43   ` Marcel Holtmann
  2 siblings, 2 replies; 12+ messages in thread
From: johan.hedberg @ 2013-09-13  8:43 UTC (permalink / raw)
  To: linux-bluetooth

From: Johan Hedberg <johan.hedberg@intel.com>

In the case of blocking sockets we should not proceed with sendmsg() if
the socket has the BT_SK_SUSPEND flag set. So far the code was only
ensuring that POLLOUT doesn't get set for non-blocking sockets using
poll() but there was no code in place to ensure that blocking sockets do
the right thing when writing to them.

This patch adds a new bt_sock_wait_unsuspend helper function to sleep in
the sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon
as it is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks
to take advantage of this new helper function.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/af_bluetooth.c      | 38 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/l2cap_sock.c        |  6 ++++++
 net/bluetooth/rfcomm/sock.c       |  7 +++++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10d43d8..3299d42 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -249,6 +249,7 @@ int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
 int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
 int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
+int  bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags);
 
 void bt_accept_enqueue(struct sock *parent, struct sock *sk);
 void bt_accept_unlink(struct sock *sk);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9096137..624f866 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -525,6 +525,44 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
 }
 EXPORT_SYMBOL(bt_sock_wait_state);
 
+int bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	unsigned long timeo;
+	int err = 0;
+
+	BT_DBG("sk %p", sk);
+
+	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
+		if (!timeo) {
+			err = -EAGAIN;
+			break;
+		}
+
+		if (signal_pending(current)) {
+			err = sock_intr_errno(timeo);
+			break;
+		}
+
+		release_sock(sk);
+		timeo = schedule_timeout(timeo);
+		lock_sock(sk);
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		err = sock_error(sk);
+		if (err)
+			break;
+	}
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(sk_sleep(sk), &wait);
+	return err;
+}
+EXPORT_SYMBOL(bt_sock_wait_unsuspend);
+
 #ifdef CONFIG_PROC_FS
 struct bt_seq_state {
 	struct bt_sock_list *l;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0098af8..1ddbc1a 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	if (sk->sk_state != BT_CONNECTED)
 		return -ENOTCONN;
 
+	lock_sock(sk);
+	err = bt_sock_wait_unsuspend(sk, msg->msg_flags);
+	release_sock(sk);
+	if (err)
+		return err;
+
 	l2cap_chan_lock(chan);
 	err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
 	l2cap_chan_unlock(chan);
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 30b3721..4203ec4 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	struct sock *sk = sock->sk;
 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
 	struct sk_buff *skb;
-	int sent = 0;
+	int err, sent = 0;
 
 	if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
 		return -ENOTCONN;
@@ -559,9 +559,12 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	err = bt_sock_wait_unsuspend(sk, msg->msg_flags);
+	if (err)
+		return err;
+
 	while (len) {
 		size_t size = min_t(size_t, len, d->mtu);
-		int err;
 
 		skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
 				msg->msg_flags & MSG_DONTWAIT, &err);
-- 
1.8.4.rc3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
  2013-09-13  8:43 ` [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag johan.hedberg
@ 2013-09-13 10:41   ` Anderson Lizardo
  2013-09-13 11:01     ` Johan Hedberg
  2013-09-14  1:43   ` Marcel Holtmann
  1 sibling, 1 reply; 12+ messages in thread
From: Anderson Lizardo @ 2013-09-13 10:41 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: BlueZ development

Hi Johan,

On Fri, Sep 13, 2013 at 4:43 AM,  <johan.hedberg@gmail.com> wrote:
> +int bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags)
> +{
> +       DECLARE_WAITQUEUE(wait, current);
> +       unsigned long timeo;
> +       int err = 0;
> +
> +       BT_DBG("sk %p", sk);
> +
> +       timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> +
> +       add_wait_queue(sk_sleep(sk), &wait);
> +       set_current_state(TASK_INTERRUPTIBLE);
> +       while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
> +               if (!timeo) {
> +                       err = -EAGAIN;
> +                       break;
> +               }
> +
> +               if (signal_pending(current)) {
> +                       err = sock_intr_errno(timeo);
> +                       break;
> +               }
> +
> +               release_sock(sk);
> +               timeo = schedule_timeout(timeo);
> +               lock_sock(sk);
> +               set_current_state(TASK_INTERRUPTIBLE);
> +
> +               err = sock_error(sk);
> +               if (err)
> +                       break;
> +       }
> +       __set_current_state(TASK_RUNNING);
> +       remove_wait_queue(sk_sleep(sk), &wait);
> +       return err;
> +}

You can add a blank line before "return err" for slightly better legibility.

> @@ -559,9 +559,12 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
>
>         lock_sock(sk);
>
> +       err = bt_sock_wait_unsuspend(sk, msg->msg_flags);
> +       if (err)
> +               return err;
> +

Is it okay to return without releasing the lock?

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
  2013-09-13 10:41   ` Anderson Lizardo
@ 2013-09-13 11:01     ` Johan Hedberg
  0 siblings, 0 replies; 12+ messages in thread
From: Johan Hedberg @ 2013-09-13 11:01 UTC (permalink / raw)
  To: Anderson Lizardo; +Cc: BlueZ development

Hi Lizardo,

On Fri, Sep 13, 2013, Anderson Lizardo wrote:
> On Fri, Sep 13, 2013 at 4:43 AM,  <johan.hedberg@gmail.com> wrote:
> > +int bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags)
> > +{
> > +       DECLARE_WAITQUEUE(wait, current);
> > +       unsigned long timeo;
> > +       int err = 0;
> > +
> > +       BT_DBG("sk %p", sk);
> > +
> > +       timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> > +
> > +       add_wait_queue(sk_sleep(sk), &wait);
> > +       set_current_state(TASK_INTERRUPTIBLE);
> > +       while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
> > +               if (!timeo) {
> > +                       err = -EAGAIN;
> > +                       break;
> > +               }
> > +
> > +               if (signal_pending(current)) {
> > +                       err = sock_intr_errno(timeo);
> > +                       break;
> > +               }
> > +
> > +               release_sock(sk);
> > +               timeo = schedule_timeout(timeo);
> > +               lock_sock(sk);
> > +               set_current_state(TASK_INTERRUPTIBLE);
> > +
> > +               err = sock_error(sk);
> > +               if (err)
> > +                       break;
> > +       }
> > +       __set_current_state(TASK_RUNNING);
> > +       remove_wait_queue(sk_sleep(sk), &wait);
> > +       return err;
> > +}
> 
> You can add a blank line before "return err" for slightly better legibility.

Sure. The reason it's not there is that I followed the existing similar
bt_sock_wait_state function implementation.

> > @@ -559,9 +559,12 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> >
> >         lock_sock(sk);
> >
> > +       err = bt_sock_wait_unsuspend(sk, msg->msg_flags);
> > +       if (err)
> > +               return err;
> > +
> 
> Is it okay to return without releasing the lock?

Nope, I'm pretty sure that's a bug. Good catch, I'll fix in a v2.

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID
  2013-09-13  8:43 ` [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID johan.hedberg
@ 2013-09-14  1:40   ` Marcel Holtmann
  2013-09-14 16:11     ` Johan Hedberg
  0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2013-09-14  1:40 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth

Hi Johan,

> If we receive an L2CAP Disconnect Request for an unknown CID we should
> not just silently drop it but reply with a proper Command Reject
> response. This patch fixes this by ensuring that the disconnect handler
> returns a proper error instead of 0 and will cause the function caller
> to send the right response.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index b3bb7bc..ea3792f 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
> 	chan = __l2cap_get_chan_by_scid(conn, dcid);
> 	if (!chan) {
> 		mutex_unlock(&conn->chan_lock);
> -		return 0;
> +		return -EINVAL;
> 	}

the only problem that I have here is that we end up with the wrong error code. The L2CAP core still contains this comment:

/* FIXME: Map err to a valid reason */

And now we default to L2CAP_REJ_NOT_UNDERSTOOD, but in this case it should be L2CAP_REJ_INVALID_CID.

Regards

Marcel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands
  2013-09-13  8:43 ` [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands johan.hedberg
@ 2013-09-14  1:42   ` Marcel Holtmann
  2013-09-14  5:48     ` Johan Hedberg
  0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2013-09-14  1:42 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth

Hi Johan,

> When we have an LE link we should not respond to any data on the BR/EDR
> L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
> link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
> This patch fixes this issue by checking for a valid link type and
> ignores data if it is wrong.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ea3792f..1d03644 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -5297,6 +5297,7 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
> static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> 					struct sk_buff *skb)
> {
> +	struct hci_conn *hcon = conn->hcon;
> 	u8 *data = skb->data;
> 	int len = skb->len;
> 	struct l2cap_cmd_hdr cmd;
> @@ -5304,6 +5305,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> 
> 	l2cap_raw_recv(conn, skb);
> 
> +	if (hcon->type != LE_LINK)
> +		return;
> +

However I do have a question here. Can we just drop the packet or should we even in this case return invalid CID. What does the core spec. recommend in this case?

Regards

Marcel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
  2013-09-13  8:43 ` [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag johan.hedberg
  2013-09-13 10:41   ` Anderson Lizardo
@ 2013-09-14  1:43   ` Marcel Holtmann
  2013-09-14  5:50     ` Johan Hedberg
  1 sibling, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2013-09-14  1:43 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth

Hi Johan,

> In the case of blocking sockets we should not proceed with sendmsg() if
> the socket has the BT_SK_SUSPEND flag set. So far the code was only
> ensuring that POLLOUT doesn't get set for non-blocking sockets using
> poll() but there was no code in place to ensure that blocking sockets do
> the right thing when writing to them.
> 
> This patch adds a new bt_sock_wait_unsuspend helper function to sleep in
> the sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon
> as it is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks
> to take advantage of this new helper function.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/bluetooth.h |  1 +
> net/bluetooth/af_bluetooth.c      | 38 ++++++++++++++++++++++++++++++++++++++
> net/bluetooth/l2cap_sock.c        |  6 ++++++
> net/bluetooth/rfcomm/sock.c       |  7 +++++--
> 4 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 10d43d8..3299d42 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -249,6 +249,7 @@ int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> +int  bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags);

I am not a huge fan of the term unsuspend. Maybe wait_ready is better here.

Regards

Marcel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands
  2013-09-14  1:42   ` Marcel Holtmann
@ 2013-09-14  5:48     ` Johan Hedberg
  0 siblings, 0 replies; 12+ messages in thread
From: Johan Hedberg @ 2013-09-14  5:48 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > @@ -5304,6 +5305,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> > 
> > 	l2cap_raw_recv(conn, skb);
> > 
> > +	if (hcon->type != LE_LINK)
> > +		return;
> > +
> 
> However I do have a question here. Can we just drop the packet or
> should we even in this case return invalid CID. What does the core
> spec. recommend in this case?

I faced this issue at the last UPF and had to do some thinking about it
as well as discuss it with some other stack developers. The conclusion
was that dropping is the right thing since on non-LE links this is in
practice an unknown CID and unknown data.

The core spec doesn't explicitly say that data should be dropped in this
case (it doesn't say anything about responding either). Instead, the LE
signaling channel is simply not defined for non-LE links (the exact
words in the spec are "not available" and "not supported"). It's like
you'd receive data on any unknown CID. Responding something on it would
be just as bad behavior as the peer sending the initial command.

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
  2013-09-14  1:43   ` Marcel Holtmann
@ 2013-09-14  5:50     ` Johan Hedberg
  0 siblings, 0 replies; 12+ messages in thread
From: Johan Hedberg @ 2013-09-14  5:50 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > In the case of blocking sockets we should not proceed with sendmsg() if
> > the socket has the BT_SK_SUSPEND flag set. So far the code was only
> > ensuring that POLLOUT doesn't get set for non-blocking sockets using
> > poll() but there was no code in place to ensure that blocking sockets do
> > the right thing when writing to them.
> > 
> > This patch adds a new bt_sock_wait_unsuspend helper function to sleep in
> > the sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon
> > as it is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks
> > to take advantage of this new helper function.
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > include/net/bluetooth/bluetooth.h |  1 +
> > net/bluetooth/af_bluetooth.c      | 38 ++++++++++++++++++++++++++++++++++++++
> > net/bluetooth/l2cap_sock.c        |  6 ++++++
> > net/bluetooth/rfcomm/sock.c       |  7 +++++--
> > 4 files changed, 50 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 10d43d8..3299d42 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -249,6 +249,7 @@ int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> > uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> > int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> > int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> > +int  bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags);
> 
> I am not a huge fan of the term unsuspend. Maybe wait_ready is better here.

I'll change it to wait_ready in the next version. I used unsuspend
simply because it waits for the clearing of a flag called SUSPEND.

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID
  2013-09-14  1:40   ` Marcel Holtmann
@ 2013-09-14 16:11     ` Johan Hedberg
  0 siblings, 0 replies; 12+ messages in thread
From: Johan Hedberg @ 2013-09-14 16:11 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > If we receive an L2CAP Disconnect Request for an unknown CID we should
> > not just silently drop it but reply with a proper Command Reject
> > response. This patch fixes this by ensuring that the disconnect handler
> > returns a proper error instead of 0 and will cause the function caller
> > to send the right response.
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > net/bluetooth/l2cap_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index b3bb7bc..ea3792f 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
> > 	chan = __l2cap_get_chan_by_scid(conn, dcid);
> > 	if (!chan) {
> > 		mutex_unlock(&conn->chan_lock);
> > -		return 0;
> > +		return -EINVAL;
> > 	}
> 
> the only problem that I have here is that we end up with the wrong
> error code. The L2CAP core still contains this comment:
> 
> /* FIXME: Map err to a valid reason */
> 
> And now we default to L2CAP_REJ_NOT_UNDERSTOOD, but in this case it
> should be L2CAP_REJ_INVALID_CID.

Well, starting to look at this opened quite a can of worms. There are
several existing inconsistencies in what error codes request handlers
return in various situations. Some are even broken in that they send
their own response but still return non-zero (causing a second response
to be sent).

The current code will also sometimes send a command reject as a
consequence of incorrectly formatted response packets, which is also
wrong according to the core spec (p.1416): "Command Reject packets
should not be sent in response to an identified Response packet.".

I'll fix all of the above issues before introducing a helper function to
map the handler return value to an L2CAP error code.

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-09-14 16:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-13  8:43 [PATCH 0/3] Bluetooth: Various L2CAP fixes johan.hedberg
2013-09-13  8:43 ` [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID johan.hedberg
2013-09-14  1:40   ` Marcel Holtmann
2013-09-14 16:11     ` Johan Hedberg
2013-09-13  8:43 ` [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands johan.hedberg
2013-09-14  1:42   ` Marcel Holtmann
2013-09-14  5:48     ` Johan Hedberg
2013-09-13  8:43 ` [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag johan.hedberg
2013-09-13 10:41   ` Anderson Lizardo
2013-09-13 11:01     ` Johan Hedberg
2013-09-14  1:43   ` Marcel Holtmann
2013-09-14  5:50     ` Johan Hedberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.