Initial set of Management interface patches

Initial set of Management interface patches

[johan.hedberg]

Hi,

Here's an initial (short) set of patches for the upcoming management
interface which should hopefully be quite straght forward. Since
bluetoothd will nowdays automatically pick up and try to use this
interface it's disabled by default and can be enabled with the
enable_mgmt module parameter. I.e. it should be safe to push these
towards upstream trees. Once I've received acks on these I'll start
preparing the patches which add some real functionality (i.e. message
handling) for managment sockets.

Johan

[PATCH 1/3] Bluetooth: Add Bluetooth Management interface definitions

[johan.hedberg]

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

Add initial definitions for the new Bluetooth Management interface to
the bluetooth headers.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/hci.h      |    4 +++
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |   46 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 include/net/bluetooth/mgmt.h

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e30e008..bf39eef 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -934,9 +934,13 @@ static inline struct hci_sco_hdr *hci_sco_hdr(const struct sk_buff *skb)
 struct sockaddr_hci {
 	sa_family_t    hci_family;
 	unsigned short hci_dev;
+	unsigned short hci_channel;
 };
 #define HCI_DEV_NONE	0xffff
 
+#define HCI_CHANNEL_RAW		0
+#define HCI_CHANNEL_CONTROL	1
+
 struct hci_filter {
 	unsigned long type_mask;
 	unsigned long event_mask[2];
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ebec8c9..b8104af 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -668,6 +668,7 @@ struct hci_pinfo {
 	struct hci_dev    *hdev;
 	struct hci_filter filter;
 	__u32             cmsg_mask;
+	unsigned short   channel;
 };
 
 /* HCI security filter */
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
new file mode 100644
index 0000000..95974da
--- /dev/null
+++ b/include/net/bluetooth/mgmt.h
@@ -0,0 +1,46 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+struct mgmt_hdr {
+	__le16 opcode;
+	__le16 len;
+} __packed;
+#define MGMT_HDR_SIZE			4
+
+#define MGMT_EV_CMD_COMPLETE		0x0001
+struct mgmt_ev_cmd_complete {
+	__le16 opcode;
+	__u8 data[0];
+} __packed;
+
+#define MGMT_EV_CMD_STATUS		0x0002
+struct mgmt_ev_cmd_status {
+	__u8 status;
+	__le16 opcode;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_ERROR	0x0003
+struct mgmt_ev_controller_error {
+	__le16 index;
+	__u8 error_code;
+} __packed;
-- 
1.7.2.3

[PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[johan.hedberg]

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

Add initial code for handling Bluetooth Management interface messages.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/hci_core.h |    3 +
 net/bluetooth/Makefile           |    2 +-
 net/bluetooth/hci_sock.c         |   39 +++++++++++++--
 net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 7 deletions(-)
 create mode 100644 net/bluetooth/mgmt.c

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b8104af..dd1573da 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -660,6 +660,9 @@ void hci_si_event(struct hci_dev *hdev, int type, int dlen, void *data);
 /* ----- HCI Sockets ----- */
 void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb);
 
+/* Management interface */
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
+
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
 
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index d1e433f..8b411d9 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -10,4 +10,4 @@ obj-$(CONFIG_BT_BNEP)	+= bnep/
 obj-$(CONFIG_BT_CMTP)	+= cmtp/
 obj-$(CONFIG_BT_HIDP)	+= hidp/
 
-bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o hci_sock.o hci_sysfs.o lib.o
+bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o hci_sock.o hci_sysfs.o lib.o
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 83acd16..730a4e8 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -49,6 +49,8 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+static int enable_mgmt = 0;
+
 /* ----- HCI socket interface ----- */
 
 static inline int hci_test_bit(int nr, void *addr)
@@ -352,25 +354,35 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long a
 
 static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 {
-	struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
+	struct sockaddr_hci haddr;
 	struct sock *sk = sock->sk;
 	struct hci_dev *hdev = NULL;
-	int err = 0;
+	int len, err = 0;
 
 	BT_DBG("sock %p sk %p", sock, sk);
 
-	if (!haddr || haddr->hci_family != AF_BLUETOOTH)
+	if (!addr)
+		return -EINVAL;
+
+	memset(&haddr, 0, sizeof(haddr));
+	len = min_t(unsigned int, sizeof(haddr), addr_len);
+	memcpy(&haddr, addr, len);
+
+	if (haddr.hci_family != AF_BLUETOOTH)
+		return -EINVAL;
+
+	if (haddr.hci_channel != HCI_CHANNEL_RAW && !enable_mgmt)
 		return -EINVAL;
 
 	lock_sock(sk);
 
-	if (hci_pi(sk)->hdev) {
+	if (sk->sk_state == BT_BOUND || hci_pi(sk)->hdev) {
 		err = -EALREADY;
 		goto done;
 	}
 
-	if (haddr->hci_dev != HCI_DEV_NONE) {
-		if (!(hdev = hci_dev_get(haddr->hci_dev))) {
+	if (haddr.hci_dev != HCI_DEV_NONE) {
+		if (!(hdev = hci_dev_get(haddr.hci_dev))) {
 			err = -ENODEV;
 			goto done;
 		}
@@ -378,6 +390,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		atomic_inc(&hdev->promisc);
 	}
 
+	hci_pi(sk)->channel = haddr.hci_channel;
 	hci_pi(sk)->hdev = hdev;
 	sk->sk_state = BT_BOUND;
 
@@ -499,6 +512,17 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	switch (hci_pi(sk)->channel) {
+	case HCI_CHANNEL_RAW:
+		break;
+	case HCI_CHANNEL_CONTROL:
+		err = mgmt_control(sk, msg, len);
+		goto done;
+	default:
+		err = -EINVAL;
+		goto done;
+	}
+
 	if (!(hdev = hci_pi(sk)->hdev)) {
 		err = -EBADFD;
 		goto done;
@@ -826,3 +850,6 @@ void __exit hci_sock_cleanup(void)
 
 	proto_unregister(&hci_sk_proto);
 }
+
+module_param(enable_mgmt, bool, 0644);
+MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
new file mode 100644
index 0000000..78255f1
--- /dev/null
+++ b/net/bluetooth/mgmt.c
@@ -0,0 +1,99 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+/* Bluetooth HCI Management interface */
+
+#include <asm/uaccess.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/mgmt.h>
+
+static void cmd_status(struct sock *sk, u16 cmd, u8 status)
+{
+	struct sk_buff *skb;
+	struct mgmt_hdr *hdr;
+	struct mgmt_ev_cmd_status *ev;
+
+	BT_DBG("sock %p", sk);
+
+	skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
+
+	hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
+	hdr->len = cpu_to_le16(3);
+
+	ev = (void *) skb_put(skb, sizeof(*ev));
+	ev->status = status;
+	put_unaligned_le16(cmd, &ev->opcode);
+
+	if (sock_queue_rcv_skb(sk, skb) < 0)
+		kfree_skb(skb);
+}
+
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
+{
+	unsigned char *buf;
+	struct mgmt_hdr *hdr;
+	u16 opcode, len;
+	int err;
+
+	BT_DBG("got %zu bytes", msglen);
+
+	if (msglen < sizeof(*hdr))
+		return -EINVAL;
+
+	buf = kmalloc(msglen, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
+		err = -EFAULT;
+		goto done;
+	}
+
+	hdr = (struct mgmt_hdr *) buf;
+	opcode = get_unaligned_le16(&hdr->opcode);
+	len = get_unaligned_le16(&hdr->len);
+
+	if (len != msglen - sizeof(struct mgmt_hdr)) {
+		err = -EINVAL;
+		goto done;
+	}
+
+	switch (opcode) {
+	default:
+		BT_DBG("Unknown op %u", opcode);
+		cmd_status(sk, opcode, 0x01);
+		break;
+	}
+
+	err = msglen;
+
+done:
+	kfree(buf);
+	return err;
+}
-- 
1.7.2.3

[PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[johan.hedberg]

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

In order to send data to management control sockets the function should:

  - skip checks intended for raw HCI data and stack internal events
  - make sure RAW HCI data or stack internal events don't go to
    management control sockets

In order to accomplish this the patch adds a new member to the bluetooth
skb private data to flag skb's that are destined for management control
sockets.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/bluetooth.h |    1 +
 net/bluetooth/hci_sock.c          |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index d81ea79..0c5e725 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -144,6 +144,7 @@ struct bt_skb_cb {
 	__u8 tx_seq;
 	__u8 retries;
 	__u8 sar;
+	unsigned short channel;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)((skb)->cb))
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 730a4e8..ab449b2 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -104,6 +104,12 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 		if (skb->sk == sk)
 			continue;
 
+		if (bt_cb(skb)->channel != hci_pi(sk)->channel)
+			continue;
+
+		if (bt_cb(skb)->channel == HCI_CHANNEL_CONTROL)
+			goto clone;
+
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
@@ -127,11 +133,13 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 				continue;
 		}
 
+clone:
 		if (!(nskb = skb_clone(skb, GFP_ATOMIC)))
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
+		if (bt_cb(skb)->channel == HCI_CHANNEL_RAW)
+			memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
-- 
1.7.2.3

Re: [PATCH 1/3] Bluetooth: Add Bluetooth Management interface definitions

[Marcel Holtmann]

Hi Johan,

> Add initial definitions for the new Bluetooth Management interface to
> the bluetooth headers.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  include/net/bluetooth/hci.h      |    4 +++
>  include/net/bluetooth/hci_core.h |    1 +
>  include/net/bluetooth/mgmt.h     |   46 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+), 0 deletions(-)
>  create mode 100644 include/net/bluetooth/mgmt.h

looks fine to me.

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

Regards

Marcel

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Marcel Holtmann]

Hi Johan,

> Add initial code for handling Bluetooth Management interface messages.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  include/net/bluetooth/hci_core.h |    3 +
>  net/bluetooth/Makefile           |    2 +-
>  net/bluetooth/hci_sock.c         |   39 +++++++++++++--
>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 136 insertions(+), 7 deletions(-)
>  create mode 100644 net/bluetooth/mgmt.c

looks good.

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

Regards

Marcel

Re: [PATCH 3/3] Bluetooth: Make hci_send_to_sock usable for management control sockets

[Marcel Holtmann]

Hi Johan,

> In order to send data to management control sockets the function should:
> 
>   - skip checks intended for raw HCI data and stack internal events
>   - make sure RAW HCI data or stack internal events don't go to
>     management control sockets
> 
> In order to accomplish this the patch adds a new member to the bluetooth
> skb private data to flag skb's that are destined for management control
> sockets.

looks fine to me.

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

Regards

Marcel

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Andrei Emeltchenko]

Hi,

On Wed, Nov 24, 2010 at 4:39 PM,  <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@nokia.com>
>
> Add initial code for handling Bluetooth Management interface messages.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  include/net/bluetooth/hci_core.h |    3 +
>  net/bluetooth/Makefile           |    2 +-
>  net/bluetooth/hci_sock.c         |   39 +++++++++++++--
>  net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 136 insertions(+), 7 deletions(-)
>  create mode 100644 net/bluetooth/mgmt.c
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index b8104af..dd1573da 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -660,6 +660,9 @@ void hci_si_event(struct hci_dev *hdev, int type, int dlen, void *data);
>  /* ----- HCI Sockets ----- */
>  void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb);
>
> +/* Management interface */
> +int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
> +
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
>
> diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
> index d1e433f..8b411d9 100644
> --- a/net/bluetooth/Makefile
> +++ b/net/bluetooth/Makefile
> @@ -10,4 +10,4 @@ obj-$(CONFIG_BT_BNEP) += bnep/
>  obj-$(CONFIG_BT_CMTP)  += cmtp/
>  obj-$(CONFIG_BT_HIDP)  += hidp/
>
> -bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o hci_sock.o hci_sysfs.o lib.o
> +bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o hci_sock.o hci_sysfs.o lib.o
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 83acd16..730a4e8 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -49,6 +49,8 @@
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
>
> +static int enable_mgmt = 0;
> +
>  /* ----- HCI socket interface ----- */
>
>  static inline int hci_test_bit(int nr, void *addr)
> @@ -352,25 +354,35 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long a
>
>  static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
>  {
> -       struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
> +       struct sockaddr_hci haddr;
>        struct sock *sk = sock->sk;
>        struct hci_dev *hdev = NULL;
> -       int err = 0;
> +       int len, err = 0;
>
>        BT_DBG("sock %p sk %p", sock, sk);
>
> -       if (!haddr || haddr->hci_family != AF_BLUETOOTH)
> +       if (!addr)
> +               return -EINVAL;
> +
> +       memset(&haddr, 0, sizeof(haddr));
> +       len = min_t(unsigned int, sizeof(haddr), addr_len);
> +       memcpy(&haddr, addr, len);
> +
> +       if (haddr.hci_family != AF_BLUETOOTH)
> +               return -EINVAL;
> +
> +       if (haddr.hci_channel != HCI_CHANNEL_RAW && !enable_mgmt)
>                return -EINVAL;
>
>        lock_sock(sk);
>
> -       if (hci_pi(sk)->hdev) {
> +       if (sk->sk_state == BT_BOUND || hci_pi(sk)->hdev) {
>                err = -EALREADY;
>                goto done;
>        }
>
> -       if (haddr->hci_dev != HCI_DEV_NONE) {
> -               if (!(hdev = hci_dev_get(haddr->hci_dev))) {
> +       if (haddr.hci_dev != HCI_DEV_NONE) {
> +               if (!(hdev = hci_dev_get(haddr.hci_dev))) {

doesn't checkpatch give errors here? Would be more clean like:
...
hdev = hci_dev_get(haddr.hci_dev);
if (!hdev)
...

At some point shall be fixed in the old code also

otherwise looks fine

Acked-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

>                        err = -ENODEV;
>                        goto done;
>                }
> @@ -378,6 +390,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
>                atomic_inc(&hdev->promisc);
>        }
>
> +       hci_pi(sk)->channel = haddr.hci_channel;
>        hci_pi(sk)->hdev = hdev;
>        sk->sk_state = BT_BOUND;
>
> @@ -499,6 +512,17 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
>
>        lock_sock(sk);
>
> +       switch (hci_pi(sk)->channel) {
> +       case HCI_CHANNEL_RAW:
> +               break;
> +       case HCI_CHANNEL_CONTROL:
> +               err = mgmt_control(sk, msg, len);
> +               goto done;
> +       default:
> +               err = -EINVAL;
> +               goto done;
> +       }
> +
>        if (!(hdev = hci_pi(sk)->hdev)) {
>                err = -EBADFD;
>                goto done;
> @@ -826,3 +850,6 @@ void __exit hci_sock_cleanup(void)
>
>        proto_unregister(&hci_sk_proto);
>  }
> +
> +module_param(enable_mgmt, bool, 0644);
> +MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> new file mode 100644
> index 0000000..78255f1
> --- /dev/null
> +++ b/net/bluetooth/mgmt.c
> @@ -0,0 +1,99 @@
> +/*
> +   BlueZ - Bluetooth protocol stack for Linux
> +   Copyright (C) 2010  Nokia Corporation
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License version 2 as
> +   published by the Free Software Foundation;
> +
> +   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> +   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
> +   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
> +   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
> +   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +
> +   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
> +   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
> +   SOFTWARE IS DISCLAIMED.
> +*/
> +
> +/* Bluetooth HCI Management interface */
> +
> +#include <asm/uaccess.h>
> +#include <asm/unaligned.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +#include <net/bluetooth/mgmt.h>
> +
> +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> +{
> +       struct sk_buff *skb;
> +       struct mgmt_hdr *hdr;
> +       struct mgmt_ev_cmd_status *ev;
> +
> +       BT_DBG("sock %p", sk);
> +
> +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
> +       if (!skb)
> +               return;
> +
> +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
> +
> +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> +       hdr->len = cpu_to_le16(3);
> +
> +       ev = (void *) skb_put(skb, sizeof(*ev));
> +       ev->status = status;
> +       put_unaligned_le16(cmd, &ev->opcode);
> +
> +       if (sock_queue_rcv_skb(sk, skb) < 0)
> +               kfree_skb(skb);
> +}
> +
> +int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> +{
> +       unsigned char *buf;
> +       struct mgmt_hdr *hdr;
> +       u16 opcode, len;
> +       int err;
> +
> +       BT_DBG("got %zu bytes", msglen);
> +
> +       if (msglen < sizeof(*hdr))
> +               return -EINVAL;
> +
> +       buf = kmalloc(msglen, GFP_ATOMIC);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
> +               err = -EFAULT;
> +               goto done;
> +       }
> +
> +       hdr = (struct mgmt_hdr *) buf;
> +       opcode = get_unaligned_le16(&hdr->opcode);
> +       len = get_unaligned_le16(&hdr->len);
> +
> +       if (len != msglen - sizeof(struct mgmt_hdr)) {
> +               err = -EINVAL;
> +               goto done;
> +       }
> +
> +       switch (opcode) {
> +       default:
> +               BT_DBG("Unknown op %u", opcode);
> +               cmd_status(sk, opcode, 0x01);
> +               break;
> +       }
> +
> +       err = msglen;
> +
> +done:
> +       kfree(buf);
> +       return err;
> +}
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Anderson Lizardo]

Hi Johan,

On Wed, Nov 24, 2010 at 10:39 AM,  <johan.hedberg@gmail.com> wrote:
>  static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
>  {
> -       struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
> +       struct sockaddr_hci haddr;

Just out of curiosity: why is this change necessary (i.e. make a stack
copy of addr data and use it instead of using a cast of addr)?

>        struct sock *sk = sock->sk;
>        struct hci_dev *hdev = NULL;
> -       int err = 0;
> +       int len, err = 0;
>
>        BT_DBG("sock %p sk %p", sock, sk);
>
> -       if (!haddr || haddr->hci_family != AF_BLUETOOTH)
> +       if (!addr)
> +               return -EINVAL;
> +
> +       memset(&haddr, 0, sizeof(haddr));
> +       len = min_t(unsigned int, sizeof(haddr), addr_len);
> +       memcpy(&haddr, addr, len);

Looks like you are playing safe here, but looking at least a few
->bind() implementations I see most just cast the original struct
sockaddr, which is has size (sizeof(unsigned short) + 14).

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Gustavo F. Padovan]

Hi Anderson,

* Anderson Lizardo <anderson.lizardo@openbossa.org> [2010-11-24 11:38:22 -0400]:

> Hi Johan,
> 
> On Wed, Nov 24, 2010 at 10:39 AM,  <johan.hedberg@gmail.com> wrote:
> >  static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> >  {
> > -       struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
> > +       struct sockaddr_hci haddr;
> 
> Just out of curiosity: why is this change necessary (i.e. make a stack
> copy of addr data and use it instead of using a cast of addr)?
> 
> >        struct sock *sk = sock->sk;
> >        struct hci_dev *hdev = NULL;
> > -       int err = 0;
> > +       int len, err = 0;
> >
> >        BT_DBG("sock %p sk %p", sock, sk);
> >
> > -       if (!haddr || haddr->hci_family != AF_BLUETOOTH)
> > +       if (!addr)
> > +               return -EINVAL;
> > +
> > +       memset(&haddr, 0, sizeof(haddr));
> > +       len = min_t(unsigned int, sizeof(haddr), addr_len);
> > +       memcpy(&haddr, addr, len);
> 
> Looks like you are playing safe here, but looking at least a few
> ->bind() implementations I see most just cast the original struct
> sockaddr, which is has size (sizeof(unsigned short) + 14).

Older userspace versions can use smaller struct sockaddr, so it's a
better idea move to th stack and zero-filling the the struct before the
copy the data, this way if the size of the data copied is smaller than
the struct, the fields in the end of the struct will be filled with
zeros and not something stranger.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Johan Hedberg]

Hi Andrei,

On Wed, Nov 24, 2010, Andrei Emeltchenko wrote:
> > -       if (haddr->hci_dev != HCI_DEV_NONE) {
> > -               if (!(hdev = hci_dev_get(haddr->hci_dev))) {
> > +       if (haddr.hci_dev != HCI_DEV_NONE) {
> > +               if (!(hdev = hci_dev_get(haddr.hci_dev))) {
> 
> doesn't checkpatch give errors here?

Probably, but I've understood that it's ok if it's the existing code
that contains the coding style issue.

> Would be more clean like:
> ...
> hdev = hci_dev_get(haddr.hci_dev);
> if (!hdev)
> ...
> 
> At some point shall be fixed in the old code also

Agreed. A separate code cleanup patch would be nice. I've intentionally
kept the old style to not mix coding style and functional changes into
the same patch and to make it clear that I'm not introducing any changes
to the code logic at this place.

Johan

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Gustavo F. Padovan]

* Johan Hedberg <johan.hedberg@gmail.com> [2010-11-24 23:47:47 +0200]:

> Hi Andrei,
> 
> On Wed, Nov 24, 2010, Andrei Emeltchenko wrote:
> > > -       if (haddr->hci_dev != HCI_DEV_NONE) {
> > > -               if (!(hdev = hci_dev_get(haddr->hci_dev))) {
> > > +       if (haddr.hci_dev != HCI_DEV_NONE) {
> > > +               if (!(hdev = hci_dev_get(haddr.hci_dev))) {
> > 
> > doesn't checkpatch give errors here?
> 
> Probably, but I've understood that it's ok if it's the existing code
> that contains the coding style issue.
> 
> > Would be more clean like:
> > ...
> > hdev = hci_dev_get(haddr.hci_dev);
> > if (!hdev)
> > ...
> > 
> > At some point shall be fixed in the old code also
> 
> Agreed. A separate code cleanup patch would be nice. I've intentionally
> kept the old style to not mix coding style and functional changes into
> the same patch and to make it clear that I'm not introducing any changes
> to the code logic at this place.

Yes, that should be a separated patch.

Johan, your patches are fine, but I have to wait the wireless-next-2.6
be synced with the net-next-2.6 tree. Then I'll able to apply it.

-- 
Gustavo F. Padovan
http://profusion.mobi

[PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[johan.hedberg]

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

Add initial code for handling Bluetooth Management interface messages.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 include/net/bluetooth/hci_core.h |    3 +
 net/bluetooth/Makefile           |    2 +-
 net/bluetooth/hci_sock.c         |   39 +++++++++++++--
 net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 7 deletions(-)
 create mode 100644 net/bluetooth/mgmt.c

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3e34359..1992fac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -660,6 +660,9 @@ void hci_si_event(struct hci_dev *hdev, int type, int dlen, void *data);
 /* ----- HCI Sockets ----- */
 void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb);
 
+/* Management interface */
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
+
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
 
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index d1e433f..8b411d9 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -10,4 +10,4 @@ obj-$(CONFIG_BT_BNEP)	+= bnep/
 obj-$(CONFIG_BT_CMTP)	+= cmtp/
 obj-$(CONFIG_BT_HIDP)	+= hidp/
 
-bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o hci_sock.o hci_sysfs.o lib.o
+bluetooth-objs := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o hci_sock.o hci_sysfs.o lib.o
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b3753ba..207be7a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -49,6 +49,8 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+static int enable_mgmt;
+
 /* ----- HCI socket interface ----- */
 
 static inline int hci_test_bit(int nr, void *addr)
@@ -353,25 +355,35 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long a
 
 static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 {
-	struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
+	struct sockaddr_hci haddr;
 	struct sock *sk = sock->sk;
 	struct hci_dev *hdev = NULL;
-	int err = 0;
+	int len, err = 0;
 
 	BT_DBG("sock %p sk %p", sock, sk);
 
-	if (!haddr || haddr->hci_family != AF_BLUETOOTH)
+	if (!addr)
+		return -EINVAL;
+
+	memset(&haddr, 0, sizeof(haddr));
+	len = min_t(unsigned int, sizeof(haddr), addr_len);
+	memcpy(&haddr, addr, len);
+
+	if (haddr.hci_family != AF_BLUETOOTH)
+		return -EINVAL;
+
+	if (haddr.hci_channel != HCI_CHANNEL_RAW && !enable_mgmt)
 		return -EINVAL;
 
 	lock_sock(sk);
 
-	if (hci_pi(sk)->hdev) {
+	if (sk->sk_state == BT_BOUND || hci_pi(sk)->hdev) {
 		err = -EALREADY;
 		goto done;
 	}
 
-	if (haddr->hci_dev != HCI_DEV_NONE) {
-		hdev = hci_dev_get(haddr->hci_dev);
+	if (haddr.hci_dev != HCI_DEV_NONE) {
+		hdev = hci_dev_get(haddr.hci_dev);
 		if (!hdev) {
 			err = -ENODEV;
 			goto done;
@@ -380,6 +392,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		atomic_inc(&hdev->promisc);
 	}
 
+	hci_pi(sk)->channel = haddr.hci_channel;
 	hci_pi(sk)->hdev = hdev;
 	sk->sk_state = BT_BOUND;
 
@@ -502,6 +515,17 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	switch (hci_pi(sk)->channel) {
+	case HCI_CHANNEL_RAW:
+		break;
+	case HCI_CHANNEL_CONTROL:
+		err = mgmt_control(sk, msg, len);
+		goto done;
+	default:
+		err = -EINVAL;
+		goto done;
+	}
+
 	hdev = hci_pi(sk)->hdev;
 	if (!hdev) {
 		err = -EBADFD;
@@ -831,3 +855,6 @@ void __exit hci_sock_cleanup(void)
 
 	proto_unregister(&hci_sk_proto);
 }
+
+module_param(enable_mgmt, bool, 0644);
+MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
new file mode 100644
index 0000000..78255f1
--- /dev/null
+++ b/net/bluetooth/mgmt.c
@@ -0,0 +1,99 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+/* Bluetooth HCI Management interface */
+
+#include <asm/uaccess.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/mgmt.h>
+
+static void cmd_status(struct sock *sk, u16 cmd, u8 status)
+{
+	struct sk_buff *skb;
+	struct mgmt_hdr *hdr;
+	struct mgmt_ev_cmd_status *ev;
+
+	BT_DBG("sock %p", sk);
+
+	skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
+
+	hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
+	hdr->len = cpu_to_le16(3);
+
+	ev = (void *) skb_put(skb, sizeof(*ev));
+	ev->status = status;
+	put_unaligned_le16(cmd, &ev->opcode);
+
+	if (sock_queue_rcv_skb(sk, skb) < 0)
+		kfree_skb(skb);
+}
+
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
+{
+	unsigned char *buf;
+	struct mgmt_hdr *hdr;
+	u16 opcode, len;
+	int err;
+
+	BT_DBG("got %zu bytes", msglen);
+
+	if (msglen < sizeof(*hdr))
+		return -EINVAL;
+
+	buf = kmalloc(msglen, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
+		err = -EFAULT;
+		goto done;
+	}
+
+	hdr = (struct mgmt_hdr *) buf;
+	opcode = get_unaligned_le16(&hdr->opcode);
+	len = get_unaligned_le16(&hdr->len);
+
+	if (len != msglen - sizeof(struct mgmt_hdr)) {
+		err = -EINVAL;
+		goto done;
+	}
+
+	switch (opcode) {
+	default:
+		BT_DBG("Unknown op %u", opcode);
+		cmd_status(sk, opcode, 0x01);
+		break;
+	}
+
+	err = msglen;
+
+done:
+	kfree(buf);
+	return err;
+}
-- 
1.7.2.3

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Anderson Lizardo]

Hi Johan,

On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> +{

I see some inconsistence on how you calculate struct sizes on this
function. See below...

> +       struct sk_buff *skb;
> +       struct mgmt_hdr *hdr;
> +       struct mgmt_ev_cmd_status *ev;
> +
> +       BT_DBG("sock %p", sk);
> +
> +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);

Here you use sizeof(<var>)

> +       if (!skb)
> +               return;
> +
> +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));

But here you use sizeof(<struct>). Could be sizeof(*hdr) ? Note there
is also the (unused) MGMT_HDR_SIZE.

> +
> +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> +       hdr->len = cpu_to_le16(3);

and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)
? or maybe create a #define for it?

> +
> +       ev = (void *) skb_put(skb, sizeof(*ev));
> +       ev->status = status;
> +       put_unaligned_le16(cmd, &ev->opcode);
> +
> +       if (sock_queue_rcv_skb(sk, skb) < 0)
> +               kfree_skb(skb);
> +}
> +
> +int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> +{
> +       unsigned char *buf;
> +       struct mgmt_hdr *hdr;
> +       u16 opcode, len;
> +       int err;
> +
> +       BT_DBG("got %zu bytes", msglen);
> +
> +       if (msglen < sizeof(*hdr))
> +               return -EINVAL;
> +
> +       buf = kmalloc(msglen, GFP_ATOMIC);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
> +               err = -EFAULT;
> +               goto done;
> +       }
> +
> +       hdr = (struct mgmt_hdr *) buf;
> +       opcode = get_unaligned_le16(&hdr->opcode);
> +       len = get_unaligned_le16(&hdr->len);
> +
> +       if (len != msglen - sizeof(struct mgmt_hdr)) {

You could use sizeof(*hdr) here.

> +               err = -EINVAL;
> +               goto done;
> +       }
> +
> +       switch (opcode) {
> +       default:
> +               BT_DBG("Unknown op %u", opcode);
> +               cmd_status(sk, opcode, 0x01);
> +               break;
> +       }
> +
> +       err = msglen;

Would there be a chance of integer overflow here? The function returns
(signed) int, but msglen is (unsigned) size_t.

> +
> +done:
> +       kfree(buf);
> +       return err;
> +}

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Johan Hedberg]

Hi Anderson,

On Mon, Dec 06, 2010, Anderson Lizardo wrote:
> On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> > +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> > +{
> 
> I see some inconsistence on how you calculate struct sizes on this
> function. See below...
> 
> > +       struct sk_buff *skb;
> > +       struct mgmt_hdr *hdr;
> > +       struct mgmt_ev_cmd_status *ev;
> > +
> > +       BT_DBG("sock %p", sk);
> > +
> > +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
> 
> Here you use sizeof(<var>)

Yep, in Chapter 14 of Documentation/CodingStyle this seems to be the
preferred form.

> > +       if (!skb)
> > +               return;
> > +
> > +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
> 
> But here you use sizeof(<struct>). Could be sizeof(*hdr)?

Yes, could be.

> > +
> > +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> > +       hdr->len = cpu_to_le16(3);
> 
> and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)?

Yes, could be.

> > +       if (len != msglen - sizeof(struct mgmt_hdr)) {
> 
> You could use sizeof(*hdr) here.

Indeed.

I suppose these style fixes should be as a separate patch since the
original one already got acks from the relevant people? (if not, someone
please enlighten me how the kernel patch process deals with comments
received after acks :)

> > +               err = -EINVAL;
> > +               goto done;
> > +       }
> > +
> > +       switch (opcode) {
> > +       default:
> > +               BT_DBG("Unknown op %u", opcode);
> > +               cmd_status(sk, opcode, 0x01);
> > +               break;
> > +       }
> > +
> > +       err = msglen;
> 
> Would there be a chance of integer overflow here? The function returns
> (signed) int, but msglen is (unsigned) size_t.

The core of the issue is in the way that the sendmsg callback for
sockets is defined. At least L2CAP and RFCOMM sockets do similar
assignments in their sendmsg callbacks so I've assumed it to be ok.

Johan

Re: [PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[Gustavo F. Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2010-12-06 16:21:16 +0200]:

> Hi Anderson,
> 
> On Mon, Dec 06, 2010, Anderson Lizardo wrote:
> > On Sun, Dec 5, 2010 at 2:19 PM,  <johan.hedberg@gmail.com> wrote:
> > > +static void cmd_status(struct sock *sk, u16 cmd, u8 status)
> > > +{
> > 
> > I see some inconsistence on how you calculate struct sizes on this
> > function. See below...
> > 
> > > +       struct sk_buff *skb;
> > > +       struct mgmt_hdr *hdr;
> > > +       struct mgmt_ev_cmd_status *ev;
> > > +
> > > +       BT_DBG("sock %p", sk);
> > > +
> > > +       skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
> > 
> > Here you use sizeof(<var>)
> 
> Yep, in Chapter 14 of Documentation/CodingStyle this seems to be the
> preferred form.
> 
> > > +       if (!skb)
> > > +               return;
> > > +
> > > +       hdr = (void *) skb_put(skb, sizeof(struct mgmt_hdr));
> > 
> > But here you use sizeof(<struct>). Could be sizeof(*hdr)?
> 
> Yes, could be.
> 
> > > +
> > > +       hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
> > > +       hdr->len = cpu_to_le16(3);
> > 
> > and here a hard-coded size. Could be sizeof(struct mgmt_ev_cmd_status)?
> 
> Yes, could be.
> 
> > > +       if (len != msglen - sizeof(struct mgmt_hdr)) {
> > 
> > You could use sizeof(*hdr) here.
> 
> Indeed.
> 
> I suppose these style fixes should be as a separate patch since the
> original one already got acks from the relevant people? (if not, someone
> please enlighten me how the kernel patch process deals with comments
> received after acks :)

>From what I understand the Acked-by is more about the whole idea of the patch,
so to me it is fine to do such styles fixes and keep the ack in the patch. You
won't change essentials parts of the patch.

-- 
Gustavo F. Padovan
http://profusion.mobi

[PATCH 2/3] Bluetooth: Add initial Bluetooth Management interface callbacks

[johan.hedberg]

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

Add initial code for handling Bluetooth Management interface messages.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 include/net/bluetooth/hci_core.h |    3 +
 net/bluetooth/Makefile           |    2 +-
 net/bluetooth/hci_sock.c         |   39 +++++++++++++--
 net/bluetooth/mgmt.c             |   99 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 7 deletions(-)
 create mode 100644 net/bluetooth/mgmt.c

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3e34359..1992fac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -660,6 +660,9 @@ void hci_si_event(struct hci_dev *hdev, int type, int dlen, void *data);
 /* ----- HCI Sockets ----- */
 void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb);
 
+/* Management interface */
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
+
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
 
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index 7ca1f46..250f954 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -10,4 +10,4 @@ obj-$(CONFIG_BT_BNEP)	+= bnep/
 obj-$(CONFIG_BT_CMTP)	+= cmtp/
 obj-$(CONFIG_BT_HIDP)	+= hidp/
 
-bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o hci_sock.o hci_sysfs.o lib.o
+bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o hci_sock.o hci_sysfs.o lib.o
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b3753ba..207be7a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -49,6 +49,8 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+static int enable_mgmt;
+
 /* ----- HCI socket interface ----- */
 
 static inline int hci_test_bit(int nr, void *addr)
@@ -353,25 +355,35 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long a
 
 static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 {
-	struct sockaddr_hci *haddr = (struct sockaddr_hci *) addr;
+	struct sockaddr_hci haddr;
 	struct sock *sk = sock->sk;
 	struct hci_dev *hdev = NULL;
-	int err = 0;
+	int len, err = 0;
 
 	BT_DBG("sock %p sk %p", sock, sk);
 
-	if (!haddr || haddr->hci_family != AF_BLUETOOTH)
+	if (!addr)
+		return -EINVAL;
+
+	memset(&haddr, 0, sizeof(haddr));
+	len = min_t(unsigned int, sizeof(haddr), addr_len);
+	memcpy(&haddr, addr, len);
+
+	if (haddr.hci_family != AF_BLUETOOTH)
+		return -EINVAL;
+
+	if (haddr.hci_channel != HCI_CHANNEL_RAW && !enable_mgmt)
 		return -EINVAL;
 
 	lock_sock(sk);
 
-	if (hci_pi(sk)->hdev) {
+	if (sk->sk_state == BT_BOUND || hci_pi(sk)->hdev) {
 		err = -EALREADY;
 		goto done;
 	}
 
-	if (haddr->hci_dev != HCI_DEV_NONE) {
-		hdev = hci_dev_get(haddr->hci_dev);
+	if (haddr.hci_dev != HCI_DEV_NONE) {
+		hdev = hci_dev_get(haddr.hci_dev);
 		if (!hdev) {
 			err = -ENODEV;
 			goto done;
@@ -380,6 +392,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		atomic_inc(&hdev->promisc);
 	}
 
+	hci_pi(sk)->channel = haddr.hci_channel;
 	hci_pi(sk)->hdev = hdev;
 	sk->sk_state = BT_BOUND;
 
@@ -502,6 +515,17 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 	lock_sock(sk);
 
+	switch (hci_pi(sk)->channel) {
+	case HCI_CHANNEL_RAW:
+		break;
+	case HCI_CHANNEL_CONTROL:
+		err = mgmt_control(sk, msg, len);
+		goto done;
+	default:
+		err = -EINVAL;
+		goto done;
+	}
+
 	hdev = hci_pi(sk)->hdev;
 	if (!hdev) {
 		err = -EBADFD;
@@ -831,3 +855,6 @@ void __exit hci_sock_cleanup(void)
 
 	proto_unregister(&hci_sk_proto);
 }
+
+module_param(enable_mgmt, bool, 0644);
+MODULE_PARM_DESC(enable_mgmt, "Enable Management interface");
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
new file mode 100644
index 0000000..d15bf67
--- /dev/null
+++ b/net/bluetooth/mgmt.c
@@ -0,0 +1,99 @@
+/*
+   BlueZ - Bluetooth protocol stack for Linux
+   Copyright (C) 2010  Nokia Corporation
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 as
+   published by the Free Software Foundation;
+
+   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+   OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+   IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
+   SOFTWARE IS DISCLAIMED.
+*/
+
+/* Bluetooth HCI Management interface */
+
+#include <asm/uaccess.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/mgmt.h>
+
+static void cmd_status(struct sock *sk, u16 cmd, u8 status)
+{
+	struct sk_buff *skb;
+	struct mgmt_hdr *hdr;
+	struct mgmt_ev_cmd_status *ev;
+
+	BT_DBG("sock %p", sk);
+
+	skb = alloc_skb(sizeof(*hdr) + sizeof(*ev), GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	hdr = (void *) skb_put(skb, sizeof(*hdr));
+
+	hdr->opcode = cpu_to_le16(MGMT_EV_CMD_STATUS);
+	hdr->len = cpu_to_le16(sizeof(*ev));
+
+	ev = (void *) skb_put(skb, sizeof(*ev));
+	ev->status = status;
+	put_unaligned_le16(cmd, &ev->opcode);
+
+	if (sock_queue_rcv_skb(sk, skb) < 0)
+		kfree_skb(skb);
+}
+
+int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
+{
+	unsigned char *buf;
+	struct mgmt_hdr *hdr;
+	u16 opcode, len;
+	int err;
+
+	BT_DBG("got %zu bytes", msglen);
+
+	if (msglen < sizeof(*hdr))
+		return -EINVAL;
+
+	buf = kmalloc(msglen, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	if (memcpy_fromiovec(buf, msg->msg_iov, msglen)) {
+		err = -EFAULT;
+		goto done;
+	}
+
+	hdr = (struct mgmt_hdr *) buf;
+	opcode = get_unaligned_le16(&hdr->opcode);
+	len = get_unaligned_le16(&hdr->len);
+
+	if (len != msglen - sizeof(*hdr)) {
+		err = -EINVAL;
+		goto done;
+	}
+
+	switch (opcode) {
+	default:
+		BT_DBG("Unknown op %u", opcode);
+		cmd_status(sk, opcode, 0x01);
+		break;
+	}
+
+	err = msglen;
+
+done:
+	kfree(buf);
+	return err;
+}
-- 
1.7.2.3