[PATCH 00/16] Rewrite HIDP Session Management

[David Herrmann <dh.herrmann@gmail.com>]

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