[PATCH 00/16] Rewrite HIDP Session Management

[PATCH 00/16] Rewrite HIDP Session Management

[David Herrmann]

Hi

If anyone tested "hotplugging" with HIDP in the last years, there's a 50% chance
you get an oops when "unplugging" devices. I never got around fixing it until
now. I took the time to thoroughly review HIDP, hci_dev and hci_conn
interaction and it turned out to be totally broken. The thing is that HIDP
highly depends on the underlying hci_conn object. However, there is no way to
notify HIDP when the connection is shut down. So this is where I started fixing
things.

First, a list of bugs that all played together and are mostly fixed in this
series:

* hci_conn use after free:
  hci_conn_del() calls hci_conn_put_device() which might call
  hci_conn_del_sysfs(), which itself calls put_device() which might call
  bt_link_release() which calls kfree(hci_conn).
  That is, the "conn->handle" check in hci_conn_del() dereferences "conn" after
  it has been freed.
  Note that the "might"-case is the usual case. So this is a real bug.

* kfree() after device_initialize()
  During hci_conn_add() we call hci_conn_init_sysfs() which calls
  device_initialize() on hci_conn. However, if we never call
  hci_conn_add_sysfs(), then hci_conn_del() will not use "put_device()" to drop
  the reference but instead call kfree(hci_conn).
  This is not allowed (see documentation for device_initialize()).

* ref-count for device_del()
  We use a "devcnt" reference-count in every hci_conn structure which controls
  when we call device_del(hci_conn). This isn't a but itself, but discouraged
  behavior. device_del() is supposed to be called when the (virtual) hardware
  of the device goes away. So when the hci-connection is closed, we are supposed
  to call device_del() immediately. If the direct call to device_del()
  introduces bugs, we _must_ fix the bugs instead of delaying
  device_del(). The correct fix is to notify all childs that the
  device goes away (they can now call device_del(child)) and then call
  device_del() afterwards. But this needs to be done synchronously.

* device_find_child() + device_move()
  This is really wrong. When an hci_conn object goes away, we move all it's
  childs to the root node. The code doesn't have a comment to say _why_ we do
  this bug digging in commit-messages it said that client RFCOMM-TTY devices
  might have a reason to stay alive even though the hci_conn is gone.
  Could anyone please tell me _one_ reason, why an RFCOMM-TTY object should stay
  even though the hci_conn is gone?

* hci_conn use after free
  If an hci_conn is grabbed via hci_conn_hold_device(), but the connection
  hasn't been added to user-space, yet, then hci_conn_del() might call kfree()
  on the device, even though a later call to hci_conn_put_device() will still
  access it.
  This is a theoretical bug as we have other means to avoid it. But it's still
  ugly.

* hci_conn_put() called more often than hci_conn_hold()
  I added a debug-print to hci_conn_put() and the ref-count dropped below 0
  everytime I disconnected a device. It doesn't break anything, but we really
  should avoid this!

* hci_conn_put_device() doesn't take the parent hci_dev into account.
  So the bug we tried to fix with _hold/put_device(), comes up again if we
  unplug the adapter instead of disconnecting the remote device.

* HIDP doesn't consider "terminate" in wait-conditions
  No wait-condition in HIDP watches for "terminate" to become non-zero, even
  though it is a _very_ likely condition to terminate HIDP.

* HIDP removes the HID/input devices asynchronously from within the session
  instead of synchronously when the connection goes down. This causes the
  session-thread to use the connection even though it's gone.

That's all I can remember for now, but there's some more small bugs that I fixed
together with the session-rewrite.

This series includes some small fixes/cleanups that can be applied right away
(which would simplify this series a lot). The other patches depend on each other
so they cannot be cherry-picked.

I tested this series and it works perfectly well. 10 connect/disconnect
cycles worked without a bug. I can shut down the device, unplug the adapter or
do an explicit disconnect without an oops.
I haven't tested suspend/resume, yet, though.

Please review!
Thanks
David


Overview:

Patch 1-3: They remove a bogus return-code in bt_sock_unregister() which doesn't
           make sense to me.

Patch 4: Add is_l2cap_socket() which allows HIDP to verify that the passed
         sockets are real l2cap sockets.

Patch 5-7: Introduce ref-counting for hci_conn objects so we can have a
           reference to them in the HIDP session. Note that we cannot reuse the
           current "refcnt" as this is used for something totally differen: It
           counts the number of active users on the underlying connection. It
           doesn't ref-count the hci_conn object itself!

Patch 8-10: Some small HIDP and HCI-core fixes

Patch 11: Add l2cap_sock_get_hci_conn() helper to allow HIDP to use l2cap
          sockets and retrieve the underlying hci_conn object instead of using
          an unnecessary hash-table lookup.

Patch 12: Add hci_conn_user objects. This allows external protocols to bind to
          an hci_conn object and get notified whenever the connection is closed
          and unlinked from sysfs.

Patch 13: Small HIDP cleanup patch

Patch 14-15: Rewrite the HIDP session-management based on hci_conn_user objects.
             I haven't found an easy way to do this in many small patches as the
             new session-management is totally different than the previous one.
             Hence, I first add the new helpers and then remove the old helpers
             and do the switch in a follow-up patch. This makes reviewing
             easier.

Patch 16: Again a small fix for HIDP that uses the new session-management
          helpers.

David Herrmann (16):
  Bluetooth: discard bt_sock_unregister() errors
  Bluetooth: change bt_sock_unregister() to return void
  Bluetooth: hidp: simplify error path in sock-init
  Bluetooth: hidp: verify l2cap sockets
  Bluetooth: rename hci_conn_put to hci_conn_drop
  Bluetooth: remove unneeded hci_conn_hold/put_device()
  Bluetooth: introduce hci_conn ref-counting
  Bluetooth: hidp: remove unused session->state field
  Bluetooth: hidp: test "terminate" before sleeping
  Bluetooth: allow constant arguments for bacmp()/bacpy()
  Bluetooth: l2cap: add l2cap_sock_get_hci_conn() helper
  Bluetooth: add hci_conn_user sub-modules
  Bluetooth: hidp: move hidp_schedule() to core.c
  Bluetooth: hidp: add new session-management helpers
  Bluetooth: hidp: remove old session-management
  Bluetooth: hidp: handle kernel_sendmsg() errors correctly

 include/net/bluetooth/bluetooth.h |   6 +-
 include/net/bluetooth/hci_core.h  |  46 ++-
 include/net/bluetooth/l2cap.h     |   2 +
 net/bluetooth/af_bluetooth.c      |  15 +-
 net/bluetooth/bnep/sock.c         |   4 +-
 net/bluetooth/cmtp/sock.c         |   4 +-
 net/bluetooth/hci_conn.c          |  92 ++++-
 net/bluetooth/hci_event.c         |  40 +-
 net/bluetooth/hci_sock.c          |   4 +-
 net/bluetooth/hci_sysfs.c         |   1 -
 net/bluetooth/hidp/core.c         | 787 +++++++++++++++++++++++++-------------
 net/bluetooth/hidp/hidp.h         |  67 ++--
 net/bluetooth/hidp/sock.c         |  35 +-
 net/bluetooth/l2cap_core.c        |   6 +-
 net/bluetooth/l2cap_sock.c        |  31 +-
 net/bluetooth/mgmt.c              |   6 +-
 net/bluetooth/rfcomm/sock.c       |   3 +-
 net/bluetooth/sco.c               |   9 +-
 net/bluetooth/smp.c               |   2 +-
 19 files changed, 737 insertions(+), 423 deletions(-)

-- 
1.8.1.4

[PATCH 01/16] Bluetooth: discard bt_sock_unregister() errors

[David Herrmann]

After we successfully registered a socket via bt_sock_register() there is
no reason to ever check the return code of bt_sock_unregister(). If
bt_sock_unregister() fails, it means the socket _is_ already unregistered
so we have what we want, don't we?

Also, to get bt_sock_unregister() to fail, another part of the kernel has
to unregister _our_ socket. This is sooo _wrong_ that it will break way
earlier than when we unregister our socket.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/bnep/sock.c   | 4 +---
 net/bluetooth/cmtp/sock.c   | 4 +---
 net/bluetooth/hci_sock.c    | 4 +---
 net/bluetooth/hidp/sock.c   | 4 +---
 net/bluetooth/l2cap_sock.c  | 4 +---
 net/bluetooth/rfcomm/sock.c | 3 +--
 net/bluetooth/sco.c         | 3 +--
 7 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
index e7154a5..5b1c04e 100644
--- a/net/bluetooth/bnep/sock.c
+++ b/net/bluetooth/bnep/sock.c
@@ -253,8 +253,6 @@ error:
 void __exit bnep_sock_cleanup(void)
 {
 	bt_procfs_cleanup(&init_net, "bnep");
-	if (bt_sock_unregister(BTPROTO_BNEP) < 0)
-		BT_ERR("Can't unregister BNEP socket");
-
+	bt_sock_unregister(BTPROTO_BNEP);
 	proto_unregister(&bnep_proto);
 }
diff --git a/net/bluetooth/cmtp/sock.c b/net/bluetooth/cmtp/sock.c
index 1c57482..58d9ede 100644
--- a/net/bluetooth/cmtp/sock.c
+++ b/net/bluetooth/cmtp/sock.c
@@ -264,8 +264,6 @@ error:
 void cmtp_cleanup_sockets(void)
 {
 	bt_procfs_cleanup(&init_net, "cmtp");
-	if (bt_sock_unregister(BTPROTO_CMTP) < 0)
-		BT_ERR("Can't unregister CMTP socket");
-
+	bt_sock_unregister(BTPROTO_CMTP);
 	proto_unregister(&cmtp_proto);
 }
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 07f0739..20c92de 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1126,8 +1126,6 @@ error:
 void hci_sock_cleanup(void)
 {
 	bt_procfs_cleanup(&init_net, "hci");
-	if (bt_sock_unregister(BTPROTO_HCI) < 0)
-		BT_ERR("HCI socket unregistration failed");
-
+	bt_sock_unregister(BTPROTO_HCI);
 	proto_unregister(&hci_sk_proto);
 }
diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index 82a829d..5d0f1ca 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -304,8 +304,6 @@ error:
 void __exit hidp_cleanup_sockets(void)
 {
 	bt_procfs_cleanup(&init_net, "hidp");
-	if (bt_sock_unregister(BTPROTO_HIDP) < 0)
-		BT_ERR("Can't unregister HIDP socket");
-
+	bt_sock_unregister(BTPROTO_HIDP);
 	proto_unregister(&hidp_proto);
 }
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1bcfb84..7f97049 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1312,8 +1312,6 @@ error:
 void l2cap_cleanup_sockets(void)
 {
 	bt_procfs_cleanup(&init_net, "l2cap");
-	if (bt_sock_unregister(BTPROTO_L2CAP) < 0)
-		BT_ERR("L2CAP socket unregistration failed");
-
+	bt_sock_unregister(BTPROTO_L2CAP);
 	proto_unregister(&l2cap_proto);
 }
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index ce3f665..851ecc9 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -1068,8 +1068,7 @@ void __exit rfcomm_cleanup_sockets(void)
 
 	debugfs_remove(rfcomm_sock_debugfs);
 
-	if (bt_sock_unregister(BTPROTO_RFCOMM) < 0)
-		BT_ERR("RFCOMM socket layer unregistration failed");
+	bt_sock_unregister(BTPROTO_RFCOMM);
 
 	proto_unregister(&rfcomm_proto);
 }
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 57f250c..da479cd 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1113,8 +1113,7 @@ void __exit sco_exit(void)
 
 	debugfs_remove(sco_debugfs);
 
-	if (bt_sock_unregister(BTPROTO_SCO) < 0)
-		BT_ERR("SCO socket unregistration failed");
+	bt_sock_unregister(BTPROTO_SCO);
 
 	proto_unregister(&sco_proto);
 }
-- 
1.8.1.4

Re: [PATCH 01/16] Bluetooth: discard bt_sock_unregister() errors

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:51 +0100]:

> After we successfully registered a socket via bt_sock_register() there is
> no reason to ever check the return code of bt_sock_unregister(). If
> bt_sock_unregister() fails, it means the socket _is_ already unregistered
> so we have what we want, don't we?
> 
> Also, to get bt_sock_unregister() to fail, another part of the kernel has
> to unregister _our_ socket. This is sooo _wrong_ that it will break way
> earlier than when we unregister our socket.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  net/bluetooth/bnep/sock.c   | 4 +---
>  net/bluetooth/cmtp/sock.c   | 4 +---
>  net/bluetooth/hci_sock.c    | 4 +---
>  net/bluetooth/hidp/sock.c   | 4 +---
>  net/bluetooth/l2cap_sock.c  | 4 +---
>  net/bluetooth/rfcomm/sock.c | 3 +--
>  net/bluetooth/sco.c         | 3 +--
>  7 files changed, 7 insertions(+), 19 deletions(-)

Patch has been applied to bluetooth-next. Thanks.

	Gustavo

[PATCH 02/16] Bluetooth: change bt_sock_unregister() to return void

[David Herrmann]

There is no reason a caller ever wants to check the return type of this
call. _Iff_ a user successfully called bt_sock_register(), they're allowed
to call bt_sock_unregister().
All other calls in the kernel (device_del, device_unregister, kfree(), ..)
that are logically equivalent return void. Lets not make callers think
they have to check the return type of this call and instead simply return
void.

We guarantee that after bt_sock_unregister() is called, the socket type
_is_ unregistered. If that is not what the caller wants, they're using the
wrong function, anyway.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/bluetooth.h |  2 +-
 net/bluetooth/af_bluetooth.c      | 15 +++------------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 2554b3f..d3222d1 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -209,7 +209,7 @@ struct bt_sock_list {
 };
 
 int  bt_sock_register(int proto, const struct net_proto_family *ops);
-int  bt_sock_unregister(int proto);
+void bt_sock_unregister(int proto);
 void bt_sock_link(struct bt_sock_list *l, struct sock *s);
 void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
 int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 5355df6..c9383bd 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -92,23 +92,14 @@ int bt_sock_register(int proto, const struct net_proto_family *ops)
 }
 EXPORT_SYMBOL(bt_sock_register);
 
-int bt_sock_unregister(int proto)
+void bt_sock_unregister(int proto)
 {
-	int err = 0;
-
 	if (proto < 0 || proto >= BT_MAX_PROTO)
-		return -EINVAL;
+		return;
 
 	write_lock(&bt_proto_lock);
-
-	if (!bt_proto[proto])
-		err = -ENOENT;
-	else
-		bt_proto[proto] = NULL;
-
+	bt_proto[proto] = NULL;
 	write_unlock(&bt_proto_lock);
-
-	return err;
 }
 EXPORT_SYMBOL(bt_sock_unregister);
 
-- 
1.8.1.4

Re: [PATCH 02/16] Bluetooth: change bt_sock_unregister() to return void

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:52 +0100]:

> There is no reason a caller ever wants to check the return type of this
> call. _Iff_ a user successfully called bt_sock_register(), they're allowed
> to call bt_sock_unregister().
> All other calls in the kernel (device_del, device_unregister, kfree(), ..)
> that are logically equivalent return void. Lets not make callers think
> they have to check the return type of this call and instead simply return
> void.
> 
> We guarantee that after bt_sock_unregister() is called, the socket type
> _is_ unregistered. If that is not what the caller wants, they're using the
> wrong function, anyway.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  include/net/bluetooth/bluetooth.h |  2 +-
>  net/bluetooth/af_bluetooth.c      | 15 +++------------
>  2 files changed, 4 insertions(+), 13 deletions(-)

Patch has been applied to bluetooth-next. Thanks.

	Gustavo

[PATCH 03/16] Bluetooth: hidp: simplify error path in sock-init

[David Herrmann]

We currently print errors twice when doing a "goto error;". Avoid this and
also use labels for each error-condition. We shouldn't mix
"inline-cleanup" and "goto-cleanup" in a single function. It makes
extending the function later just more work.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/sock.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index 5d0f1ca..ca86436 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -281,22 +281,22 @@ int __init hidp_init_sockets(void)
 	err = bt_sock_register(BTPROTO_HIDP, &hidp_sock_family_ops);
 	if (err < 0) {
 		BT_ERR("Can't register HIDP socket");
-		goto error;
+		goto err_proto;
 	}
 
 	err = bt_procfs_init(THIS_MODULE, &init_net, "hidp", &hidp_sk_list, NULL);
 	if (err < 0) {
 		BT_ERR("Failed to create HIDP proc file");
-		bt_sock_unregister(BTPROTO_HIDP);
-		goto error;
+		goto err_sock;
 	}
 
 	BT_INFO("HIDP socket layer initialized");
 
 	return 0;
 
-error:
-	BT_ERR("Can't register HIDP socket");
+err_sock:
+	bt_sock_unregister(BTPROTO_HIDP);
+err_proto:
 	proto_unregister(&hidp_proto);
 	return err;
 }
-- 
1.8.1.4

Re: [PATCH 03/16] Bluetooth: hidp: simplify error path in sock-init

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:53 +0100]:

> We currently print errors twice when doing a "goto error;". Avoid this and
> also use labels for each error-condition. We shouldn't mix
> "inline-cleanup" and "goto-cleanup" in a single function. It makes
> extending the function later just more work.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  net/bluetooth/hidp/sock.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
> index 5d0f1ca..ca86436 100644
> --- a/net/bluetooth/hidp/sock.c
> +++ b/net/bluetooth/hidp/sock.c
> @@ -281,22 +281,22 @@ int __init hidp_init_sockets(void)
>  	err = bt_sock_register(BTPROTO_HIDP, &hidp_sock_family_ops);
>  	if (err < 0) {
>  		BT_ERR("Can't register HIDP socket");
> -		goto error;
> +		goto err_proto;
>  	}
>  
>  	err = bt_procfs_init(THIS_MODULE, &init_net, "hidp", &hidp_sk_list, NULL);
>  	if (err < 0) {
>  		BT_ERR("Failed to create HIDP proc file");
> -		bt_sock_unregister(BTPROTO_HIDP);
> -		goto error;
> +		goto err_sock;

Actually I'm ok with mixing inline and goto clean up here, It's simpler
than add another goto.

>  	}
>  
>  	BT_INFO("HIDP socket layer initialized");
>  
>  	return 0;
>  
> -error:
> -	BT_ERR("Can't register HIDP socket");

So just remove this error message and we should be fine. 

	Gustavo

Re: [PATCH 03/16] Bluetooth: hidp: simplify error path in sock-init

[David Herrmann]

Hi Gustavo

On Tue, Feb 26, 2013 at 8:56 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi David,
>
> * David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:53 +0100]:
>
>> We currently print errors twice when doing a "goto error;". Avoid this and
>> also use labels for each error-condition. We shouldn't mix
>> "inline-cleanup" and "goto-cleanup" in a single function. It makes
>> extending the function later just more work.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  net/bluetooth/hidp/sock.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
>> index 5d0f1ca..ca86436 100644
>> --- a/net/bluetooth/hidp/sock.c
>> +++ b/net/bluetooth/hidp/sock.c
>> @@ -281,22 +281,22 @@ int __init hidp_init_sockets(void)
>>       err = bt_sock_register(BTPROTO_HIDP, &hidp_sock_family_ops);
>>       if (err < 0) {
>>               BT_ERR("Can't register HIDP socket");
>> -             goto error;
>> +             goto err_proto;
>>       }
>>
>>       err = bt_procfs_init(THIS_MODULE, &init_net, "hidp", &hidp_sk_list, NULL);
>>       if (err < 0) {
>>               BT_ERR("Failed to create HIDP proc file");
>> -             bt_sock_unregister(BTPROTO_HIDP);
>> -             goto error;
>> +             goto err_sock;
>
> Actually I'm ok with mixing inline and goto clean up here, It's simpler
> than add another goto.

But it makes review harder. If there's one central place (trailing
goto's) where cleanup is done, it's much easier to review
cleanup-paths. Anyway, your decision. I will fix it up and resend.

Thanks!
David

[PATCH 04/16] Bluetooth: hidp: verify l2cap sockets

[David Herrmann]

We need to verify that the given sockets are actually l2cap sockets. If
they aren't, we are not supposed to access bt_sk(sock) and we shouldn't
start the session if the offsets turn out to be valid local BT addresses.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/l2cap.h | 1 +
 net/bluetooth/hidp/core.c     | 2 ++
 net/bluetooth/l2cap_sock.c    | 6 ++++++
 3 files changed, 9 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7588ef4..ae6210e 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -787,6 +787,7 @@ extern bool disable_ertm;
 
 int l2cap_init_sockets(void);
 void l2cap_cleanup_sockets(void);
+bool is_l2cap_socket(struct socket *s);
 
 void __l2cap_connect_rsp_defer(struct l2cap_chan *chan);
 int __l2cap_wait_ack(struct sock *sk);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a7352ff..61b50a6 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -969,6 +969,8 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 
 	BT_DBG("");
 
+	if (!is_l2cap_socket(ctrl_sock) || !is_l2cap_socket(intr_sock))
+		return -EINVAL;
 	if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
 			bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))
 		return -ENOTUNIQ;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 7f97049..598b167 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -43,6 +43,12 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent);
 static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
 				     int proto, gfp_t prio);
 
+bool is_l2cap_socket(struct socket *s)
+{
+	return s && s->ops == &l2cap_sock_ops;
+}
+EXPORT_SYMBOL(is_l2cap_socket);
+
 static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 {
 	struct sock *sk = sock->sk;
-- 
1.8.1.4

Re: [PATCH 04/16] Bluetooth: hidp: verify l2cap sockets

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:54 +0100]:

> We need to verify that the given sockets are actually l2cap sockets. If
> they aren't, we are not supposed to access bt_sk(sock) and we shouldn't
> start the session if the offsets turn out to be valid local BT addresses.

What is the issue you are trying to fix here, I don't get it.

> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  include/net/bluetooth/l2cap.h | 1 +
>  net/bluetooth/hidp/core.c     | 2 ++
>  net/bluetooth/l2cap_sock.c    | 6 ++++++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 7588ef4..ae6210e 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -787,6 +787,7 @@ extern bool disable_ertm;
>  
>  int l2cap_init_sockets(void);
>  void l2cap_cleanup_sockets(void);
> +bool is_l2cap_socket(struct socket *s);

Make it l2cap_is_socket() or something like that. Use 'sk' instead of 's'.

	Gustavo

Re: [PATCH 04/16] Bluetooth: hidp: verify l2cap sockets

[David Herrmann]

Hi Gustavo

On Tue, Feb 26, 2013 at 9:01 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi David,
>
> * David Herrmann <dh.herrmann@gmail.com> [2013-02-24 19:36:54 +0100]:
>
>> We need to verify that the given sockets are actually l2cap sockets. If
>> they aren't, we are not supposed to access bt_sk(sock) and we shouldn't
>> start the session if the offsets turn out to be valid local BT addresses.
>
> What is the issue you are trying to fix here, I don't get it.

Look what happens if you pass TCP sockets instead of l2cap sockets (or
any other kind of socket). In hidp_add_connection() we do:

if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
    bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))

Who guarantees that ctrl_sock->sk is a bt_sock? We shouldn't access
bt_sk(ctrl_sock->sk)->src if it's not a bt_sock. Because there might
be sockets that have a "struct X_sock" that is smaller than "bt_sock"
and hence, we would access unknown kernel memory.

>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  include/net/bluetooth/l2cap.h | 1 +
>>  net/bluetooth/hidp/core.c     | 2 ++
>>  net/bluetooth/l2cap_sock.c    | 6 ++++++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 7588ef4..ae6210e 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -787,6 +787,7 @@ extern bool disable_ertm;
>>
>>  int l2cap_init_sockets(void);
>>  void l2cap_cleanup_sockets(void);
>> +bool is_l2cap_socket(struct socket *s);
>
> Make it l2cap_is_socket() or something like that. Use 'sk' instead of 's'.

I will fix that.

Thanks!
David

[PATCH 05/16] Bluetooth: rename hci_conn_put to hci_conn_drop

[David Herrmann]

We use _get() and _put() for device ref-counting in the kernel. However,
hci_conn_put() is _not_ used for ref-counting, hence, rename it to
hci_conn_drop() so we can later fix ref-counting and introduce
hci_conn_put().

hci_conn_hold() and hci_conn_put() are currently used to manage how long a
connection should be held alive. When the last user drops the connection,
we spawn a delayed work that performs the disconnect. Obviously, this has
nothing to do with ref-counting for the _object_ but rather for the
keep-alive of the connection.

But we really _need_ proper ref-counting for the _object_ to allow
connection-users like rfcomm-tty, HIDP or others.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/hci_core.h |  2 +-
 net/bluetooth/hci_conn.c         |  6 +++---
 net/bluetooth/hci_event.c        | 36 ++++++++++++++++++------------------
 net/bluetooth/l2cap_core.c       |  6 +++---
 net/bluetooth/mgmt.c             |  6 +++---
 net/bluetooth/sco.c              |  6 +++---
 net/bluetooth/smp.c              |  2 +-
 7 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 014a2ea..2fde836 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -605,7 +605,7 @@ static inline void hci_conn_hold(struct hci_conn *conn)
 	cancel_delayed_work(&conn->disc_work);
 }
 
-static inline void hci_conn_put(struct hci_conn *conn)
+static inline void hci_conn_drop(struct hci_conn *conn)
 {
 	BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4925a02..319e7a4 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -433,7 +433,7 @@ int hci_conn_del(struct hci_conn *conn)
 		struct hci_conn *acl = conn->link;
 		if (acl) {
 			acl->link = NULL;
-			hci_conn_put(acl);
+			hci_conn_drop(acl);
 		}
 	}
 
@@ -565,7 +565,7 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
 	if (!sco) {
 		sco = hci_conn_add(hdev, type, dst);
 		if (!sco) {
-			hci_conn_put(acl);
+			hci_conn_drop(acl);
 			return ERR_PTR(-ENOMEM);
 		}
 	}
@@ -980,7 +980,7 @@ void hci_chan_del(struct hci_chan *chan)
 
 	synchronize_rcu();
 
