[PATCH 1/3] Bluetooth: Fix missing hci_dev_lock_bh in user_confirm_reply

[PATCH 1/3] Bluetooth: Fix missing hci_dev_lock_bh in user_confirm_reply

[johan.hedberg]

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

The code was correctly calling _unlock at the end of the function but
there was no actual _lock call anywhere.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0054c74..4476d8e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1230,6 +1230,8 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
 	if (!hdev)
 		return cmd_status(sk, index, mgmt_op, ENODEV);
 
+	hci_dev_lock_bh(hdev);
+
 	if (!test_bit(HCI_UP, &hdev->flags)) {
 		err = cmd_status(sk, index, mgmt_op, ENETDOWN);
 		goto failed;
-- 
1.7.4.1

[PATCH 2/3] Bluetooth: mgmt: Add local name information to read_info reply

[johan.hedberg]

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

This patch adds the name of the adapter to the reply of the read_info
management command.

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

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 5fabfa8..4deeea1 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -55,6 +55,7 @@ struct mgmt_rp_read_info {
 	__u16 manufacturer;
 	__u8 hci_ver;
 	__u16 hci_rev;
+	__u8 name[249];
 } __packed;
 
 struct mgmt_mode {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4476d8e..67529c8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -183,6 +183,8 @@ static int read_controller_info(struct sock *sk, u16 index)
 
 	set_bit(HCI_MGMT, &hdev->flags);
 
+	memset(&rp, 0, sizeof(rp));
+
 	rp.type = hdev->dev_type;
 
 	rp.powered = test_bit(HCI_UP, &hdev->flags);
@@ -204,6 +206,8 @@ static int read_controller_info(struct sock *sk, u16 index)
 	rp.hci_ver = hdev->hci_ver;
 	put_unaligned_le16(hdev->hci_rev, &rp.hci_rev);
 
+	memcpy(rp.name, hdev->dev_name, sizeof(hdev->dev_name));
+
 	hci_dev_unlock_bh(hdev);
 	hci_dev_put(hdev);
 
-- 
1.7.4.1

[PATCH 3/3] Bluetooth: mgmt: Add support for setting the local name

[johan.hedberg]

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

This patch adds a new set_local_name management command as well as a
local_name_changed management event. With these user space can both
change the local name as well as monitor changes to it by others.

The management messages reserve 249 bytes for the name instead of 248
(like in the HCI spec) so that there is always a guarantee that it is
nul-terminated. That way it can safely be passed onto string
manipulation functions.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |   10 +++++
 net/bluetooth/hci_event.c        |    9 +++-
 net/bluetooth/mgmt.c             |   76 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 441dadb..c77c082 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -767,6 +767,7 @@ int mgmt_user_confirm_reply_complete(u16 index, bdaddr_t *bdaddr, u8 status);
 int mgmt_user_confirm_neg_reply_complete(u16 index, bdaddr_t *bdaddr,
 								u8 status);
 int mgmt_auth_failed(u16 index, bdaddr_t *bdaddr, u8 status);
+int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 4deeea1..f7d4df3 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -168,6 +168,11 @@ struct mgmt_rp_user_confirm_reply {
 
 #define MGMT_OP_USER_CONFIRM_NEG_REPLY	0x0016
 
+#define MGMT_OP_SET_LOCAL_NAME		0x0017
+struct mgmt_cp_set_local_name {
+	__u8 name[249];
+} __packed;
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16 opcode;
@@ -235,3 +240,8 @@ struct mgmt_ev_auth_failed {
 	bdaddr_t bdaddr;
 	__u8 status;
 } __packed;
+
+#define MGMT_EV_LOCAL_NAME_CHANGED	0x0011
+struct mgmt_ev_local_name_changed {
+	__u8 name[249];
+} __packed;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3fbfa50..e7f7cb0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -193,13 +193,16 @@ static void hci_cc_write_local_name(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (status)
-		return;
-
 	sent = hci_sent_cmd_data(hdev, HCI_OP_WRITE_LOCAL_NAME);
 	if (!sent)
 		return;
 
+	if (test_bit(HCI_MGMT, &hdev->flags))
+		mgmt_set_local_name_complete(hdev->id, sent, status);
+
+	if (status)
+		return;
+
 	memcpy(hdev->dev_name, sent, 248);
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 67529c8..6e84ab2 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1258,6 +1258,46 @@ failed:
 	return err;
 }
 
+static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
+								u16 len)
+{
+	struct mgmt_cp_set_local_name *mgmt_cp = (void *) data;
+	struct hci_cp_write_local_name hci_cp;
+	struct hci_dev *hdev;
+	struct pending_cmd *cmd;
+	int err;
+
+	BT_DBG("");
+
+	if (len != sizeof(*mgmt_cp))
+		return cmd_status(sk, index, MGMT_OP_SET_LOCAL_NAME, EINVAL);
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return cmd_status(sk, index, MGMT_OP_SET_LOCAL_NAME, ENODEV);
+
+
+	hci_dev_lock_bh(hdev);
+
+	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LOCAL_NAME, index, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto failed;
+	}
+
+	memcpy(hci_cp.name, mgmt_cp->name, sizeof(hci_cp.name));
+	err = hci_send_cmd(hdev, HCI_OP_WRITE_LOCAL_NAME, sizeof(hci_cp),
+								&hci_cp);
+	if (err < 0)
+		mgmt_pending_remove(cmd);
+
+failed:
+	hci_dev_unlock_bh(hdev);
+	hci_dev_put(hdev);
+
+	return err;
+}
+
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 {
 	unsigned char *buf;
@@ -1353,6 +1393,9 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_USER_CONFIRM_NEG_REPLY:
 		err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
 		break;
+	case MGMT_OP_SET_LOCAL_NAME:
+		err = set_local_name(sk, index, buf + sizeof(*hdr), len);
+		break;
 	default:
 		BT_DBG("Unknown op %u", opcode);
 		err = cmd_status(sk, index, opcode, 0x01);
@@ -1649,3 +1692,36 @@ int mgmt_auth_failed(u16 index, bdaddr_t *bdaddr, u8 status)
 
 	return mgmt_event(MGMT_EV_AUTH_FAILED, index, &ev, sizeof(ev), NULL);
 }
+
+int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status)
+{
+	struct pending_cmd *cmd;
+	struct mgmt_cp_set_local_name ev;
+	int err;
+
+	memset(&ev, 0, sizeof(ev));
+	memcpy(ev.name, name, 248);
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_LOCAL_NAME, index);
+	if (!cmd)
+		goto send_event;
+
+	if (status) {
+		err = cmd_status(cmd->sk, index, MGMT_OP_SET_LOCAL_NAME, EIO);
+		goto failed;
+	}
+
+	err = cmd_complete(cmd->sk, index, MGMT_OP_SET_LOCAL_NAME, &ev,
+								sizeof(ev));
+	if (err < 0)
+		goto failed;
+
+send_event:
+	err = mgmt_event(MGMT_EV_LOCAL_NAME_CHANGED, index, &ev, sizeof(ev),
+							cmd ? cmd->sk : NULL);
+
+failed:
+	if (cmd)
+		mgmt_pending_remove(cmd);
+	return -err;
+}
-- 
1.7.4.1

Re: [PATCH 3/3] Bluetooth: mgmt: Add support for setting the local name

[Anderson Lizardo]

Hi Johan,

On Tue, Mar 15, 2011 at 2:54 PM,  <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@nokia.com>
>
> This patch adds a new set_local_name management command as well as a
> local_name_changed management event. With these user space can both
> change the local name as well as monitor changes to it by others.
>
> The management messages reserve 249 bytes for the name instead of 248
> (like in the HCI spec) so that there is always a guarantee that it is
> nul-terminated. That way it can safely be passed onto string
> manipulation functions.

I wonder if it is "future-proof" to use 248 and 249 along the code
without using some #define for the the spec defined value. Someone may
look at the code later and think there is a bug, even though it is
clear from the commit message.

My two cents,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 3/3] Bluetooth: mgmt: Add support for setting the local name

[Szymon Janc]

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> This patch adds a new set_local_name management command as well as a
> local_name_changed management event. With these user space can both
> change the local name as well as monitor changes to it by others.
> 
> The management messages reserve 249 bytes for the name instead of 248
> (like in the HCI spec) so that there is always a guarantee that it is
> nul-terminated. That way it can safely be passed onto string
> manipulation functions.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  include/net/bluetooth/mgmt.h     |   10 +++++
>  net/bluetooth/hci_event.c        |    9 +++-
>  net/bluetooth/mgmt.c             |   76 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 441dadb..c77c082 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -767,6 +767,7 @@ int mgmt_user_confirm_reply_complete(u16 index, bdaddr_t *bdaddr, u8 status);
>  int mgmt_user_confirm_neg_reply_complete(u16 index, bdaddr_t *bdaddr,
>  								u8 status);
>  int mgmt_auth_failed(u16 index, bdaddr_t *bdaddr, u8 status);
> +int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status);
>  
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 4deeea1..f7d4df3 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -168,6 +168,11 @@ struct mgmt_rp_user_confirm_reply {
>  
>  #define MGMT_OP_USER_CONFIRM_NEG_REPLY	0x0016
>  
> +#define MGMT_OP_SET_LOCAL_NAME		0x0017
> +struct mgmt_cp_set_local_name {
> +	__u8 name[249];
> +} __packed;
> +
>  #define MGMT_EV_CMD_COMPLETE		0x0001
>  struct mgmt_ev_cmd_complete {
>  	__le16 opcode;
> @@ -235,3 +240,8 @@ struct mgmt_ev_auth_failed {
>  	bdaddr_t bdaddr;
>  	__u8 status;
>  } __packed;
> +
> +#define MGMT_EV_LOCAL_NAME_CHANGED	0x0011
> +struct mgmt_ev_local_name_changed {
> +	__u8 name[249];
> +} __packed;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 3fbfa50..e7f7cb0 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -193,13 +193,16 @@ static void hci_cc_write_local_name(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (status)
> -		return;
> -
>  	sent = hci_sent_cmd_data(hdev, HCI_OP_WRITE_LOCAL_NAME);
>  	if (!sent)
>  		return;
>  
> +	if (test_bit(HCI_MGMT, &hdev->flags))
> +		mgmt_set_local_name_complete(hdev->id, sent, status);
> +
> +	if (status)
> +		return;
> +
>  	memcpy(hdev->dev_name, sent, 248);
>  }
>  
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 67529c8..6e84ab2 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1258,6 +1258,46 @@ failed:
>  	return err;
>  }
>  
> +static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
> +								u16 len)

Can fit 1 more tab :)

> +{
> +	struct mgmt_cp_set_local_name *mgmt_cp = (void *) data;
> +	struct hci_cp_write_local_name hci_cp;
> +	struct hci_dev *hdev;
> +	struct pending_cmd *cmd;
> +	int err;
> +
> +	BT_DBG("");
> +
> +	if (len != sizeof(*mgmt_cp))
> +		return cmd_status(sk, index, MGMT_OP_SET_LOCAL_NAME, EINVAL);
> +
> +	hdev = hci_dev_get(index);
> +	if (!hdev)
> +		return cmd_status(sk, index, MGMT_OP_SET_LOCAL_NAME, ENODEV);
> +
> +

Not needed empty line.

> +	hci_dev_lock_bh(hdev);
> +
> +	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LOCAL_NAME, index, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	memcpy(hci_cp.name, mgmt_cp->name, sizeof(hci_cp.name));
> +	err = hci_send_cmd(hdev, HCI_OP_WRITE_LOCAL_NAME, sizeof(hci_cp),
> +								&hci_cp);
> +	if (err < 0)
> +		mgmt_pending_remove(cmd);
> +
> +failed:
> +	hci_dev_unlock_bh(hdev);
> +	hci_dev_put(hdev);
> +
> +	return err;
> +}
> +
>  int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>  {
>  	unsigned char *buf;
> @@ -1353,6 +1393,9 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>  	case MGMT_OP_USER_CONFIRM_NEG_REPLY:
>  		err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
>  		break;
> +	case MGMT_OP_SET_LOCAL_NAME:
> +		err = set_local_name(sk, index, buf + sizeof(*hdr), len);
> +		break;
>  	default:
>  		BT_DBG("Unknown op %u", opcode);
>  		err = cmd_status(sk, index, opcode, 0x01);
> @@ -1649,3 +1692,36 @@ int mgmt_auth_failed(u16 index, bdaddr_t *bdaddr, u8 status)
>  
>  	return mgmt_event(MGMT_EV_AUTH_FAILED, index, &ev, sizeof(ev), NULL);
>  }
> +
> +int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +	struct mgmt_cp_set_local_name ev;
> +	int err;
> +
> +	memset(&ev, 0, sizeof(ev));
> +	memcpy(ev.name, name, 248);
> +
> +	cmd = mgmt_pending_find(MGMT_OP_SET_LOCAL_NAME, index);
> +	if (!cmd)
> +		goto send_event;
> +
> +	if (status) {
> +		err = cmd_status(cmd->sk, index, MGMT_OP_SET_LOCAL_NAME, EIO);
> +		goto failed;
> +	}
> +
> +	err = cmd_complete(cmd->sk, index, MGMT_OP_SET_LOCAL_NAME, &ev,
> +								sizeof(ev));
> +	if (err < 0)
> +		goto failed;
> +
> +send_event:
> +	err = mgmt_event(MGMT_EV_LOCAL_NAME_CHANGED, index, &ev, sizeof(ev),
> +							cmd ? cmd->sk : NULL);
> +
> +failed:
> +	if (cmd)
> +		mgmt_pending_remove(cmd);
> +	return -err;

err is already < 0 here.

> +}
> 

And I agree with Anderson Lizardo about 248-249 case.
We should have define for spec name length and use 
__u8 name[HCIDEV_NAME_LEN + 1] and comment (at least in docs) why
and that it is guaranteed to be null terminated.

Would also be good to get rid of magic numbers from code (but this is
more generic issue:)

-- 
Szymon Janc