ping? On Sun, Feb 24, 2013 at 7:36 PM, David Herrmann <dh.herrmann@xxxxxxxxx> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