-	hci_conn_put(conn);
+	hci_conn_drop(conn);
 
 	skb_queue_purge(&chan->data_q);
 	kfree(chan);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 81b4448..11df1c4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1475,7 +1475,7 @@ static void hci_cs_auth_requested(struct hci_dev *hdev, __u8 status)
 	if (conn) {
 		if (conn->state == BT_CONFIG) {
 			hci_proto_connect_cfm(conn, status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	}
 
@@ -1502,7 +1502,7 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
 	if (conn) {
 		if (conn->state == BT_CONFIG) {
 			hci_proto_connect_cfm(conn, status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	}
 
@@ -1664,7 +1664,7 @@ static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
 	if (conn) {
 		if (conn->state == BT_CONFIG) {
 			hci_proto_connect_cfm(conn, status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	}
 
@@ -1691,7 +1691,7 @@ static void hci_cs_read_remote_ext_features(struct hci_dev *hdev, __u8 status)
 	if (conn) {
 		if (conn->state == BT_CONFIG) {
 			hci_proto_connect_cfm(conn, status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	}
 
@@ -2154,7 +2154,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		} else {
 			conn->state = BT_CONNECT2;
 			hci_proto_connect_cfm(conn, 0);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	} else {
 		/* Connection rejected */
@@ -2261,14 +2261,14 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		} else {
 			conn->state = BT_CONNECTED;
 			hci_proto_connect_cfm(conn, ev->status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		}
 	} else {
 		hci_auth_cfm(conn, ev->status);
 
 		hci_conn_hold(conn);
 		conn->disc_timeout = HCI_DISCONN_TIMEOUT;
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 	if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) {
@@ -2352,7 +2352,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 		if (ev->status && conn->state == BT_CONNECTED) {
 			hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 			goto unlock;
 		}
 
@@ -2361,7 +2361,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 				conn->state = BT_CONNECTED;
 
 			hci_proto_connect_cfm(conn, ev->status);
-			hci_conn_put(conn);
+			hci_conn_drop(conn);
 		} else
 			hci_encrypt_cfm(conn, ev->status, ev->encrypt);
 	}
@@ -2436,7 +2436,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev,
 	if (!hci_outgoing_auth_needed(hdev, conn)) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 unlock:
@@ -2996,7 +2996,7 @@ static void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (conn->state == BT_CONNECTED) {
 		hci_conn_hold(conn);
 		conn->disc_timeout = HCI_PAIRING_TIMEOUT;
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 	if (!test_bit(HCI_PAIRABLE, &hdev->dev_flags))
@@ -3099,7 +3099,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		if (ev->key_type != HCI_LK_CHANGED_COMBINATION)
 			conn->key_type = ev->key_type;
 
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 	if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
@@ -3268,7 +3268,7 @@ static void hci_remote_ext_features_evt(struct hci_dev *hdev,
 	if (!hci_outgoing_auth_needed(hdev, conn)) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 unlock:
@@ -3413,7 +3413,7 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
 
 	if (ev->status && conn->state == BT_CONNECTED) {
 		hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 		goto unlock;
 	}
 
@@ -3422,13 +3422,13 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
 			conn->state = BT_CONNECTED;
 
 		hci_proto_connect_cfm(conn, ev->status);
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	} else {
 		hci_auth_cfm(conn, ev->status);
 
 		hci_conn_hold(conn);
 		conn->disc_timeout = HCI_DISCONN_TIMEOUT;
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 	}
 
 unlock:
@@ -3689,7 +3689,7 @@ static void hci_simple_pair_complete_evt(struct hci_dev *hdev,
 		mgmt_auth_failed(hdev, &conn->dst, conn->type, conn->dst_type,
 				 ev->status);
 
-	hci_conn_put(conn);
+	hci_conn_drop(conn);
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -3777,7 +3777,7 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
 
 	hci_conn_hold(hcon);
 	hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
-	hci_conn_put(hcon);
+	hci_conn_drop(hcon);
 
 	hci_conn_hold_device(hcon);
 	hci_conn_add_sysfs(hcon);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 22e6583..175dd32 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -571,7 +571,7 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		chan->conn = NULL;
 
 		if (chan->chan_type != L2CAP_CHAN_CONN_FIX_A2MP)
-			hci_conn_put(conn->hcon);
+			hci_conn_drop(conn->hcon);
 
 		if (mgr && mgr->bredr_chan == chan)
 			mgr->bredr_chan = NULL;
@@ -1702,7 +1702,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 	conn = l2cap_conn_add(hcon, 0);
 	if (!conn) {
-		hci_conn_put(hcon);
+		hci_conn_drop(hcon);
 		err = -ENOMEM;
 		goto done;
 	}
@@ -1712,7 +1712,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 		if (!list_empty(&conn->chan_l)) {
 			err = -EBUSY;
-			hci_conn_put(hcon);
+			hci_conn_drop(hcon);
 		}
 
 		if (err)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f559b96..cca9187 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1857,7 +1857,7 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
 	conn->security_cfm_cb = NULL;
 	conn->disconn_cfm_cb = NULL;
 
-	hci_conn_put(conn);
+	hci_conn_drop(conn);
 
 	mgmt_pending_remove(cmd);
 }
@@ -1943,7 +1943,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 	if (conn->connect_cfm_cb) {
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 		err = cmd_complete(sk, hdev->id, MGMT_OP_PAIR_DEVICE,
 				   MGMT_STATUS_BUSY, &rp, sizeof(rp));
 		goto unlock;
@@ -1952,7 +1952,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	cmd = mgmt_pending_add(sk, MGMT_OP_PAIR_DEVICE, hdev, data, len);
 	if (!cmd) {
 		err = -ENOMEM;
-		hci_conn_put(conn);
+		hci_conn_drop(conn);
 		goto unlock;
 	}
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index da479cd..cde5aa3 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -185,7 +185,7 @@ static int sco_connect(struct sock *sk)
 
 	conn = sco_conn_add(hcon);
 	if (!conn) {
-		hci_conn_put(hcon);
+		hci_conn_drop(hcon);
 		err = -ENOMEM;
 		goto done;
 	}
@@ -355,7 +355,7 @@ static void __sco_sock_close(struct sock *sk)
 		if (sco_pi(sk)->conn->hcon) {
 			sk->sk_state = BT_DISCONN;
 			sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
-			hci_conn_put(sco_pi(sk)->conn->hcon);
+			hci_conn_drop(sco_pi(sk)->conn->hcon);
 			sco_pi(sk)->conn->hcon = NULL;
 		} else
 			sco_chan_del(sk, ECONNRESET);
@@ -883,7 +883,7 @@ static void sco_chan_del(struct sock *sk, int err)
 		sco_conn_unlock(conn);
 
 		if (conn->hcon)
-			hci_conn_put(conn->hcon);
+			hci_conn_drop(conn->hcon);
 	}
 
 	sk->sk_state = BT_CLOSED;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 5abefb1..b2296d3 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -522,7 +522,7 @@ void smp_chan_destroy(struct l2cap_conn *conn)
 	kfree(smp);
 	conn->smp_chan = NULL;
 	conn->hcon->smp_conn = NULL;
-	hci_conn_put(conn->hcon);
+	hci_conn_drop(conn->hcon);
 }
 
 int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
-- 
1.8.1.4

[PATCH 06/16] Bluetooth: remove unneeded hci_conn_hold/put_device()

[David Herrmann]

hci_conn_hold/put_device() is used to control when hci_conn->dev is no
longer needed and can be deleted from the system. Lets first look how they
are currently used throughout the code (excluding HIDP!).

All code that uses hci_conn_hold_device() looks like this:
    ...
    hci_conn_hold_device();
    hci_conn_add_sysfs();
    ...
On the other side, hci_conn_put_device() is exclusively used in
hci_conn_del().

So, considering that hci_conn_del() must not be called twice (which would
fail horribly), we know that hci_conn_put_device() is only called _once_
(which is in hci_conn_del()).
On the other hand, hci_conn_add_sysfs() must not be called twice, either
(it would call device_add twice, which breaks the device, see
drivers/base/core.c). So we know that hci_conn_hold_device() is also
called only once (it's only called directly before hci_conn_add_sysfs()).

So hold and put are known to be called only once. That means we can safely
remove them and directly call hci_conn_del_sysfs() in hci_conn_del().

But there is one issue left: HIDP also uses hci_conn_hold/put_device().
However, this case can be ignored and simply removed as it is totally
broken. The issue is, the only thing HIDP delays with
hci_conn_hold_device() is the removal of the hci_conn->dev from sysfs.
But, the hci_conn device has no mechanism to get notified when its own
parent (hci_dev) gets removed from sysfs. hci_dev_hold/put() does _not_
control when it is removed but only when the device object is created
and destroyed.
And hci_dev calls hci_conn_flush_*() when it removes itself from sysfs,
which itself causes hci_conn_del() to be called, but it does _not_ cause
hci_conn_del_sysfs() to be called, which is wrong.

Hence, we fix it to call hci_conn_del_sysfs() in hci_conn_del(). This
guarantees that a hci_conn object is removed from sysfs _before_ its
parent hci_dev is removed.

The changes to HIDP look scary, wrong and broken. However, if you look at
the HIDP session management, you will notice they're already broken in the
exact _same_ way (ever tried "unplugging" HIDP devices? Breaks _all_ the
time).
So this patch only makes HIDP look _scary_ and _obviously broken_. It does
not break HIDP itself, it already is!

See later patches in this series which fix HIDP to use proper
session-management.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/hci_core.h |  4 ----
 net/bluetooth/hci_conn.c         | 17 +----------------
 net/bluetooth/hci_event.c        |  4 ----
 net/bluetooth/hidp/core.c        | 20 +++-----------------
 4 files changed, 4 insertions(+), 41 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2fde836..cd5b126 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -338,7 +338,6 @@ struct hci_conn {
 	struct timer_list auto_accept_timer;
 
 	struct device	dev;
-	atomic_t	devref;
 
 	struct hci_dev	*hdev;
 	void		*l2cap_data;
@@ -594,9 +593,6 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
 
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
-void hci_conn_hold_device(struct hci_conn *conn);
-void hci_conn_put_device(struct hci_conn *conn);
-
 static inline void hci_conn_hold(struct hci_conn *conn)
 {
 	BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 319e7a4..134ab9f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -398,8 +398,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 	if (hdev->notify)
 		hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
 
-	atomic_set(&conn->devref, 0);
-
 	hci_conn_init_sysfs(conn);
 
 	return conn;
@@ -448,7 +446,7 @@ int hci_conn_del(struct hci_conn *conn)
 
 	skb_queue_purge(&conn->data_q);
 
-	hci_conn_put_device(conn);
+	hci_conn_del_sysfs(conn);
 
 	hci_dev_put(hdev);
 
@@ -835,19 +833,6 @@ void hci_conn_check_pending(struct hci_dev *hdev)
 	hci_dev_unlock(hdev);
 }
 
-void hci_conn_hold_device(struct hci_conn *conn)
-{
-	atomic_inc(&conn->devref);
-}
-EXPORT_SYMBOL(hci_conn_hold_device);
-
-void hci_conn_put_device(struct hci_conn *conn)
-{
-	if (atomic_dec_and_test(&conn->devref))
-		hci_conn_del_sysfs(conn);
-}
-EXPORT_SYMBOL(hci_conn_put_device);
-
 int hci_get_conn_list(void __user *arg)
 {
 	struct hci_conn *c;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 11df1c4..d089f5e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2000,7 +2000,6 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		} else
 			conn->state = BT_CONNECTED;
 
-		hci_conn_hold_device(conn);
 		hci_conn_add_sysfs(conn);
 
 		if (test_bit(HCI_AUTH, &hdev->flags))
@@ -3302,7 +3301,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		conn->handle = __le16_to_cpu(ev->handle);
 		conn->state  = BT_CONNECTED;
 
-		hci_conn_hold_device(conn);
 		hci_conn_add_sysfs(conn);
 		break;
 
@@ -3779,7 +3777,6 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
 	hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
 	hci_conn_drop(hcon);
 
-	hci_conn_hold_device(hcon);
 	hci_conn_add_sysfs(hcon);
 
 	amp_physical_cfm(bredr_hcon, hcon);
@@ -3913,7 +3910,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	conn->handle = __le16_to_cpu(ev->handle);
 	conn->state = BT_CONNECTED;
 
-	hci_conn_hold_device(conn);
 	hci_conn_add_sysfs(conn);
 
 	hci_proto_connect_cfm(conn, ev->status);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 61b50a6..143feb2 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -73,18 +73,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
 	return NULL;
 }
 
-static void __hidp_link_session(struct hidp_session *session)
-{
-	list_add(&session->list, &hidp_session_list);
-}
-
-static void __hidp_unlink_session(struct hidp_session *session)
-{
-	hci_conn_put_device(session->conn);
-
-	list_del(&session->list);
-}
-
 static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
 {
 	memset(ci, 0, sizeof(*ci));
@@ -756,7 +744,7 @@ static int hidp_session(void *arg)
 
 	fput(session->ctrl_sock->file);
 
-	__hidp_unlink_session(session);
+	list_del(&session->list);
 
 	up_write(&hidp_session_sem);
 
@@ -779,8 +767,6 @@ static struct hci_conn *hidp_get_connection(struct hidp_session *session)
 
 	hci_dev_lock(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
-	if (conn)
-		hci_conn_hold_device(conn);
 	hci_dev_unlock(hdev);
 
 	hci_dev_put(hdev);
@@ -1022,7 +1008,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 	session->flags   = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
 	session->idle_to = req->idle_to;
 
-	__hidp_link_session(session);
+	list_add(&session->list, &hidp_session_list);
 
 	if (req->rd_size > 0) {
 		err = hidp_setup_hid(session, req);
@@ -1102,7 +1088,7 @@ unlink:
 	session->rd_data = NULL;
 
 purge:
-	__hidp_unlink_session(session);
+	list_del(&session->list);
 
 	skb_queue_purge(&session->ctrl_transmit);
 	skb_queue_purge(&session->intr_transmit);
-- 
1.8.1.4

[PATCH 07/16] Bluetooth: introduce hci_conn ref-counting

[David Herrmann]

We currently do not allow using hci_conn from outside of HCI-core.
However, several other users could make great use of it. This includes
HIDP, rfcomm and all other sub-protocols that rely on an active
connection.

Hence, we now introduce hci_conn ref-counting. We currently never call
get_device(). put_device() is exclusively used in hci_conn_del_sysfs().
Hence, we currently never have a greater device-refcnt than 1.
Therefore, it is safe to remove the put_device() call from
hci_conn_del_sysfs() to hci_conn_del() (it's the only caller). In fact,
this even fixes a "use-after-free" bug as we access hci_conn after calling
hci_conn_del_sysfs() in hci_conn_del().

>From now on we can add references to hci_conn objects in other layers
(like l2cap_sock, HIDP, rfcomm, ...) and grab a reference via
hci_conn_get(). This does _not_ guarantee, that the connection is still
alive. However, this isn't what we want. We can simply lock the hci_conn
device and use "device_is_registered(hci_conn->dev)" to test that.
However, this is hardly necessary as outside users should never rely on
the HCI connection to be alive, anyway. Instead, they should solely rely
on the device-object to be available.
But if sub-devices want the hci_conn object as sysfs parent, they need to
be notified when the connection drops. This will be introduced in later
patches with hci_conn-users.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/hci_core.h | 31 +++++++++++++++++++++++++++++++
 net/bluetooth/hci_conn.c         |  3 +--
 net/bluetooth/hci_sysfs.c        |  1 -
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cd5b126..fa8fe72 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -593,6 +593,37 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
 
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
+/*
+ * hci_conn_get() and hci_conn_put() are used to control the life-time of an
+ * "hci_conn" object. They do not guarantee that the hci_conn object is running,
+ * working or anything else. They just guarantee that the object is available
+ * and can be dereferenced. So you can use its locks, local variables and any
+ * other constant data.
+ * Before accessing runtime data, you _must_ lock the object and then check that
+ * it is still running. As soon as you release the locks, the connection might
+ * get dropped, though.
+ *
+ * On the other hand, hci_conn_hold() and hci_conn_drop() are used to control
+ * how long the underlying connection is held. So every channel that runs on the
+ * hci_conn object calls this to prevent the connection from disappearing. As
+ * long as you hold a device, you must also guarantee that you have a valid
+ * reference to the device via hci_conn_get() (or the initial reference from
+ * hci_conn_add()).
+ * The hold()/drop() ref-count is known to drop below 0 sometimes, which doesn't
+ * break because nobody cares for that. But this means, we cannot use
+ * _get()/_drop() in it, but require the caller to have a valid ref (FIXME).
+ */
+
+static inline void hci_conn_get(struct hci_conn *conn)
+{
+	get_device(&conn->dev);
+}
+
+static inline void hci_conn_put(struct hci_conn *conn)
+{
+	put_device(&conn->dev);
+}
+
 static inline void hci_conn_hold(struct hci_conn *conn)
 {
 	BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 134ab9f..75b75a8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -450,8 +450,7 @@ int hci_conn_del(struct hci_conn *conn)
 
 	hci_dev_put(hdev);
 
-	if (conn->handle == 0)
-		kfree(conn);
+	hci_conn_put(conn);
 
 	return 0;
 }
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..0384b76 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -145,7 +145,6 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
 	}
 
 	device_del(&conn->dev);
-	put_device(&conn->dev);
 
 	hci_dev_put(hdev);
 }
-- 
1.8.1.4

[PATCH 08/16] Bluetooth: hidp: remove unused session->state field

[David Herrmann]

This field is always BT_CONNECTED. Remove it and set it to BT_CONNECTED in
hidp_copy_session() unconditionally.

Also note that this field is totally bogus. Userspace can query an
hidp-session for its state. However, whenever user-space queries us, this
field should be BT_CONNECTED. If it wasn't BT_CONNECTED, then we would be
currently cleaning up the session and the session itself would exit in the
next few milliseconds. Hence, there is no reason to let user-space know
that the session will exit now if they cannot make _any_ use of that.

Thus, remove the field and let user-space think that a session is always
BT_CONNECTED as long as they can query it.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 5 ++---
 net/bluetooth/hidp/hidp.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 143feb2..c2979a7 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -79,7 +79,7 @@ static void __hidp_copy_session(struct hidp_session *session, struct hidp_connin
 	bacpy(&ci->bdaddr, &session->bdaddr);
 
 	ci->flags = session->flags;
-	ci->state = session->state;
+	ci->state = BT_CONNECTED;
 
 	ci->vendor  = 0x0000;
 	ci->product = 0x0000;
@@ -966,7 +966,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 	down_write(&hidp_session_sem);
 
 	s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
-	if (s && s->state == BT_CONNECTED) {
+	if (s) {
 		up_write(&hidp_session_sem);
 		return -EEXIST;
 	}
@@ -988,7 +988,6 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 
 	session->ctrl_sock = ctrl_sock;
 	session->intr_sock = intr_sock;
-	session->state     = BT_CONNECTED;
 
 	session->conn = hidp_get_connection(session);
 	if (!session->conn) {
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index af1bcc8..57a6191 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -135,7 +135,6 @@ struct hidp_session {
 
 	bdaddr_t bdaddr;
 
-	unsigned long state;
 	unsigned long flags;
 	unsigned long idle_to;
 
-- 
1.8.1.4

[PATCH 09/16] Bluetooth: hidp: test "terminate" before sleeping

[David Herrmann]

The "terminate" flag is guaranteed to be set before the session terminates
and the handlers are woken up. Hence, we need to add it to the
sleep-condition.

Note that testing the flags is not enough as nothing prevents us from
setting the flags again after the session-handler terminated.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index c2979a7..162f22d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -327,11 +327,13 @@ static int hidp_get_raw_report(struct hid_device *hid,
 
 	/* Wait for the return of the report. The returned report
 	   gets put in session->report_return.  */
-	while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+	while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
+	       !atomic_read(&session->terminate)) {
 		int res;
 
 		res = wait_event_interruptible_timeout(session->report_queue,
-			!test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
+			!test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)
+				|| atomic_read(&session->terminate),
 			5*HZ);
 		if (res == 0) {
 			/* timeout */
@@ -396,11 +398,13 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
 		goto err;
 
 	/* Wait for the ACK from the device. */
-	while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)) {
+	while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
+	       !atomic_read(&session->terminate)) {
 		int res;
 
 		res = wait_event_interruptible_timeout(session->report_queue,
-			!test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags),
+			!test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
+				|| atomic_read(&session->terminate),
 			10*HZ);
 		if (res == 0) {
 			/* timeout */
-- 
1.8.1.4

[PATCH 10/16] Bluetooth: allow constant arguments for bacmp()/bacpy()

[David Herrmann]

There is no reason to require the source arguments to be writeable so fix
this to allow constant source addresses.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/bluetooth.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index d3222d1..9928cef 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -170,11 +170,11 @@ typedef struct {
 #define BDADDR_LOCAL (&(bdaddr_t) {{0, 0, 0, 0xff, 0xff, 0xff} })
 
 /* Copy, swap, convert BD Address */
-static inline int bacmp(bdaddr_t *ba1, bdaddr_t *ba2)
+static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
 {
 	return memcmp(ba1, ba2, sizeof(bdaddr_t));
 }
-static inline void bacpy(bdaddr_t *dst, bdaddr_t *src)
+static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
 {
 	memcpy(dst, src, sizeof(bdaddr_t));
 }
-- 
1.8.1.4

[PATCH 11/16] Bluetooth: l2cap: add l2cap_sock_get_hci_conn() helper

[David Herrmann]

Many sub-modules like HIDP get l2cap sockets as input from user-space but
need access to the hci_conn to properly track the connection. This helper
returns a reference to the current hci_conn of a given l2cap-socket. If
the socket is not connected, NULL is returned.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/l2cap.h |  1 +
 net/bluetooth/l2cap_sock.c    | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index ae6210e..17d0940 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -788,6 +788,7 @@ extern bool disable_ertm;
 int l2cap_init_sockets(void);
 void l2cap_cleanup_sockets(void);
 bool is_l2cap_socket(struct socket *s);
+struct hci_conn *l2cap_sock_get_hci_conn(struct socket *s);
 
 void __l2cap_connect_rsp_defer(struct l2cap_chan *chan);
 int __l2cap_wait_ack(struct sock *sk);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 598b167..f69f604 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -49,6 +49,27 @@ bool is_l2cap_socket(struct socket *s)
 }
 EXPORT_SYMBOL(is_l2cap_socket);
 
+struct hci_conn *l2cap_sock_get_hci_conn(struct socket *sock)
+{
+	struct sock *sk = sock->sk;
+	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+	struct hci_conn *hcon;
+
+	l2cap_chan_lock(chan);
+
+	if (chan->conn) {
+		hcon = chan->conn->hcon;
+		hci_conn_get(hcon);
+	} else {
+		hcon = NULL;
+	}
+
+	l2cap_chan_unlock(chan);
+
+	return hcon;
+}
+EXPORT_SYMBOL(l2cap_sock_get_hci_conn);
+
 static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 {
 	struct sock *sk = sock->sk;
-- 
1.8.1.4

[PATCH 12/16] Bluetooth: add hci_conn_user sub-modules

[David Herrmann]

Several sub-modules like HIDP, rfcomm, ... need to track HCI connections.
The hci_conn->dev object is used as parent for sysfs devices so the
sub-modules need to be notified when the hci_conn object is removed from
sysfs.

This patch introduces hci_conn_user objects which contain a "probe" and
"remove" callback. You can register these on any hci_conn object and if it
is active, the "probe" callback will get called. Otherwise, an error is
returned.

The hci_conn object will call your "remove" callback directly before it is
removed from user-space. This allows you to remove your submodules
_before_ the hci_conn object is removed.

At any time you can asynchronously unregister your hci_conn_user object if
your submodule vanishes before the hci_conn object does.

There is no way around hci_conn_user. If we want wire-protocols in the
kernel, we always want the hci_conn object as parent in the sysfs tree. We
cannot use a channel here since we might need multiple channels for a
single protocol.
But the problem is, we _must_ get notified when an hci_conn object is
removed. We cannot use reference-counting for object-removal! This is not
how it works. If a hardware is removed, we should immediately remove the
object from sysfs. Any other behavior would be inconsistent with the rest
of the system. Also note that device_del() might sleep, but it doesn't
wait for user-space or block very long. It only _unlinks_ the object from
sysfs and the whole device-tree. Everything else is handled by ref-counts!
This is exactly what the other sub-modules must do: unlink their devices
when the "remove" hci_conn_user callback is called. They should not do any
cleanup or synchronous shutdowns.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/net/bluetooth/hci_core.h | 11 +++++++
 net/bluetooth/hci_conn.c         | 66 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index fa8fe72..3779568 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -296,6 +296,7 @@ struct hci_conn {
 	struct list_head list;
 
 	atomic_t	refcnt;
+	struct list_head users;
 
 	bdaddr_t	dst;
 	__u8		dst_type;
@@ -352,6 +353,12 @@ struct hci_conn {
 	void (*disconn_cfm_cb)	(struct hci_conn *conn, u8 reason);
 };
 
+struct hci_conn_user {
+	struct list_head list;
+	int (*probe) (struct hci_conn *conn, struct hci_conn_user *user);
+	void (*remove) (struct hci_conn *conn, struct hci_conn_user *user);
+};
+
 struct hci_chan {
 	struct list_head list;
 	__u16 handle;
@@ -593,6 +600,10 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
 
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
+int hci_conn_register_user(struct hci_conn *conn, struct hci_conn_user *user);
+void hci_conn_unregister_user(struct hci_conn *conn,
+			      struct hci_conn_user *user);
+
 /*
  * hci_conn_get() and hci_conn_put() are used to control the life-time of an
  * "hci_conn" object. They do not guarantee that the hci_conn object is running,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 75b75a8..5ba5e29 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -342,6 +342,70 @@ static void hci_conn_auto_accept(unsigned long arg)
 		     &conn->dst);
 }
 
+int hci_conn_register_user(struct hci_conn *conn, struct hci_conn_user *user)
+{
+	struct hci_dev *hdev = conn->hdev;
+	int ret;
+
+	hci_dev_lock(hdev);
+
+	if (user->list.next || user->list.prev) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (!device_is_registered(&conn->dev)) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	ret = user->probe(conn, user);
+	if (ret)
+		goto out_unlock;
+
+	list_add(&user->list, &conn->users);
+	ret = 0;
+
+out_unlock:
+	hci_dev_unlock(hdev);
+	return ret;
+}
+EXPORT_SYMBOL(hci_conn_register_user);
+
+void hci_conn_unregister_user(struct hci_conn *conn,
+			      struct hci_conn_user *user)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	hci_dev_lock(hdev);
+
+	if (!user->list.next || !user->list.prev)
+		goto out_unlock;
+
+	list_del(&user->list);
+	user->list.next = NULL;
+	user->list.prev = NULL;
+	user->remove(conn, user);
+
+out_unlock:
+	hci_dev_unlock(hdev);
+}
+EXPORT_SYMBOL(hci_conn_unregister_user);
+
+static void hci_conn_unregister_all_users(struct hci_conn *conn)
+{
+	struct hci_conn_user *user;
+
+	while (!list_empty(&conn->users)) {
+		user = list_first_entry(&conn->users, struct hci_conn_user,
+					list);
+		list_del(&user->list);
+		user->list.next = NULL;
+		user->list.prev = NULL;
+		user->remove(conn, user);
+	}
+}
+
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 {
 	struct hci_conn *conn;
@@ -384,6 +448,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
 	skb_queue_head_init(&conn->data_q);
 
 	INIT_LIST_HEAD(&conn->chan_list);
+	INIT_LIST_HEAD(&conn->users);
 
 	INIT_DELAYED_WORK(&conn->disc_work, hci_conn_timeout);
 	setup_timer(&conn->idle_timer, hci_conn_idle, (unsigned long)conn);
@@ -446,6 +511,7 @@ int hci_conn_del(struct hci_conn *conn)
 
 	skb_queue_purge(&conn->data_q);
 
+	hci_conn_unregister_all_users(conn);
 	hci_conn_del_sysfs(conn);
 
 	hci_dev_put(hdev);
-- 
1.8.1.4

[PATCH 13/16] Bluetooth: hidp: move hidp_schedule() to core.c

[David Herrmann]

There is no reason to keep this helper in the header file. No other file
depends on it so move it into hidp/core.c where it belongs.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 9 +++++++++
 net/bluetooth/hidp/hidp.h | 9 ---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 162f22d..2b79bf7 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -59,6 +59,15 @@ static unsigned char hidp_keycode[256] = {
 
 static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
 
+static inline void hidp_schedule(struct hidp_session *session)
+{
+	struct sock *ctrl_sk = session->ctrl_sock->sk;
+	struct sock *intr_sk = session->intr_sock->sk;
+
+	wake_up_interruptible(sk_sleep(ctrl_sk));
+	wake_up_interruptible(sk_sleep(intr_sk));
+}
+
 static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
 {
 	struct hidp_session *session;
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 57a6191..c844420 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -174,15 +174,6 @@ struct hidp_session {
 	int waiting_for_startup;
 };
 
-static inline void hidp_schedule(struct hidp_session *session)
-{
-	struct sock *ctrl_sk = session->ctrl_sock->sk;
-	struct sock *intr_sk = session->intr_sock->sk;
-
-	wake_up_interruptible(sk_sleep(ctrl_sk));
-	wake_up_interruptible(sk_sleep(intr_sk));
-}
-
 /* HIDP init defines */
 extern int __init hidp_init_sockets(void);
 extern void __exit hidp_cleanup_sockets(void);
-- 
1.8.1.4

[PATCH 14/16] Bluetooth: hidp: add new session-management helpers

[David Herrmann]

This is a rewrite of the HIDP session management. It implements HIDP as a
hci_conn_user sub-module so we get proper notification when the underlying
connection goes away.

The helpers are not yet used but only added in this commit. The old
session management is still used and will be removed in a following patch.

The old session-management was flawed. Hotplugging is horribly broken and
we have no way of getting notified when the underlying connection goes
down. The whole idea of removing the HID/input sub-devices from within the
session itself is broken and suffers from major dead-locks. We never can
guarantee that the session can unregister itself as long as we use
synchronous shutdowns. This can only work with asynchronous shutdowns.
However, in this case we _must_ be able to unregister the session from the
outside as otherwise the hci_conn object might be unlinked before we are.

The new session-management is based on hci_conn_user. There is only one
way how to add a session and how to delete a session: "probe" and "remove"
callbacks from hci_conn_user.
This guarantees that the session can be registered and unregistered at
_any_ time without any synchronous shutdown.
On the other hand, much work has been put into proper session-refcounting.
We can unregister/unlink the session only if we can guarantee that it will
stay alive. But for asynchronous shutdowns we never know when the last
user goes away so we must use proper ref-counting.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 539 ++++++++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hidp/hidp.h |  52 +++--
 2 files changed, 570 insertions(+), 21 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 2b79bf7..3f48a03 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1,6 +1,7 @@
 /*
    HIDP implementation for Linux Bluetooth stack (BlueZ).
    Copyright (C) 2003-2004 Marcel Holtmann <marcel@holtmann.org>
+   Copyright (C) 2013 David Herrmann <dh.herrmann@gmail.com>
 
    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
@@ -20,6 +21,7 @@
    SOFTWARE IS DISCLAIMED.
 */
 
+#include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/file.h>
 #include <linux/kthread.h>
@@ -59,6 +61,13 @@ static unsigned char hidp_keycode[256] = {
 
 static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
 
+static int hidp_session_probe(struct hci_conn *conn,
+			      struct hci_conn_user *user);
+static void hidp_session_remove(struct hci_conn *conn,
+				struct hci_conn_user *user);
+static int hidp_session_thread(void *arg);
+static void hidp_session_terminate(struct hidp_session *s);
+
 static inline void hidp_schedule(struct hidp_session *session)
 {
 	struct sock *ctrl_sk = session->ctrl_sock->sk;
@@ -960,6 +969,535 @@ fault:
 	return err;
 }
 
+/* initialize session devices */
+static int hidp_session_dev_init(struct hidp_session *session,
+				 struct hidp_connadd_req *req)
+{
+	int ret;
+
+	if (req->rd_size > 0) {
+		ret = hidp_setup_hid(session, req);
+		if (ret && ret != -ENODEV)
+			return ret;
+	}
+
+	if (!session->hid) {
+		ret = hidp_setup_input(session, req);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/* destroy session devices */
+static void hidp_session_dev_destroy(struct hidp_session *session)
+{
+	if (session->hid)
+		put_device(&session->hid->dev);
+	else if (session->input)
+		input_put_device(session->input);
+
+	kfree(session->rd_data);
+	session->rd_data = NULL;
+}
+
+/* add HID/input devices to their underlying bus systems */
+static int hidp_session_dev_add(struct hidp_session *session)
+{
+	int ret;
+
+	/* Both HID and input systems drop a ref-count when unregistering the
+	 * device but they don't take a ref-count when registering them. Work
+	 * around this by explicitly taking a refcount during registration
+	 * which is dropped automatically by unregistering the devices. */
+
+	if (session->hid) {
+		ret = hid_add_device(session->hid);
+		if (ret)
+			return ret;
+		get_device(&session->hid->dev);
+	} else if (session->input) {
+		ret = input_register_device(session->input);
+		if (ret)
+			return ret;
+		input_get_device(session->input);
+	}
+
+	return 0;
+}
+
+/* remove HID/input devices from their bus systems */
+static void hidp_session_dev_del(struct hidp_session *session)
+{
+	if (session->hid)
+		hid_destroy_device(session->hid);
+	else if (session->input)
+		input_unregister_device(session->input);
+}
+
+/*
+ * Create new session object
+ * Allocate session object, initialize static fields, copy input data into the
+ * object and take a reference to all sub-objects.
+ * This returns 0 on success and puts a pointer to the new session object in
+ * \out. Otherwise, an error code is returned.
+ * The new session object has an initial ref-count of 1.
+ */
+static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr,
+			    struct socket *ctrl_sock,
+			    struct socket *intr_sock,
+			    struct hidp_connadd_req *req,
+			    struct hci_conn *conn)
+{
+	struct hidp_session *session;
+	int ret;
+	struct bt_sock *ctrl, *intr;
+
+	ctrl = bt_sk(ctrl_sock->sk);
+	intr = bt_sk(intr_sock->sk);
+
+	session = kzalloc(sizeof(*session), GFP_KERNEL);
+	if (!session)
+		return -ENOMEM;
+
+	/* object and runtime management */
+	kref_init(&session->ref);
+	atomic_set(&session->state, HIDP_SESSION_IDLING);
+	init_waitqueue_head(&session->state_queue);
+	session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
+
+	/* connection management */
+	bacpy(&session->bdaddr, bdaddr);
+	session->conn = conn;
+	session->user.probe = hidp_session_probe;
+	session->user.remove = hidp_session_remove;
+	session->ctrl_sock = ctrl_sock;
+	session->intr_sock = intr_sock;
+	skb_queue_head_init(&session->ctrl_transmit);
+	skb_queue_head_init(&session->intr_transmit);
+	session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl)->chan->omtu,
+					l2cap_pi(ctrl)->chan->imtu);
+	session->intr_mtu = min_t(uint, l2cap_pi(intr)->chan->omtu,
+					l2cap_pi(intr)->chan->imtu);
+	session->idle_to = req->idle_to;
+
+	/* device management */
+	setup_timer(&session->timer, hidp_idle_timeout,
+		    (unsigned long)session);
+
+	/* session data */
+	mutex_init(&session->report_mutex);
+	init_waitqueue_head(&session->report_queue);
+
+	ret = hidp_session_dev_init(session, req);
+	if (ret)
+		goto err_free;
+
+	hci_conn_get(session->conn);
+	get_file(session->intr_sock->file);
+	get_file(session->ctrl_sock->file);
+	*out = session;
+	return 0;
+
+err_free:
+	kfree(session);
+	return ret;
+}
+
+/* increase ref-count of the given session by one */
+static void hidp_session_get(struct hidp_session *session)
+{
+	kref_get(&session->ref);
+}
+
+/* release callback */
+static void session_free(struct kref *ref)
+{
+	struct hidp_session *session = container_of(ref, struct hidp_session,
+						    ref);
+
+	hidp_session_dev_destroy(session);
+	skb_queue_purge(&session->ctrl_transmit);
+	skb_queue_purge(&session->intr_transmit);
+	fput(session->intr_sock->file);
+	fput(session->ctrl_sock->file);
+	hci_conn_put(session->conn);
+	kfree(session);
+}
+
+/* decrease ref-count of the given session by one */
+static void hidp_session_put(struct hidp_session *session)
+{
+	kref_put(&session->ref, session_free);
+}
+
+/*
+ * Search the list of active sessions for a session with target address
+ * \bdaddr. You must hold at least a read-lock on \hidp_session_sem. As long as
+ * you do not release this lock, the session objects cannot vanish and you can
+ * safely take a reference to the session yourself.
+ */
+static struct hidp_session *__hidp_session_find(const bdaddr_t *bdaddr)
+{
+	struct hidp_session *session;
+
+	list_for_each_entry(session, &hidp_session_list, list) {
+		if (!bacmp(bdaddr, &session->bdaddr))
+			return session;
+	}
+
+	return NULL;
+}
+
+/*
+ * Same as __hidp_session_find() but no locks must be held. This also takes a
+ * reference of the returned session (if non-NULL) so you must drop this
+ * reference if you no longer use the object.
+ */
+static struct hidp_session *hidp_session_find(const bdaddr_t *bdaddr)
+{
+	struct hidp_session *session;
+
+	down_read(&hidp_session_sem);
+
+	session = __hidp_session_find(bdaddr);
+	if (session)
+		hidp_session_get(session);
+
+	up_read(&hidp_session_sem);
+
+	return session;
+}
+
+/*
+ * Start session synchronously
+ * This starts a session thread and waits until initialization
+ * is done or returns an error if it couldn't be started.
+ * If this returns 0 the session thread is up and running. You must call
+ * hipd_session_stop_sync() before deleting any runtime resources.
+ */
+static int hidp_session_start_sync(struct hidp_session *session)
+{
+	unsigned int vendor, product;
+
+	if (session->hid) {
+		vendor  = session->hid->vendor;
+		product = session->hid->product;
+	} else if (session->input) {
+		vendor  = session->input->id.vendor;
+		product = session->input->id.product;
+	} else {
+		vendor = 0x0000;
+		product = 0x0000;
+	}
+
+	session->task = kthread_run(hidp_session_thread, session,
+				    "khidpd_%04x%04x", vendor, product);
+	if (IS_ERR(session->task))
+		return PTR_ERR(session->task);
+
+	while (atomic_read(&session->state) <= HIDP_SESSION_IDLING)
+		wait_event(session->state_queue,
+			   atomic_read(&session->state) > HIDP_SESSION_IDLING);
+
+	return 0;
+}
+
+/*
+ * Terminate session thread
+ * Wake up session thread and notify it to stop. This is asynchronous and
+ * returns immediately. Call this whenever a runtime error occurs and you want
+ * the session to stop.
+ * Note: wake_up_process() performs any necessary memory-barriers for us.
+ */
+static void hidp_session_terminate(struct hidp_session *session)
+{
+	atomic_inc(&session->terminate);
+	wake_up_process(session->task);
+}
+
+/*
+ * Probe HIDP session
+ * This is called from the hci_conn core when our hci_conn_user object is bound
+ * to the hci-connection. We get the session via the \user object and can now
+ * start the session thread, register the HID/input devices and link it into
+ * the global session list.
+ * The global session-list owns its own reference to the session object so you
+ * can drop your own reference after registering the hci_conn_user object.
+ */
+static int hidp_session_probe(struct hci_conn *conn,
+			      struct hci_conn_user *user)
+{
+	struct hidp_session *session = container_of(user,
+						    struct hidp_session,
+						    user);
+	struct hidp_session *s;
+	int ret;
+
+	down_write(&hidp_session_sem);
+
+	/* check that no other session for this device exists */
+	s = __hidp_session_find(&session->bdaddr);
+	if (s) {
+		ret = -EEXIST;
+		goto out_unlock;
+	}
+
+	ret = hidp_session_start_sync(session);
+	if (ret)
+		goto out_unlock;
+
+	ret = hidp_session_dev_add(session);
+	if (ret)
+		goto out_stop;
+
+	hidp_session_get(session);
+	list_add(&session->list, &hidp_session_list);
+	ret = 0;
+	goto out_unlock;
+
+out_stop:
+	hidp_session_terminate(session);
+out_unlock:
+	up_write(&hidp_session_sem);
+	return ret;
+}
+
+/*
+ * Remove HIDP session
+ * This is called from the hci_conn core when either we explicitly unregistered
+ * the hci_conn_user object or if the underlying connection is shut down.
+ * We signal the hidp-session thread to shut down, unregister the HID/input
+ * devices and unlink the session from the global list.
+ * This drops the reference to the session that is owned by the global
+ * session-list.
+ * Note: We _must_ not synchronosly wait for the session-thread to shut down.
+ * This is, because the session-thread might be waiting for an HCI lock that is
+ * held while we are called. Therefore, we only unregister the devices and
+ * notify the session-thread to terminate. The thread itself owns a reference
+ * to the session object so it can safely shut down.
+ */
+static void hidp_session_remove(struct hci_conn *conn,
+				struct hci_conn_user *user)
+{
+	struct hidp_session *session = container_of(user,
+						    struct hidp_session,
+						    user);
+
+	down_write(&hidp_session_sem);
+
+	hidp_session_terminate(session);
+	hidp_session_dev_del(session);
+	list_del(&session->list);
+
+	up_write(&hidp_session_sem);
+
+	hidp_session_put(session);
+}
+
+/*
+ * Session Worker
+ * This performs the actual main-loop of the HIDP worker. We first check
+ * whether the underlying connection is still alive, then parse all pending
+ * messages and finally send all outstanding messages.
+ */
+static void hidp_session_run(struct hidp_session *session)
+{
+	struct sock *ctrl_sk = session->ctrl_sock->sk;
+	struct sock *intr_sk = session->intr_sock->sk;
+	struct sk_buff *skb;
+
+	for (;;) {
+		/*
+		 * This thread can be woken up two ways:
+		 *  - You call hidp_session_terminate() which sets the
+		 *    session->terminate flag and wakes this thread up.
+		 *  - Via modifying the socket state of ctrl/intr_sock. This
+		 *    thread is woken up by ->sk_state_changed().
+		 *
+		 * Note: set_current_state() performs any necessary
+		 * memory-barriers for us.
+		 */
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (atomic_read(&session->terminate))
+			break;
+
+		if (ctrl_sk->sk_state != BT_CONNECTED ||
+		    intr_sk->sk_state != BT_CONNECTED)
+			break;
+
+		/* parse incoming intr-skbs */
+		while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
+			skb_orphan(skb);
+			if (!skb_linearize(skb))
+				hidp_recv_intr_frame(session, skb);
+			else
+				kfree_skb(skb);
+		}
+
+		/* send pending intr-skbs */
+		hidp_process_intr_transmit(session);
+
+		/* parse incoming ctrl-skbs */
+		while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
+			skb_orphan(skb);
+			if (!skb_linearize(skb))
+				hidp_recv_ctrl_frame(session, skb);
+			else
+				kfree_skb(skb);
+		}
+
+		/* send pending ctrl-skbs */
+		hidp_process_ctrl_transmit(session);
+
+		schedule();
+	}
+
+	atomic_inc(&session->terminate);
+	set_current_state(TASK_RUNNING);
+}
+
+/*
+ * HIDP session thread
+ * This thread runs the I/O for a single HIDP session. Startup is synchronous
+ * which allows us to take references to ourself here instead of doing that in
+ * the caller.
+ * When we are ready to run we notify the caller and call hidp_session_run().
+ */
+static int hidp_session_thread(void *arg)
+{
+	struct hidp_session *session = arg;
+	wait_queue_t ctrl_wait, intr_wait;
+
+	BT_DBG("session %p", session);
+
+	/* initialize runtime environment */
+	hidp_session_get(session);
+	__module_get(THIS_MODULE);
+	set_user_nice(current, -15);
+	hidp_set_timer(session);
+
+	init_waitqueue_entry(&ctrl_wait, current);
+	init_waitqueue_entry(&intr_wait, current);
+	add_wait_queue(sk_sleep(session->ctrl_sock->sk), &ctrl_wait);
+	add_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+	/* This memory barrier is paired with wq_has_sleeper(). See
+	 * sock_poll_wait() for more information why this is needed. */
+	smp_mb();
+
+	/* notify synchronous startup that we're ready */
+	atomic_inc(&session->state);
+	wake_up(&session->state_queue);
+
+	/* run session */
+	hidp_session_run(session);
+
+	/* cleanup runtime environment */
+	remove_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+	remove_wait_queue(sk_sleep(session->intr_sock->sk), &ctrl_wait);
+	wake_up_interruptible(&session->report_queue);
+	hidp_del_timer(session);
+
+	/*
+	 * If we stopped ourself due to any internal signal, we should try to
+	 * unregister our own session here to avoid having it linger until the
+	 * parent hci_conn dies or user-space cleans it up.
+	 * This does not deadlock as we don't do any synchronous shutdown.
+	 * Instead, this call has the same semantics as if user-space tried to
+	 * delete the session.
+	 */
+	hci_conn_unregister_user(session->conn, &session->user);
+	hidp_session_put(session);
+
+	module_put_and_exit(0);
+	return 0;
+}
+
+static int hidp_verify_sockets(struct socket *ctrl_sock,
+			       struct socket *intr_sock)
+{
+	struct bt_sock *ctrl, *intr;
+	struct hidp_session *session;
+
+	if (!is_l2cap_socket(ctrl_sock) || !is_l2cap_socket(intr_sock))
+		return -EINVAL;
+
+	ctrl = bt_sk(ctrl_sock->sk);
+	intr = bt_sk(intr_sock->sk);
+
+	if (bacmp(&ctrl->src, &intr->src) || bacmp(&ctrl->dst, &intr->dst))
+		return -ENOTUNIQ;
+	if (ctrl->sk.sk_state != BT_CONNECTED ||
+	    intr->sk.sk_state != BT_CONNECTED)
+		return -EBADFD;
+
+	/* early session check, we check again during session registration */
+	session = hidp_session_find(&ctrl->dst);
+	if (session) {
+		hidp_session_put(session);
+		return -EEXIST;
+	}
+
+	return 0;
+}
+
+int hidp_connection_add(struct hidp_connadd_req *req,
+			struct socket *ctrl_sock,
+			struct socket *intr_sock)
+{
+	struct hidp_session *session;
+	struct hci_conn *conn;
+	int ret;
+
+	ret = hidp_verify_sockets(ctrl_sock, intr_sock);
+	if (ret)
+		return ret;
+
+	conn = l2cap_sock_get_hci_conn(ctrl_sock);
+	if (!conn)
+		return -EBADFD;
+
+	ret = hidp_session_new(&session, &bt_sk(ctrl_sock->sk)->dst, ctrl_sock,
+			       intr_sock, req, conn);
+	if (ret)
+		goto out_conn;
+
+	ret = hci_conn_register_user(conn, &session->user);
+	if (ret)
+		goto out_session;
+
+	ret = 0;
+
+out_session:
+	hidp_session_put(session);
+out_conn:
+	hci_conn_put(conn);
+	return ret;
+}
+
+int hidp_connection_del(struct hidp_conndel_req *req)
+{
+	struct hidp_session *session;
+
+	session = hidp_session_find(&req->bdaddr);
+	if (!session)
+		return -ENOENT;
+
+	if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG))
+		hidp_send_ctrl_message(session,
+				       HIDP_TRANS_HID_CONTROL |
+				         HIDP_CTRL_VIRTUAL_CABLE_UNPLUG,
+				       NULL, 0);
+	else
+		hci_conn_unregister_user(session->conn, &session->user);
+
+	hidp_session_put(session);
+
+	return 0;
+}
+
 int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
 {
 	struct hidp_session *session, *s;
@@ -1204,6 +1742,7 @@ module_init(hidp_init);
 module_exit(hidp_exit);
 
 MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
 MODULE_DESCRIPTION("Bluetooth HIDP ver " VERSION);
 MODULE_VERSION(VERSION);
 MODULE_LICENSE("GPL");
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index c844420..54622db 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -24,7 +24,9 @@
 #define __HIDP_H
 
 #include <linux/types.h>
+#include <linux/kref.h>
 #include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
 
 /* HIDP header masks */
 #define HIDP_HEADER_TRANS_MASK			0xf0
@@ -119,42 +121,54 @@ struct hidp_connlist_req {
 	struct hidp_conninfo __user *ci;
 };
 
+int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
+int hidp_connection_del(struct hidp_conndel_req *req);
 int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
 int hidp_del_connection(struct hidp_conndel_req *req);
 int hidp_get_connlist(struct hidp_connlist_req *req);
 int hidp_get_conninfo(struct hidp_conninfo *ci);
 
+enum hidp_session_state {
+	HIDP_SESSION_IDLING,
+	HIDP_SESSION_RUNNING,
+};
+
 /* HIDP session defines */
 struct hidp_session {
 	struct list_head list;
+	struct kref ref;
 
-	struct hci_conn *conn;
+	/* runtime management */
+	atomic_t state;
+	wait_queue_head_t state_queue;
+	atomic_t terminate;
+	struct task_struct *task;
+	unsigned long flags;
 
+	/* connection management */
+	bdaddr_t bdaddr;
+	struct hci_conn *conn;
+	struct hci_conn_user user;
 	struct socket *ctrl_sock;
 	struct socket *intr_sock;
-
-	bdaddr_t bdaddr;
-
-	unsigned long flags;
-	unsigned long idle_to;
-
+	struct sk_buff_head ctrl_transmit;
+	struct sk_buff_head intr_transmit;
 	uint ctrl_mtu;
 	uint intr_mtu;
+	unsigned long idle_to;
 
-	atomic_t terminate;
-	struct task_struct *task;
-
-	unsigned char keys[8];
-	unsigned char leds;
-
+	/* device management */
 	struct input_dev *input;
-
 	struct hid_device *hid;
-
 	struct timer_list timer;
 
-	struct sk_buff_head ctrl_transmit;
-	struct sk_buff_head intr_transmit;
+	/* Report descriptor */
+	__u8 *rd_data;
+	uint rd_size;
+
+	/* session data */
+	unsigned char keys[8];
+	unsigned char leds;
 
 	/* Used in hidp_get_raw_report() */
 	int waiting_report_type; /* HIDP_DATA_RTYPE_* */
@@ -166,10 +180,6 @@ struct hidp_session {
 	/* Used in hidp_output_raw_report() */
 	int output_report_success; /* boolean */
 
-	/* Report descriptor */
-	__u8 *rd_data;
-	uint rd_size;
-
 	wait_queue_head_t startup_queue;
 	int waiting_for_startup;
 };
-- 
1.8.1.4

[PATCH 15/16] Bluetooth: hidp: remove old session-management

[David Herrmann]

We have the full new session-management now available so lets switch over
and remove all the old code. Few semantics changed, so we need to adjust
the sock.c callers a bit. But this mostly simplifies the logic.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 334 ++--------------------------------------------
 net/bluetooth/hidp/hidp.h |   5 -
 net/bluetooth/hidp/sock.c |  21 +--
 3 files changed, 16 insertions(+), 344 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 3f48a03..5d07589 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -77,21 +77,7 @@ static inline void hidp_schedule(struct hidp_session *session)
 	wake_up_interruptible(sk_sleep(intr_sk));
 }
 
-static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
-{
-	struct hidp_session *session;
-
-	BT_DBG("");
-
-	list_for_each_entry(session, &hidp_session_list, list) {
-		if (!bacmp(bdaddr, &session->bdaddr))
-			return session;
-	}
-
-	return NULL;
-}
-
-static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
+static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
 {
 	memset(ci, 0, sizeof(*ci));
 	bacpy(&ci->bdaddr, &session->bdaddr);
@@ -453,8 +439,7 @@ static void hidp_idle_timeout(unsigned long arg)
 {
 	struct hidp_session *session = (struct hidp_session *) arg;
 
-	atomic_inc(&session->terminate);
-	wake_up_process(session->task);
+	hidp_session_terminate(session);
 }
 
 static void hidp_set_timer(struct hidp_session *session)
@@ -522,8 +507,7 @@ static void hidp_process_hid_control(struct hidp_session *session,
 		skb_queue_purge(&session->ctrl_transmit);
 		skb_queue_purge(&session->intr_transmit);
 
-		atomic_inc(&session->terminate);
-		wake_up_process(current);
+		hidp_session_terminate(session);
 	}
 }
 
@@ -683,119 +667,6 @@ static void hidp_process_ctrl_transmit(struct hidp_session *session)
 	}
 }
 
-static int hidp_session(void *arg)
-{
-	struct hidp_session *session = arg;
-	struct sock *ctrl_sk = session->ctrl_sock->sk;
-	struct sock *intr_sk = session->intr_sock->sk;
-	struct sk_buff *skb;
-	wait_queue_t ctrl_wait, intr_wait;
-
-	BT_DBG("session %p", session);
-
-	__module_get(THIS_MODULE);
-	set_user_nice(current, -15);
-
-	init_waitqueue_entry(&ctrl_wait, current);
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
-	add_wait_queue(sk_sleep(intr_sk), &intr_wait);
-	session->waiting_for_startup = 0;
-	wake_up_interruptible(&session->startup_queue);
-	set_current_state(TASK_INTERRUPTIBLE);
-	while (!atomic_read(&session->terminate)) {
-		if (ctrl_sk->sk_state != BT_CONNECTED ||
-				intr_sk->sk_state != BT_CONNECTED)
-			break;
-
-		while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
-			skb_orphan(skb);
-			if (!skb_linearize(skb))
-				hidp_recv_intr_frame(session, skb);
-			else
-				kfree_skb(skb);
-		}
-
-		hidp_process_intr_transmit(session);
-
-		while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
-			skb_orphan(skb);
-			if (!skb_linearize(skb))
-				hidp_recv_ctrl_frame(session, skb);
-			else
-				kfree_skb(skb);
-		}
-
-		hidp_process_ctrl_transmit(session);
-
-		schedule();
-		set_current_state(TASK_INTERRUPTIBLE);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(sk_sleep(intr_sk), &intr_wait);
-	remove_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
-
-	clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
-	clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
-	wake_up_interruptible(&session->report_queue);
-
-	down_write(&hidp_session_sem);
-
-	hidp_del_timer(session);
-
-	if (session->input) {
-		input_unregister_device(session->input);
-		session->input = NULL;
-	}
-
-	if (session->hid) {
-		hid_destroy_device(session->hid);
-		session->hid = NULL;
-	}
-
-	/* Wakeup user-space polling for socket errors */
-	session->intr_sock->sk->sk_err = EUNATCH;
-	session->ctrl_sock->sk->sk_err = EUNATCH;
-
-	hidp_schedule(session);
-
-	fput(session->intr_sock->file);
-
-	wait_event_timeout(*(sk_sleep(ctrl_sk)),
-		(ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
-
-	fput(session->ctrl_sock->file);
-
-	list_del(&session->list);
-
-	up_write(&hidp_session_sem);
-
-	kfree(session->rd_data);
-	kfree(session);
-	module_put_and_exit(0);
-	return 0;
-}
-
-static struct hci_conn *hidp_get_connection(struct hidp_session *session)
-{
-	bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
-	bdaddr_t *dst = &bt_sk(session->ctrl_sock->sk)->dst;
-	struct hci_conn *conn;
-	struct hci_dev *hdev;
-
-	hdev = hci_get_route(dst, src);
-	if (!hdev)
-		return NULL;
-
-	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
-	hci_dev_unlock(hdev);
-
-	hci_dev_put(hdev);
-
-	return conn;
-}
-
 static int hidp_setup_input(struct hidp_session *session,
 				struct hidp_connadd_req *req)
 {
@@ -1498,187 +1369,6 @@ int hidp_connection_del(struct hidp_conndel_req *req)
 	return 0;
 }
 
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
-{
-	struct hidp_session *session, *s;
-	int vendor, product;
-	int err;
-
-	BT_DBG("");
-
-	if (!is_l2cap_socket(ctrl_sock) || !is_l2cap_socket(intr_sock))
-		return -EINVAL;
-	if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
-			bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))
-		return -ENOTUNIQ;
-
-	BT_DBG("rd_data %p rd_size %d", req->rd_data, req->rd_size);
-
-	down_write(&hidp_session_sem);
-
-	s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
-	if (s) {
-		up_write(&hidp_session_sem);
-		return -EEXIST;
-	}
-
-	session = kzalloc(sizeof(struct hidp_session), GFP_KERNEL);
-	if (!session) {
-		up_write(&hidp_session_sem);
-		return -ENOMEM;
-	}
-
-	bacpy(&session->bdaddr, &bt_sk(ctrl_sock->sk)->dst);
-
-	session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl_sock->sk)->chan->omtu,
-					l2cap_pi(ctrl_sock->sk)->chan->imtu);
-	session->intr_mtu = min_t(uint, l2cap_pi(intr_sock->sk)->chan->omtu,
-					l2cap_pi(intr_sock->sk)->chan->imtu);
-
-	BT_DBG("ctrl mtu %d intr mtu %d", session->ctrl_mtu, session->intr_mtu);
-
-	session->ctrl_sock = ctrl_sock;
-	session->intr_sock = intr_sock;
-
-	session->conn = hidp_get_connection(session);
-	if (!session->conn) {
-		err = -ENOTCONN;
-		goto failed;
-	}
-
-	setup_timer(&session->timer, hidp_idle_timeout, (unsigned long)session);
-
-	skb_queue_head_init(&session->ctrl_transmit);
-	skb_queue_head_init(&session->intr_transmit);
-
-	mutex_init(&session->report_mutex);
-	init_waitqueue_head(&session->report_queue);
-	init_waitqueue_head(&session->startup_queue);
-	session->waiting_for_startup = 1;
-	session->flags   = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
-	session->idle_to = req->idle_to;
-
-	list_add(&session->list, &hidp_session_list);
-
-	if (req->rd_size > 0) {
-		err = hidp_setup_hid(session, req);
-		if (err && err != -ENODEV)
-			goto purge;
-	}
-
-	if (!session->hid) {
-		err = hidp_setup_input(session, req);
-		if (err < 0)
-			goto purge;
-	}
-
-	hidp_set_timer(session);
-
-	if (session->hid) {
-		vendor  = session->hid->vendor;
-		product = session->hid->product;
-	} else if (session->input) {
-		vendor  = session->input->id.vendor;
-		product = session->input->id.product;
-	} else {
-		vendor = 0x0000;
-		product = 0x0000;
-	}
-
-	session->task = kthread_run(hidp_session, session, "khidpd_%04x%04x",
-							vendor, product);
-	if (IS_ERR(session->task)) {
-		err = PTR_ERR(session->task);
-		goto unlink;
-	}
-
-	while (session->waiting_for_startup) {
-		wait_event_interruptible(session->startup_queue,
-			!session->waiting_for_startup);
-	}
-
-	if (session->hid)
-		err = hid_add_device(session->hid);
-	else
-		err = input_register_device(session->input);
-
-	if (err < 0) {
-		atomic_inc(&session->terminate);
-		wake_up_process(session->task);
-		up_write(&hidp_session_sem);
-		return err;
-	}
-
-	if (session->input) {
-		hidp_send_ctrl_message(session,
-			HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_BOOT, NULL, 0);
-		session->flags |= (1 << HIDP_BOOT_PROTOCOL_MODE);
-
-		session->leds = 0xff;
-		hidp_input_event(session->input, EV_LED, 0, 0);
-	}
-
-	up_write(&hidp_session_sem);
-	return 0;
-
-unlink:
-	hidp_del_timer(session);
-
-	if (session->input) {
-		input_unregister_device(session->input);
-		session->input = NULL;
-	}
-
-	if (session->hid) {
-		hid_destroy_device(session->hid);
-		session->hid = NULL;
-	}
-
-	kfree(session->rd_data);
-	session->rd_data = NULL;
-
-purge:
-	list_del(&session->list);
-
-	skb_queue_purge(&session->ctrl_transmit);
-	skb_queue_purge(&session->intr_transmit);
-
-failed:
-	up_write(&hidp_session_sem);
-
-	kfree(session);
-	return err;
-}
-
-int hidp_del_connection(struct hidp_conndel_req *req)
-{
-	struct hidp_session *session;
-	int err = 0;
-
-	BT_DBG("");
-
-	down_read(&hidp_session_sem);
-
-	session = __hidp_get_session(&req->bdaddr);
-	if (session) {
-		if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) {
-			hidp_send_ctrl_message(session,
-				HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, NULL, 0);
-		} else {
-			/* Flush the transmit queues */
-			skb_queue_purge(&session->ctrl_transmit);
-			skb_queue_purge(&session->intr_transmit);
-
-			atomic_inc(&session->terminate);
-			wake_up_process(session->task);
-		}
-	} else
-		err = -ENOENT;
-
-	up_read(&hidp_session_sem);
-	return err;
-}
-
 int hidp_get_connlist(struct hidp_connlist_req *req)
 {
 	struct hidp_session *session;
@@ -1691,7 +1381,7 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
 	list_for_each_entry(session, &hidp_session_list, list) {
 		struct hidp_conninfo ci;
 
-		__hidp_copy_session(session, &ci);
+		hidp_copy_session(session, &ci);
 
 		if (copy_to_user(req->ci, &ci, sizeof(ci))) {
 			err = -EFAULT;
@@ -1712,18 +1402,14 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
 int hidp_get_conninfo(struct hidp_conninfo *ci)
 {
 	struct hidp_session *session;
-	int err = 0;
 
-	down_read(&hidp_session_sem);
-
-	session = __hidp_get_session(&ci->bdaddr);
-	if (session)
-		__hidp_copy_session(session, ci);
-	else
-		err = -ENOENT;
+	session = hidp_session_find(&ci->bdaddr);
+	if (session) {
+		hidp_copy_session(session, ci);
+		hidp_session_put(session);
+	}
 
-	up_read(&hidp_session_sem);
-	return err;
+	return session ? 0 : -ENOENT;
 }
 
 static int __init hidp_init(void)
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 54622db..fdff80c 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -123,8 +123,6 @@ struct hidp_connlist_req {
 
 int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
 int hidp_connection_del(struct hidp_conndel_req *req);
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
-int hidp_del_connection(struct hidp_conndel_req *req);
 int hidp_get_connlist(struct hidp_connlist_req *req);
 int hidp_get_conninfo(struct hidp_conninfo *ci);
 
@@ -179,9 +177,6 @@ struct hidp_session {
 
 	/* Used in hidp_output_raw_report() */
 	int output_report_success; /* boolean */
-
-	wait_queue_head_t startup_queue;
-	int waiting_for_startup;
 };
 
 /* HIDP init defines */
diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index ca86436..c1a41fd 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -77,21 +77,12 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
 			return err;
 		}
 
-		if (csock->sk->sk_state != BT_CONNECTED ||
-				isock->sk->sk_state != BT_CONNECTED) {
-			sockfd_put(csock);
-			sockfd_put(isock);
-			return -EBADFD;
-		}
+		err = hidp_connection_add(&ca, csock, isock);
+		if (!err && copy_to_user(argp, &ca, sizeof(ca)))
+			err = -EFAULT;
 
-		err = hidp_add_connection(&ca, csock, isock);
-		if (!err) {
-			if (copy_to_user(argp, &ca, sizeof(ca)))
-				err = -EFAULT;
-		} else {
-			sockfd_put(csock);
-			sockfd_put(isock);
-		}
+		sockfd_put(csock);
+		sockfd_put(isock);
 
 		return err;
 
@@ -102,7 +93,7 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
 		if (copy_from_user(&cd, argp, sizeof(cd)))
 			return -EFAULT;
 
-		return hidp_del_connection(&cd);
+		return hidp_connection_del(&cd);
 
 	case HIDPGETCONNLIST:
 		if (copy_from_user(&cl, argp, sizeof(cl)))
-- 
1.8.1.4

[PATCH 16/16] Bluetooth: hidp: handle kernel_sendmsg() errors correctly

[David Herrmann]

We shouldn't push back the skbs if kernel_sendmsg() fails. Instead, we
terminate the connection and drop the skb. Only on EAGAIN we push it back
and return.
l2cap doesn't return EAGAIN, yet, but this guarantees we're safe if it
will at some time in the future.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 net/bluetooth/hidp/core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 5d07589..807523c 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -636,13 +636,19 @@ static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)
 static void hidp_process_intr_transmit(struct hidp_session *session)
 {
 	struct sk_buff *skb;
+	int ret;
 
 	BT_DBG("session %p", session);
 
 	while ((skb = skb_dequeue(&session->intr_transmit))) {
-		if (hidp_send_frame(session->intr_sock, skb->data, skb->len) < 0) {
+		ret = hidp_send_frame(session->intr_sock, skb->data, skb->len);
+		if (ret == -EAGAIN) {
 			skb_queue_head(&session->intr_transmit, skb);
 			break;
+		} else if (ret < 0) {
+			hidp_session_terminate(session);
+			kfree_skb(skb);
+			break;
 		}
 
 		hidp_set_timer(session);
@@ -653,13 +659,19 @@ static void hidp_process_intr_transmit(struct hidp_session *session)
 static void hidp_process_ctrl_transmit(struct hidp_session *session)
 {
 	struct sk_buff *skb;
+	int ret;
 
 	BT_DBG("session %p", session);
 
 	while ((skb = skb_dequeue(&session->ctrl_transmit))) {
-		if (hidp_send_frame(session->ctrl_sock, skb->data, skb->len) < 0) {
+		ret = hidp_send_frame(session->ctrl_sock, skb->data, skb->len);
+		if (ret == -EAGAIN) {
 			skb_queue_head(&session->ctrl_transmit, skb);
 			break;
+		} else if (ret < 0) {
+			hidp_session_terminate(session);
+			kfree_skb(skb);
+			break;
 		}
 
 		hidp_set_timer(session);
-- 
1.8.1.4

Re: [PATCH 00/16] Rewrite HIDP Session Management

[David Herrmann]

ping?

On Sun, Feb 24, 2013 at 7:36 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> If anyone tested "hotplugging" with HIDP in the last years, there's a 50% chance
> you get an oops when "unplugging" devices. I never got around fixing it until
> now. I took the time to thoroughly review HIDP, hci_dev and hci_conn
> interaction and it turned out to be totally broken. The thing is that HIDP
> highly depends on the underlying hci_conn object. However, there is no way to
> notify HIDP when the connection is shut down. So this is where I started fixing
> things.
>
> First, a list of bugs that all played together and are mostly fixed in this
> series:
>
> * hci_conn use after free:
>   hci_conn_del() calls hci_conn_put_device() which might call
>   hci_conn_del_sysfs(), which itself calls put_device() which might call
>   bt_link_release() which calls kfree(hci_conn).
>   That is, the "conn->handle" check in hci_conn_del() dereferences "conn" after
>   it has been freed.
>   Note that the "might"-case is the usual case. So this is a real bug.
>
> * kfree() after device_initialize()
>   During hci_conn_add() we call hci_conn_init_sysfs() which calls
>   device_initialize() on hci_conn. However, if we never call
>   hci_conn_add_sysfs(), then hci_conn_del() will not use "put_device()" to drop
>   the reference but instead call kfree(hci_conn).
>   This is not allowed (see documentation for device_initialize()).
>
> * ref-count for device_del()
>   We use a "devcnt" reference-count in every hci_conn structure which controls
>   when we call device_del(hci_conn). This isn't a but itself, but discouraged
>   behavior. device_del() is supposed to be called when the (virtual) hardware
>   of the device goes away. So when the hci-connection is closed, we are supposed
>   to call device_del() immediately. If the direct call to device_del()
>   introduces bugs, we _must_ fix the bugs instead of delaying
>   device_del(). The correct fix is to notify all childs that the
>   device goes away (they can now call device_del(child)) and then call
>   device_del() afterwards. But this needs to be done synchronously.
>
> * device_find_child() + device_move()
>   This is really wrong. When an hci_conn object goes away, we move all it's
>   childs to the root node. The code doesn't have a comment to say _why_ we do
>   this bug digging in commit-messages it said that client RFCOMM-TTY devices
>   might have a reason to stay alive even though the hci_conn is gone.
>   Could anyone please tell me _one_ reason, why an RFCOMM-TTY object should stay
>   even though the hci_conn is gone?
>
> * hci_conn use after free
>   If an hci_conn is grabbed via hci_conn_hold_device(), but the connection
>   hasn't been added to user-space, yet, then hci_conn_del() might call kfree()
>   on the device, even though a later call to hci_conn_put_device() will still
>   access it.
>   This is a theoretical bug as we have other means to avoid it. But it's still
>   ugly.
>
> * hci_conn_put() called more often than hci_conn_hold()
>   I added a debug-print to hci_conn_put() and the ref-count dropped below 0
>   everytime I disconnected a device. It doesn't break anything, but we really
>   should avoid this!
>
> * hci_conn_put_device() doesn't take the parent hci_dev into account.
>   So the bug we tried to fix with _hold/put_device(), comes up again if we
>   unplug the adapter instead of disconnecting the remote device.
>
> * HIDP doesn't consider "terminate" in wait-conditions
>   No wait-condition in HIDP watches for "terminate" to become non-zero, even
>   though it is a _very_ likely condition to terminate HIDP.
>
> * HIDP removes the HID/input devices asynchronously from within the session
>   instead of synchronously when the connection goes down. This causes the
>   session-thread to use the connection even though it's gone.
>
> That's all I can remember for now, but there's some more small bugs that I fixed
> together with the session-rewrite.
>
> This series includes some small fixes/cleanups that can be applied right away
> (which would simplify this series a lot). The other patches depend on each other
> so they cannot be cherry-picked.
>
> I tested this series and it works perfectly well. 10 connect/disconnect
> cycles worked without a bug. I can shut down the device, unplug the adapter or
> do an explicit disconnect without an oops.
> I haven't tested suspend/resume, yet, though.
>
> Please review!
> Thanks
> David
>
>
> Overview:
>
> Patch 1-3: They remove a bogus return-code in bt_sock_unregister() which doesn't
>            make sense to me.
>
> Patch 4: Add is_l2cap_socket() which allows HIDP to verify that the passed
>          sockets are real l2cap sockets.
>
> Patch 5-7: Introduce ref-counting for hci_conn objects so we can have a
>            reference to them in the HIDP session. Note that we cannot reuse the
>            current "refcnt" as this is used for something totally differen: It
>            counts the number of active users on the underlying connection. It
>            doesn't ref-count the hci_conn object itself!
>
> Patch 8-10: Some small HIDP and HCI-core fixes
>
> Patch 11: Add l2cap_sock_get_hci_conn() helper to allow HIDP to use l2cap
>           sockets and retrieve the underlying hci_conn object instead of using
>           an unnecessary hash-table lookup.
>
> Patch 12: Add hci_conn_user objects. This allows external protocols to bind to
>           an hci_conn object and get notified whenever the connection is closed
>           and unlinked from sysfs.
>
> Patch 13: Small HIDP cleanup patch
>
> Patch 14-15: Rewrite the HIDP session-management based on hci_conn_user objects.
>              I haven't found an easy way to do this in many small patches as the
>              new session-management is totally different than the previous one.
>              Hence, I first add the new helpers and then remove the old helpers
>              and do the switch in a follow-up patch. This makes reviewing
>              easier.
>
> Patch 16: Again a small fix for HIDP that uses the new session-management
>           helpers.
>
> David Herrmann (16):
>   Bluetooth: discard bt_sock_unregister() errors
>   Bluetooth: change bt_sock_unregister() to return void
>   Bluetooth: hidp: simplify error path in sock-init
>   Bluetooth: hidp: verify l2cap sockets
>   Bluetooth: rename hci_conn_put to hci_conn_drop
>   Bluetooth: remove unneeded hci_conn_hold/put_device()
>   Bluetooth: introduce hci_conn ref-counting
>   Bluetooth: hidp: remove unused session->state field
>   Bluetooth: hidp: test "terminate" before sleeping
>   Bluetooth: allow constant arguments for bacmp()/bacpy()
>   Bluetooth: l2cap: add l2cap_sock_get_hci_conn() helper
>   Bluetooth: add hci_conn_user sub-modules
>   Bluetooth: hidp: move hidp_schedule() to core.c
>   Bluetooth: hidp: add new session-management helpers
>   Bluetooth: hidp: remove old session-management
>   Bluetooth: hidp: handle kernel_sendmsg() errors correctly
>
>  include/net/bluetooth/bluetooth.h |   6 +-
>  include/net/bluetooth/hci_core.h  |  46 ++-
>  include/net/bluetooth/l2cap.h     |   2 +
>  net/bluetooth/af_bluetooth.c      |  15 +-
>  net/bluetooth/bnep/sock.c         |   4 +-
>  net/bluetooth/cmtp/sock.c         |   4 +-
>  net/bluetooth/hci_conn.c          |  92 ++++-
>  net/bluetooth/hci_event.c         |  40 +-
>  net/bluetooth/hci_sock.c          |   4 +-
>  net/bluetooth/hci_sysfs.c         |   1 -
>  net/bluetooth/hidp/core.c         | 787 +++++++++++++++++++++++++-------------
>  net/bluetooth/hidp/hidp.h         |  67 ++--
>  net/bluetooth/hidp/sock.c         |  35 +-
>  net/bluetooth/l2cap_core.c        |   6 +-
>  net/bluetooth/l2cap_sock.c        |  31 +-
>  net/bluetooth/mgmt.c              |   6 +-
>  net/bluetooth/rfcomm/sock.c       |   3 +-
>  net/bluetooth/sco.c               |   9 +-
>  net/bluetooth/smp.c               |   2 +-
>  19 files changed, 737 insertions(+), 423 deletions(-)
>
> --
> 1.8.1.4
>

Re: [PATCH 00/16] Rewrite HIDP Session Management

[Karl Relton]

On Tue, 2013-03-12 at 18:24 +0100, David Herrmann wrote:
> ping?
> 

I looked over the patches, and they seemed sensible to me. Thanks for
the good work!

Regards
Karl

> On Sun, Feb 24, 2013 at 7:36 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> > Hi
> >
> > If anyone tested "hotplugging" with HIDP in the last years, there's a 50% chance
> > you get an oops when "unplugging" devices. I never got around fixing it until
> > now. I took the time to thoroughly review HIDP, hci_dev and hci_conn
> > interaction and it turned out to be totally broken. The thing is that HIDP
> > highly depends on the underlying hci_conn object. However, there is no way to
> > notify HIDP when the connection is shut down. So this is where I started fixing
> > things.
> >
> > First, a list of bugs that all played together and are mostly fixed in this
> > series:
> >
> > * hci_conn use after free:
> >   hci_conn_del() calls hci_conn_put_device() which might call
> >   hci_conn_del_sysfs(), which itself calls put_device() which might call
> >   bt_link_release() which calls kfree(hci_conn).
> >   That is, the "conn->handle" check in hci_conn_del() dereferences "conn" after
> >   it has been freed.
> >   Note that the "might"-case is the usual case. So this is a real bug.
> >
> > * kfree() after device_initialize()
> >   During hci_conn_add() we call hci_conn_init_sysfs() which calls
> >   device_initialize() on hci_conn. However, if we never call
> >   hci_conn_add_sysfs(), then hci_conn_del() will not use "put_device()" to drop
> >   the reference but instead call kfree(hci_conn).
> >   This is not allowed (see documentation for device_initialize()).
> >
> > * ref-count for device_del()
> >   We use a "devcnt" reference-count in every hci_conn structure which controls
> >   when we call device_del(hci_conn). This isn't a but itself, but discouraged
> >   behavior. device_del() is supposed to be called when the (virtual) hardware
> >   of the device goes away. So when the hci-connection is closed, we are supposed
> >   to call device_del() immediately. If the direct call to device_del()
> >   introduces bugs, we _must_ fix the bugs instead of delaying
> >   device_del(). The correct fix is to notify all childs that the
> >   device goes away (they can now call device_del(child)) and then call
> >   device_del() afterwards. But this needs to be done synchronously.
> >
> > * device_find_child() + device_move()
> >   This is really wrong. When an hci_conn object goes away, we move all it's
> >   childs to the root node. The code doesn't have a comment to say _why_ we do
> >   this bug digging in commit-messages it said that client RFCOMM-TTY devices
> >   might have a reason to stay alive even though the hci_conn is gone.
> >   Could anyone please tell me _one_ reason, why an RFCOMM-TTY object should stay
> >   even though the hci_conn is gone?
> >
> > * hci_conn use after free
> >   If an hci_conn is grabbed via hci_conn_hold_device(), but the connection
> >   hasn't been added to user-space, yet, then hci_conn_del() might call kfree()
> >   on the device, even though a later call to hci_conn_put_device() will still
> >   access it.
> >   This is a theoretical bug as we have other means to avoid it. But it's still
> >   ugly.
> >
> > * hci_conn_put() called more often than hci_conn_hold()
> >   I added a debug-print to hci_conn_put() and the ref-count dropped below 0
> >   everytime I disconnected a device. It doesn't break anything, but we really
> >   should avoid this!
> >
> > * hci_conn_put_device() doesn't take the parent hci_dev into account.
> >   So the bug we tried to fix with _hold/put_device(), comes up again if we
> >   unplug the adapter instead of disconnecting the remote device.
> >
> > * HIDP doesn't consider "terminate" in wait-conditions
> >   No wait-condition in HIDP watches for "terminate" to become non-zero, even
> >   though it is a _very_ likely condition to terminate HIDP.
> >
> > * HIDP removes the HID/input devices asynchronously from within the session
> >   instead of synchronously when the connection goes down. This causes the
> >   session-thread to use the connection even though it's gone.
> >
> > That's all I can remember for now, but there's some more small bugs that I fixed
> > together with the session-rewrite.
> >
> > This series includes some small fixes/cleanups that can be applied right away
> > (which would simplify this series a lot). The other patches depend on each other
> > so they cannot be cherry-picked.
> >
> > I tested this series and it works perfectly well. 10 connect/disconnect
> > cycles worked without a bug. I can shut down the device, unplug the adapter or
> > do an explicit disconnect without an oops.
> > I haven't tested suspend/resume, yet, though.
> >
> > Please review!
> > Thanks
> > David
> >
> >
> > Overview:
> >
> > Patch 1-3: They remove a bogus return-code in bt_sock_unregister() which doesn't
> >            make sense to me.
> >
> > Patch 4: Add is_l2cap_socket() which allows HIDP to verify that the passed
> >          sockets are real l2cap sockets.
> >
> > Patch 5-7: Introduce ref-counting for hci_conn objects so we can have a
> >            reference to them in the HIDP session. Note that we cannot reuse the
> >            current "refcnt" as this is used for something totally differen: It
> >            counts the number of active users on the underlying connection. It
> >            doesn't ref-count the hci_conn object itself!
> >
> > Patch 8-10: Some small HIDP and HCI-core fixes
> >
> > Patch 11: Add l2cap_sock_get_hci_conn() helper to allow HIDP to use l2cap
> >           sockets and retrieve the underlying hci_conn object instead of using
> >           an unnecessary hash-table lookup.
> >
> > Patch 12: Add hci_conn_user objects. This allows external protocols to bind to
> >           an hci_conn object and get notified whenever the connection is closed
> >           and unlinked from sysfs.
> >
> > Patch 13: Small HIDP cleanup patch
> >
> > Patch 14-15: Rewrite the HIDP session-management based on hci_conn_user objects.
> >              I haven't found an easy way to do this in many small patches as the
> >              new session-management is totally different than the previous one.
> >              Hence, I first add the new helpers and then remove the old helpers
> >              and do the switch in a follow-up patch. This makes reviewing
> >              easier.
> >
> > Patch 16: Again a small fix for HIDP that uses the new session-management
> >           helpers.
> >
> > David Herrmann (16):
> >   Bluetooth: discard bt_sock_unregister() errors
> >   Bluetooth: change bt_sock_unregister() to return void
> >   Bluetooth: hidp: simplify error path in sock-init
> >   Bluetooth: hidp: verify l2cap sockets
> >   Bluetooth: rename hci_conn_put to hci_conn_drop
> >   Bluetooth: remove unneeded hci_conn_hold/put_device()
> >   Bluetooth: introduce hci_conn ref-counting
> >   Bluetooth: hidp: remove unused session->state field
> >   Bluetooth: hidp: test "terminate" before sleeping
> >   Bluetooth: allow constant arguments for bacmp()/bacpy()
> >   Bluetooth: l2cap: add l2cap_sock_get_hci_conn() helper
> >   Bluetooth: add hci_conn_user sub-modules
> >   Bluetooth: hidp: move hidp_schedule() to core.c
> >   Bluetooth: hidp: add new session-management helpers
> >   Bluetooth: hidp: remove old session-management
> >   Bluetooth: hidp: handle kernel_sendmsg() errors correctly
> >
> >  include/net/bluetooth/bluetooth.h |   6 +-
> >  include/net/bluetooth/hci_core.h  |  46 ++-
> >  include/net/bluetooth/l2cap.h     |   2 +
> >  net/bluetooth/af_bluetooth.c      |  15 +-
> >  net/bluetooth/bnep/sock.c         |   4 +-
> >  net/bluetooth/cmtp/sock.c         |   4 +-
> >  net/bluetooth/hci_conn.c          |  92 ++++-
> >  net/bluetooth/hci_event.c         |  40 +-
> >  net/bluetooth/hci_sock.c          |   4 +-
> >  net/bluetooth/hci_sysfs.c         |   1 -
> >  net/bluetooth/hidp/core.c         | 787 +++++++++++++++++++++++++-------------
> >  net/bluetooth/hidp/hidp.h         |  67 ++--
> >  net/bluetooth/hidp/sock.c         |  35 +-
> >  net/bluetooth/l2cap_core.c        |   6 +-
> >  net/bluetooth/l2cap_sock.c        |  31 +-
> >  net/bluetooth/mgmt.c              |   6 +-
> >  net/bluetooth/rfcomm/sock.c       |   3 +-
> >  net/bluetooth/sco.c               |   9 +-
> >  net/bluetooth/smp.c               |   2 +-
> >  19 files changed, 737 insertions(+), 423 deletions(-)
> >
> > --
> > 1.8.1.4