* [PATCH v5 7/8] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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 b0947ae..636a3b4 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static __le16 l2cap_err_to_reason(int err)
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;
@@ -5318,6 +5319,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);
@@ -5355,6 +5359,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;
@@ -5362,6 +5367,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
* [PATCH v5 6/8] Bluetooth: Fix sending responses to identified L2CAP response packets
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When L2CAP packets return a non-zero error and the value is passed
onwards by l2cap_bredr_sig_cmd this will trigger a command reject packet
to be sent. However, the core specification (page 1416 in core 4.0) says
the following: "Command Reject packets should not be sent in response to
an identified Response packet.".
This patch ensures that a command reject packet is not sent for any
identified response packet by ignoring the error return value from the
response handler functions.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2ff35a2..b0947ae 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5212,7 +5212,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
case L2CAP_CONN_RSP:
case L2CAP_CREATE_CHAN_RSP:
- err = l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
+ l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CONF_REQ:
@@ -5220,7 +5220,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_CONF_RSP:
- err = l2cap_config_rsp(conn, cmd, cmd_len, data);
+ l2cap_config_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_DISCONN_REQ:
@@ -5228,7 +5228,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_DISCONN_RSP:
- err = l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
+ l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_ECHO_REQ:
@@ -5243,7 +5243,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_INFO_RSP:
- err = l2cap_information_rsp(conn, cmd, cmd_len, data);
+ l2cap_information_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CREATE_CHAN_REQ:
@@ -5255,7 +5255,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_RSP:
- err = l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_MOVE_CHAN_CFM:
@@ -5263,7 +5263,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_CFM_RSP:
- err = l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
break;
default:
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 5/8] Bluetooth: Fix L2CAP command reject reason
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
There are several possible reason codes that can be sent in the command
reject L2CAP packet. Before this patch the code has used a hard-coded
single response code ("command not understood"). This patch adds a
helper function to map the return value of an L2CAP handler function to
the correct command reject reason.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 31bf812..2ff35a2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5294,6 +5294,20 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
}
}
+static __le16 l2cap_err_to_reason(int err)
+{
+ switch (err) {
+ case -EBADSLT:
+ return __constant_cpu_to_le16(L2CAP_REJ_INVALID_CID);
+ case -EMSGSIZE:
+ return __constant_cpu_to_le16(L2CAP_REJ_MTU_EXCEEDED);
+ case -EINVAL:
+ case -EPROTO:
+ default:
+ return __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ }
+}
+
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
@@ -5326,8 +5340,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
@@ -5371,8 +5384,7 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
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 eb60cf7..31bf812 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 -EBADSLT;
}
l2cap_chan_lock(chan);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 3/8] Bluetooth: Fix L2CAP error return used for failed channel lookups
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The EFAULT error should only be used for memory address related errors
and ENOENT might be needed for other purposes than invalid CID errors.
This patch fixes the l2cap_config_req, l2cap_connect_create_rsp and
l2cap_create_channel_req handlers to use the unique EBADSLT error to
indicate failed lookups on a given CID.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b8a270..eb60cf7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3884,13 +3884,13 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
if (scid) {
chan = __l2cap_get_chan_by_scid(conn, scid);
if (!chan) {
- err = -EFAULT;
+ err = -EBADSLT;
goto unlock;
}
} else {
chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
if (!chan) {
- err = -EFAULT;
+ err = -EBADSLT;
goto unlock;
}
}
@@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
chan = l2cap_get_chan_by_scid(conn, dcid);
if (!chan)
- return -ENOENT;
+ return -EBADSLT;
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
@@ -4438,7 +4438,7 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
hs_hcon = hci_conn_hash_lookup_ba(hdev, AMP_LINK, conn->dst);
if (!hs_hcon) {
hci_dev_put(hdev);
- return -EFAULT;
+ return -EBADSLT;
}
BT_DBG("mgr %p bredr_chan %p hs_hcon %p", mgr, chan, hs_hcon);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When an L2CAP request handler returns non-zero the calling code will
send a command reject response. The l2cap_create_chan_req function will
in some cases send its own response but then still return a -EFAULT
error which would cause two responses to be sent. This patch fixes this
by making the function return 0 after sending its own response.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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..0b8a270 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4462,7 +4462,7 @@ error:
l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
sizeof(rsp), &rsp);
- return -EFAULT;
+ return 0;
}
static void l2cap_send_move_chan_req(struct l2cap_chan *chan, u8 dest_amp_id)
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 1/8] Bluetooth: Remove unused event mask struct
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379325919-19003-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The struct for HCI_Set_Event_Mask is never used. Instead a local 8-byte
array is used for sending this command. Therefore, remove the
unnecessary struct definition.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 57aa408..7ede266 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -695,9 +695,6 @@ struct hci_cp_sniff_subrate {
} __packed;
#define HCI_OP_SET_EVENT_MASK 0x0c01
-struct hci_cp_set_event_mask {
- __u8 mask[8];
-} __packed;
#define HCI_OP_RESET 0x0c03
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v5 0/8] Bluetooth: Various L2CAP fixes
From: johan.hedberg @ 2013-09-16 10:05 UTC (permalink / raw)
To: linux-bluetooth
Hi,
Here's a fifth revision with all feedback taken into account (as far as
these 8 patches are concerned). There's still the issue of verifying
sk locking in l2cap_sock_sendmsg as well as a potentially missing
comment for the wait_state function, but even if those fixes are needed
they'll end up being in separate patches. So, I'm hoping this patch set
could start reaching its conclusion.
Johan
----------------------------------------------------------------
Johan Hedberg (8):
Bluetooth: Remove unused event mask struct
Bluetooth: Fix double error response for l2cap_create_chan_req
Bluetooth: Fix L2CAP error return used for failed channel lookups
Bluetooth: Fix L2CAP Disconnect response for unknown CID
Bluetooth: Fix L2CAP command reject reason
Bluetooth: Fix sending responses to identified L2CAP response packets
Bluetooth: Fix responding to invalid L2CAP signaling commands
Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/hci.h | 3 ---
net/bluetooth/af_bluetooth.c | 40 ++++++++++++++++++++++++++++
net/bluetooth/l2cap_core.c | 52 +++++++++++++++++++++++++------------
net/bluetooth/l2cap_sock.c | 6 +++++
net/bluetooth/rfcomm/sock.c | 7 ++++-
6 files changed, 89 insertions(+), 20 deletions(-)
^ permalink raw reply
* Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: Johan Hedberg @ 2013-09-16 9:54 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <B44973E0-9AF5-4AE2-8C81-E49EE46C6E31@holtmann.org>
Hi Marcel,
On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> I have nothing against ENOENT. I however am a bit concerned about that
> we accidentally use ENOENT in some other case and end up overloading
> its meaning.
>
> So just for the sake of making this unique, I would consider going
> with EBADSLT. I am not married to this at all. However I wanted to
> have this talked through so that we understand the whole picture. If I
> would know the perfect answer, I would have just forced you to use it
> ;)
I really don't have a strong opinion one way or another, both seem good
enough to me, but since ENOENT has you feeling a bit uneasy I'll change
this to EBADSLT for the next revision (which will hopefully be the last
one for this error code issue).
Johan
^ permalink raw reply
* [RFC SBC] build: Add configure option --disable-high-precision
From: Luiz Augusto von Dentz @ 2013-09-16 9:22 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This enables high precision using 64 bits accumulators by default which
can be disabled with --disable-high-precision.
---
configure.ac | 8 ++++++++
sbc/sbc_tables.h | 2 --
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index c052616..5f994d1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -58,4 +58,12 @@ if (test "${enable_tester}" != "no"); then
fi
AM_CONDITIONAL(TESTER, test "${enable_tester}" != "no")
+AC_ARG_ENABLE(high-precision, AC_HELP_STRING([--disable-high-precision],
+ [disable high precision support]),
+ [enable_high_precision=${enableval}])
+if (test "${enable_high_precision}" != "no"); then
+ AC_DEFINE(SBC_HIGH_PRECISION, 1,
+ [Define to 1 to enable high precision build of SBC encoder])
+fi
+
AC_OUTPUT(Makefile sbc/sbc.pc)
diff --git a/sbc/sbc_tables.h b/sbc/sbc_tables.h
index 25e24e6..3fd80e1 100644
--- a/sbc/sbc_tables.h
+++ b/sbc/sbc_tables.h
@@ -136,8 +136,6 @@ static const int32_t synmatrix8[16][8] = {
SN8(0xfb8e3130), SN8(0xf8275a10), SN8(0xfe70747c), SN8(0x06a6d988) }
};
-/* Uncomment the following line to enable high precision build of SBC encoder */
-
/* #define SBC_HIGH_PRECISION */
#ifdef SBC_HIGH_PRECISION
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Johan Hedberg @ 2013-09-16 8:19 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <63CB3872-A353-4997-9A00-E746DA99E6C6@holtmann.org>
Hi Marcel,
On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> > @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> > }
> > EXPORT_SYMBOL(bt_sock_wait_state);
> >
>
> add a small comment here that this needs to be called with sk lock held.
I suppose I should do that to the old bt_sock_wait_state function too?
(which btw my wait_ready function is essentially a copy of, with the
exception of which condition it looks for)
> > +int bt_sock_wait_ready(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_ready);
> > +
> > #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..ad95b42 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_ready(sk, msg->msg_flags);
> > + release_sock(sk);
> > + if (err)
> > + return err;
> > +
>
> After starting to look into this now, I am not sure we have proper
> locking in that function in the first place. Can you check that it is
> actually fine not holding the socket lock for the send operation
> itself.
What procedure did you have in mind for checking what's fine and what's
not? Some quick googling and trying to look at the code of other address
families didn't help me getting much wiser about this.
This is the way that RFCOMM and L2CAP sockets seem to always have done
locking and we haven't seen any obvious breakage because of it. The
RFCOMM side does indeed use lock_sock for the send operation and L2CAP
doesn't. However, I think the reason it works is that l2cap_chan_lock
(which is used before calling l2cap_chan_send) serves a similar purpose.
> > --- 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,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> >
> > lock_sock(sk);
> >
> > + err = bt_sock_wait_ready(sk, msg->msg_flags);
> > + if (err) {
> > + release_sock(sk);
> > + return err;
> > + }
> > +
>
> I would be fine with using the sent variable here. And then of course
> create a label for the failure case. Assuming bt_sock_wait_ready()
> returning 0 on success you could use it to initialize sent here.
>
> However at minimum, I want a label here to jump at the end of the
> function in error case. I like to keep the number of lock_sock +
> release_sock balanced.
Makes sense. I'll do these improvements to the next version.
> Which is something that needs to be fixed for l2cap_sock_recvmsg()
> actually.
I'll see if I can roll up a separate patch for that.
Johan
^ permalink raw reply
* Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: Marcel Holtmann @ 2013-09-16 0:17 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20130915200021.GA8409@x220.p-661hnu-f1>
Hi Johan,
>>>>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
>>>>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
>>>>>>> that error code instead of ENOENT.
>>>>>>>
>>>>>>> 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 9ec1561f..d134501 100644
>>>>>>> --- a/net/bluetooth/l2cap_core.c
>>>>>>> +++ b/net/bluetooth/l2cap_core.c
>>>>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>>>>>>>
>>>>>>> chan = l2cap_get_chan_by_scid(conn, dcid);
>>>>>>> if (!chan)
>>>>>>> - return -ENOENT;
>>>>>>> + return -EFAULT;
>>>>>>
>>>>>> do we really want to use EFAULT. That is normally only used if a
>>>>>> memory copy from userspace to kernel or vice versa fails.
>>>>>
>>>>> EFAULT seemed to be the most common error code in this case, but now
>>>>> that I did some research it seems it's mostly from newish AMP related
>>>>> code. Would you be ok if I just switch all places to use ENOENT then?
>>>>
>>>> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
>>>>
>>>> I am fine with keep using ENOENT if we used that before. If that is
>>>> what the original code used, then lets keep using it. Otherwise we
>>>> better find a more descriptive error.
>>>
>>> If we ignore new AMP code there's only one "legacy" reference for this
>>> kind of situation which is l2cap_config_req. Since it has always used
>>> ENOENT for a failed CID lookup I'll follow that lead and use the same
>>> error for all places that need it.
>>
>> have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a
>> better choice.
>
> I'd rather have a consensus here first before resending the entire patch
> set yet another time because of this issue (which, let's face it, is not
> about any public API but consistency inside a single c-file).
>
> As quite often with trying to map POSIX error codes to our own domain
> we've ended up here with trying to find the "least bad" option in the
> absence of a single obviously correct one.
>
> EBADRQC sounds like it's not related to PDU parameters but the request
> code of the PDU. EBADSLT is "invalid slot", so if we consider the CID to
> be a "slot" this might be an option. EBADR talks about a "request
> descriptor" which I don't quite understand how it's different from a
> "request code". EBADE means "invalid exchange" whereas this isn't really
> about an exchange but just an incorrect CID in a PDU.
>
> I agree that ENOENT isn't perfect either since, even though internally
> this is about a failed lookup, externally this is just the peer sending
> us an invalid parameter value. Therefore, we could even consider EINVAL
> if it wasn't for the fact that it's already used e.g. when we receive an
> unknown opcode.
>
> In the end I'm willing to accept pretty much any of the proposed options
> since there really isn't a single right answer to this and since this is
> not about public API that we really need to stick to once we decide
> which value to use. The important thing is that the code is kept
> consistent regarding the use of this error.
I have nothing against ENOENT. I however am a bit concerned about that we accidentally use ENOENT in some other case and end up overloading its meaning.
So just for the sake of making this unique, I would consider going with EBADSLT. I am not married to this at all. However I wanted to have this talked through so that we understand the whole picture. If I would know the perfect answer, I would have just forced you to use it ;)
If you want to keep using ENOENT, then I will sign off your current patches.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: Johan Hedberg @ 2013-09-15 20:00 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <5A99E897-60EC-486C-94D9-775B3C01FB16@holtmann.org>
Hi Marcel,
On Sun, Sep 15, 2013, Marcel Holtmann wrote:
> >>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
> >>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
> >>>>> that error code instead of ENOENT.
> >>>>>
> >>>>> 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 9ec1561f..d134501 100644
> >>>>> --- a/net/bluetooth/l2cap_core.c
> >>>>> +++ b/net/bluetooth/l2cap_core.c
> >>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> >>>>>
> >>>>> chan = l2cap_get_chan_by_scid(conn, dcid);
> >>>>> if (!chan)
> >>>>> - return -ENOENT;
> >>>>> + return -EFAULT;
> >>>>
> >>>> do we really want to use EFAULT. That is normally only used if a
> >>>> memory copy from userspace to kernel or vice versa fails.
> >>>
> >>> EFAULT seemed to be the most common error code in this case, but now
> >>> that I did some research it seems it's mostly from newish AMP related
> >>> code. Would you be ok if I just switch all places to use ENOENT then?
> >>
> >> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
> >>
> >> I am fine with keep using ENOENT if we used that before. If that is
> >> what the original code used, then lets keep using it. Otherwise we
> >> better find a more descriptive error.
> >
> > If we ignore new AMP code there's only one "legacy" reference for this
> > kind of situation which is l2cap_config_req. Since it has always used
> > ENOENT for a failed CID lookup I'll follow that lead and use the same
> > error for all places that need it.
>
> have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a
> better choice.
I'd rather have a consensus here first before resending the entire patch
set yet another time because of this issue (which, let's face it, is not
about any public API but consistency inside a single c-file).
As quite often with trying to map POSIX error codes to our own domain
we've ended up here with trying to find the "least bad" option in the
absence of a single obviously correct one.
EBADRQC sounds like it's not related to PDU parameters but the request
code of the PDU. EBADSLT is "invalid slot", so if we consider the CID to
be a "slot" this might be an option. EBADR talks about a "request
descriptor" which I don't quite understand how it's different from a
"request code". EBADE means "invalid exchange" whereas this isn't really
about an exchange but just an incorrect CID in a PDU.
I agree that ENOENT isn't perfect either since, even though internally
this is about a failed lookup, externally this is just the peer sending
us an invalid parameter value. Therefore, we could even consider EINVAL
if it wasn't for the fact that it's already used e.g. when we receive an
unknown opcode.
In the end I'm willing to accept pretty much any of the proposed options
since there really isn't a single right answer to this and since this is
not about public API that we really need to stick to once we decide
which value to use. The important thing is that the code is kept
consistent regarding the use of this error.
Johan
^ permalink raw reply
* Re: [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Marcel Holtmann @ 2013-09-15 19:15 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379269001-6313-9-git-send-email-johan.hedberg@gmail.com>
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_ready 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 | 39 +++++++++++++++++++++++++++++++++++++++
> net/bluetooth/l2cap_sock.c | 6 ++++++
> net/bluetooth/rfcomm/sock.c | 9 +++++++--
> 4 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 10d43d8..afbc711 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_ready(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..a883171c 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> }
> EXPORT_SYMBOL(bt_sock_wait_state);
>
add a small comment here that this needs to be called with sk lock held.
> +int bt_sock_wait_ready(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_ready);
> +
> #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..ad95b42 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_ready(sk, msg->msg_flags);
> + release_sock(sk);
> + if (err)
> + return err;
> +
After starting to look into this now, I am not sure we have proper locking in that function in the first place. Can you check that it is actually fine not holding the socket lock for the send operation itself.
> 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..02d96e4 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,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
>
> lock_sock(sk);
>
> + err = bt_sock_wait_ready(sk, msg->msg_flags);
> + if (err) {
> + release_sock(sk);
> + return err;
> + }
> +
I would be fine with using the sent variable here. And then of course create a label for the failure case. Assuming bt_sock_wait_ready() returning 0 on success you could use it to initialize sent here.
However at minimum, I want a label here to jump at the end of the function in error case. I like to keep the number of lock_sock + release_sock balanced.
Which is something that needs to be fixed for l2cap_sock_recvmsg() actually.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v4 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req
From: Marcel Holtmann @ 2013-09-15 19:05 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379269001-6313-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> When an L2CAP request handler returns non-zero the calling code will
> send a command reject response. The l2cap_create_chan_req function will
> in some cases send its own response but then still return a -EFAULT
> error which would cause two responses to be sent. This patch fixes this
> by making the function return 0 after sending its own response.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: Marcel Holtmann @ 2013-09-15 19:04 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20130915181552.GA6383@x220.p-661hnu-f1>
Hi Johan,
>>>>> The convention of the L2CAP code is to return EFAULT when a CID in the
>>>>> request packet is invalid. This patch fixes the l2cap_config_req to use
>>>>> that error code instead of ENOENT.
>>>>>
>>>>> 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 9ec1561f..d134501 100644
>>>>> --- a/net/bluetooth/l2cap_core.c
>>>>> +++ b/net/bluetooth/l2cap_core.c
>>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>>>>>
>>>>> chan = l2cap_get_chan_by_scid(conn, dcid);
>>>>> if (!chan)
>>>>> - return -ENOENT;
>>>>> + return -EFAULT;
>>>>
>>>> do we really want to use EFAULT. That is normally only used if a
>>>> memory copy from userspace to kernel or vice versa fails.
>>>
>>> EFAULT seemed to be the most common error code in this case, but now
>>> that I did some research it seems it's mostly from newish AMP related
>>> code. Would you be ok if I just switch all places to use ENOENT then?
>>
>> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors.
>>
>> I am fine with keep using ENOENT if we used that before. If that is
>> what the original code used, then lets keep using it. Otherwise we
>> better find a more descriptive error.
>
> If we ignore new AMP code there's only one "legacy" reference for this
> kind of situation which is l2cap_config_req. Since it has always used
> ENOENT for a failed CID lookup I'll follow that lead and use the same
> error for all places that need it.
have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a better choice.
Regards
Marcel
^ permalink raw reply
* [PATCH v4 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
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_ready 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 | 39 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/l2cap_sock.c | 6 ++++++
net/bluetooth/rfcomm/sock.c | 9 +++++++--
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10d43d8..afbc711 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_ready(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..a883171c 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
}
EXPORT_SYMBOL(bt_sock_wait_state);
+int bt_sock_wait_ready(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_ready);
+
#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..ad95b42 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_ready(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..02d96e4 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,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
lock_sock(sk);
+ err = bt_sock_wait_ready(sk, msg->msg_flags);
+ if (err) {
+ release_sock(sk);
+ 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
* [PATCH v4 7/8] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
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 020b723..fe40d3e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static __le16 l2cap_err_to_reason(int err)
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;
@@ -5318,6 +5319,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);
@@ -5355,6 +5359,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;
@@ -5362,6 +5367,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
* [PATCH v4 6/8] Bluetooth: Fix sending responses to identified L2CAP response packets
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When L2CAP packets return a non-zero error and the value is passed
onwards by l2cap_bredr_sig_cmd this will trigger a command reject packet
to be sent. However, the core specification (page 1416 in core 4.0) says
the following: "Command Reject packets should not be sent in response to
an identified Response packet.".
This patch ensures that a command reject packet is not sent for any
identified response packet by ignoring the error return value from the
response handler functions.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3b0633f..020b723 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5212,7 +5212,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
case L2CAP_CONN_RSP:
case L2CAP_CREATE_CHAN_RSP:
- err = l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
+ l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CONF_REQ:
@@ -5220,7 +5220,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_CONF_RSP:
- err = l2cap_config_rsp(conn, cmd, cmd_len, data);
+ l2cap_config_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_DISCONN_REQ:
@@ -5228,7 +5228,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_DISCONN_RSP:
- err = l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
+ l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_ECHO_REQ:
@@ -5243,7 +5243,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_INFO_RSP:
- err = l2cap_information_rsp(conn, cmd, cmd_len, data);
+ l2cap_information_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CREATE_CHAN_REQ:
@@ -5255,7 +5255,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_RSP:
- err = l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_MOVE_CHAN_CFM:
@@ -5263,7 +5263,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_CFM_RSP:
- err = l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
break;
default:
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 5/8] Bluetooth: Fix L2CAP error return used for failed channel lookups
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The EFAULT error should only be used for memory address related errors
whereas ENOENT should be used for failed lookups for the given CID. This
patch fixes the l2cap_connect_create_rsp and l2cap_create_channel_req
handlers to use the correct ENOENT error return.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b1a656..3b0633f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3884,13 +3884,13 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
if (scid) {
chan = __l2cap_get_chan_by_scid(conn, scid);
if (!chan) {
- err = -EFAULT;
+ err = -ENOENT;
goto unlock;
}
} else {
chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
if (!chan) {
- err = -EFAULT;
+ err = -ENOENT;
goto unlock;
}
}
@@ -4438,7 +4438,7 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
hs_hcon = hci_conn_hash_lookup_ba(hdev, AMP_LINK, conn->dst);
if (!hs_hcon) {
hci_dev_put(hdev);
- return -EFAULT;
+ return -ENOENT;
}
BT_DBG("mgr %p bredr_chan %p hs_hcon %p", mgr, chan, hs_hcon);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
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 8c718dd..0b1a656 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 -ENOENT;
}
l2cap_chan_lock(chan);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 3/8] Bluetooth: Fix L2CAP command reject reason
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
There are several possible reason codes that can be sent in the command
reject L2CAP packet. Before this patch the code has used a hard-coded
single response code ("command not understood"). This patch adds a
helper function to map the return value of an L2CAP handler function to
the correct command reject reason.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0b8a270..8c718dd 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5294,6 +5294,20 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
}
}
+static __le16 l2cap_err_to_reason(int err)
+{
+ switch (err) {
+ case -ENOENT:
+ return __constant_cpu_to_le16(L2CAP_REJ_INVALID_CID);
+ case -EMSGSIZE:
+ return __constant_cpu_to_le16(L2CAP_REJ_MTU_EXCEEDED);
+ case -EINVAL:
+ case -EPROTO:
+ default:
+ return __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ }
+}
+
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
@@ -5326,8 +5340,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
@@ -5371,8 +5384,7 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = l2cap_err_to_reason(err);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 2/8] Bluetooth: Fix double error response for l2cap_create_chan_req
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When an L2CAP request handler returns non-zero the calling code will
send a command reject response. The l2cap_create_chan_req function will
in some cases send its own response but then still return a -EFAULT
error which would cause two responses to be sent. This patch fixes this
by making the function return 0 after sending its own 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..0b8a270 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4462,7 +4462,7 @@ error:
l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
sizeof(rsp), &rsp);
- return -EFAULT;
+ return 0;
}
static void l2cap_send_move_chan_req(struct l2cap_chan *chan, u8 dest_amp_id)
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 1/8] Bluetooth: Remove unused event mask struct
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379269001-6313-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The struct for HCI_Set_Event_Mask is never used. Instead a local 8-byte
array is used for sending this command. Therefore, remove the
unnecessary struct definition.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 57aa408..7ede266 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -695,9 +695,6 @@ struct hci_cp_sniff_subrate {
} __packed;
#define HCI_OP_SET_EVENT_MASK 0x0c01
-struct hci_cp_set_event_mask {
- __u8 mask[8];
-} __packed;
#define HCI_OP_RESET 0x0c03
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v4 0/8] Bluetooth: Various L2CAP fixes
From: johan.hedberg @ 2013-09-15 18:16 UTC (permalink / raw)
To: linux-bluetooth
Hi,
Here's a fourth revision of this patch set with all feedback received so
far taken into account.
----------------------------------------------------------------
Johan Hedberg (8):
Bluetooth: Remove unused event mask struct
Bluetooth: Fix double error response for l2cap_create_chan_req
Bluetooth: Fix L2CAP command reject reason
Bluetooth: Fix L2CAP Disconnect response for unknown CID
Bluetooth: Fix L2CAP error return used for failed channel lookups
Bluetooth: Fix sending responses to identified L2CAP response packets
Bluetooth: Fix responding to invalid L2CAP signaling commands
Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/hci.h | 3 ---
net/bluetooth/af_bluetooth.c | 39 +++++++++++++++++++++++++++++
net/bluetooth/l2cap_core.c | 50 ++++++++++++++++++++++++++-----------
net/bluetooth/l2cap_sock.c | 6 +++++
net/bluetooth/rfcomm/sock.c | 9 +++++--
6 files changed, 88 insertions(+), 20 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox