[RFC] Bluetooth: hidp: Check for valid ACL connection

Check for valid ACL connection before creating an hid device.
__hidp_link_session() needs session->conn but hidp_add_connection does not check
whether session->conn is set.

Signed-off-by: David Herrmann <redacted>
---

I recently got several kernel panics in hidp_add_connection when creating a
bluetooth HID device. I got these panics when the device shut down *while*
trying to initiate the HID connection.
This fix aborts the HID device creation if the ACL connection is disconnected.
This fixes the problem for me but I don't know whether there is still a
race-condition between obtainig session->conn and calling hci_conn_hold_device
in __hidp_link_session().

Also I think it would make the code nicer when calling hidp_get_device() in
hidp_add_connection and hidp_setup_input/hid just retrieves the parent-device
from session->conn.

Anyway, you can find my kernel oops message here:
https://gist.github.com/1156759
As far as I got, it is caused by:
hidp_add_connection()
 -> __hidp_link_session()
  -> hci_conn_hold_device()
   -> atomic_inc()
    -> test_and_set_bit()

Regards
David

 net/bluetooth/hidp/core.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index fb68f34..baec330 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c

@@ -837,6 +837,11 @@ static int hidp_setup_input(struct hidp_session *session,
 	}
 
 	input->dev.parent = hidp_get_device(session);
+	if (!input->dev.parent) {
+		input_free_device(input);
+		session->input = NULL;
+		return -ENODEV;
+	}
 
 	input->event = hidp_input_event;
 

@@ -941,6 +946,9 @@ static int hidp_setup_hid(struct hidp_session *session,
 	strncpy(hid->uniq, batostr(&bt_sk(session->ctrl_sock->sk)->dst), 64);
 
 	hid->dev.parent = hidp_get_device(session);
+	if (!hid->dev.parent)
+		goto dev_err;
+
 	hid->ll_driver = &hidp_hid_driver;
 
 	hid->hid_get_raw_report = hidp_get_raw_report;

@@ -948,6 +956,9 @@ static int hidp_setup_hid(struct hidp_session *session,
 
 	return 0;
 
+dev_err:
+	hid_destroy_device(hid);
+	session->hid = NULL;
 fault:
 	kfree(session->rd_data);
 	session->rd_data = NULL;

-- 
1.7.6

Hi David,

On Fri, 2011-08-19 at 10:41 -0400, David Herrmann wrote:

Also I think it would make the code nicer when calling hidp_get_device() =

in

hidp_add_connection and hidp_setup_input/hid just retrieves the parent-de=

vice

from session->conn.

What about something like this instead?

This:
1. moves the hidp_get_device out into hidp_add_connection
2. renames hidp_get_device to hidp_find_connection
3. checks for null connection value return
4. performs the connection list access with device lock held
5. bumps the hci connection device hold with device lock held

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index e02c6a2..f6db935 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c

@@ -97,8 +97,6 @@ static void __hidp_link_session(struct hidp_session *sess=

ion)
 {
 	__module_get(THIS_MODULE);
 	list_add(&session->list, &hidp_session_list);
-
-	hci_conn_hold_device(session->conn);
 }
=20
 static void __hidp_unlink_session(struct hidp_session *session)

@@ -763,24 +761,26 @@ static int hidp_session(void *arg)
 	return 0;
 }

=20
-static struct device *hidp_get_device(struct hidp_session *session)
+static struct hci_conn *hidp_find_connection(struct hidp_session *session)
 {
 	bdaddr_t *src =3D &bt_sk(session->ctrl_sock->sk)->src;
 	bdaddr_t *dst =3D &bt_sk(session->ctrl_sock->sk)->dst;
-	struct device *device =3D NULL;
+	struct hci_conn *conn;
 	struct hci_dev *hdev;
=20
 	hdev =3D hci_get_route(dst, src);
 	if (!hdev)
 		return NULL;
=20
-	session->conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
-	if (session->conn)
-		device =3D &session->conn->dev;
+	hci_dev_lock_bh(hdev);
+	conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
+	if (conn)
+		hci_conn_hold_device(conn);
+	hci_dev_unlock_bh(hdev);
=20
 	hci_dev_put(hdev);
=20
-	return device;
+	return conn;
 }
=20
 static int hidp_setup_input(struct hidp_session *session,

@@ -830,7 +830,7 @@ static int hidp_setup_input(struct hidp_session *sessio=

n,
 		input->relbit[0] |=3D BIT_MASK(REL_WHEEL);
 	}
=20
-	input->dev.parent =3D hidp_get_device(session);
+	input->dev.parent =3D &session->conn->dev;
=20
 	input->event =3D hidp_input_event;
=20

@@ -934,7 +934,7 @@ static int hidp_setup_hid(struct hidp_session *session,
 	strncpy(hid->phys, batostr(&bt_sk(session->ctrl_sock->sk)->src), 64);
 	strncpy(hid->uniq, batostr(&bt_sk(session->ctrl_sock->sk)->dst), 64);

=20
-	hid->dev.parent =3D hidp_get_device(session);
+	hid->dev.parent =3D &session->conn->dev;
 	hid->ll_driver =3D &hidp_hid_driver;
=20
 	hid->hid_get_raw_report =3D hidp_get_raw_report;

@@ -975,6 +975,10 @@ int hidp_add_connection(struct hidp_connadd_req *req, =

struct socket *ctrl_sock,
 		goto failed;
 	}
=20
+	session->conn =3D hidp_find_connection(session);
+	if (!session->conn)
+		goto failed;
+
 	bacpy(&session->bdaddr, &bt_sk(ctrl_sock->sk)->dst);
=20
 	session->ctrl_mtu =3D min_t(uint, l2cap_pi(ctrl_sock->sk)->chan->omtu,

@@ -1000,6 +1004,8 @@ int hidp_add_connection(struct hidp_connadd_req *req,=
 struct socket *ctrl_sock,
 	session->flags   =3D req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
 	session->idle_to =3D req->idle_to;

=20
+	__hidp_link_session(session);
+
 	if (req->rd_size > 0) {
 		err =3D hidp_setup_hid(session, req);
 		if (err && err !=3D -ENODEV)

@@ -1012,8 +1018,6 @@ int hidp_add_connection(struct hidp_connadd_req *req,=
 struct socket *ctrl_sock,
 			goto purge;
 	}

=20
-	__hidp_link_session(session);
-
 	hidp_set_timer(session);
=20
 	if (session->hid) {

@@ -1062,8 +1066,6 @@ int hidp_add_connection(struct hidp_connadd_req *req,=
 struct socket *ctrl_sock,
 unlink:
 	hidp_del_timer(session);

=20
-	__hidp_unlink_session(session);
-
 	if (session->input) {
 		input_unregister_device(session->input);
 		session->input =3D NULL;

@@ -1076,6 +1078,8 @@ unlink:
 	session->rd_data =3D NULL;

=20
 purge:
+	__hidp_unlink_session(session);
+
 	skb_queue_purge(&session->ctrl_transmit);
 	skb_queue_purge(&session->intr_transmit);
=20
--=20

Regards,
Peter Hurley

Hi Peter

On Tue, Aug 30, 2011 at 3:40 PM, Peter Hurley [off-list ref] wr=
ote:

Hi David,

On Fri, 2011-08-19 at 10:41 -0400, David Herrmann wrote:

quoted

Also I think it would make the code nicer when calling hidp_get_device()=

 in

quoted

hidp_add_connection and hidp_setup_input/hid just retrieves the parent-d=

evice

ssion)

n)

=A0{
=A0 =A0 =A0 =A0bdaddr_t *src =3D &bt_sk(session->ctrl_sock->sk)->src;
=A0 =A0 =A0 =A0bdaddr_t *dst =3D &bt_sk(session->ctrl_sock->sk)->dst;
- =A0 =A0 =A0 struct device *device =3D NULL;
+ =A0 =A0 =A0 struct hci_conn *conn;
=A0 =A0 =A0 =A0struct hci_dev *hdev;

=A0 =A0 =A0 =A0hdev =3D hci_get_route(dst, src);
=A0 =A0 =A0 =A0if (!hdev)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return NULL;

- =A0 =A0 =A0 session->conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK, d=

st);

ion,

n,

, struct socket *ctrl_sock,

q, struct socket *ctrl_sock,

=A0 =A0 =A0 =A0session->flags =A0 =3D req->flags & (1 << HIDP_BLUETOOTH_V=

ENDOR_ID);

q, struct socket *ctrl_sock,

q, struct socket *ctrl_sock,

Great! This fixes the race-condition I mentioned earlier,
error-recovery also seems fine here. Thanks!
After Marcel or Gustavo commented on that, can you resend it with a
proper commit-message? Or should I resend it?
I am fine with you doing that, here's my signed-off line anyway:

Signed-off-by: David Herrmann <redacted>

Regards,
Peter Hurley

Regards
David