From: David Herrmann <dh.herrmann@gmail.com>
To: linux-bluetooth@vger.kernel.org
Cc: Marcel Holtmann <marcel@holtmann.org>,
Gustavo Padovan <gustavo@padovan.org>,
David Herrmann <dh.herrmann@gmail.com>
Subject: [PATCH 07/16] Bluetooth: introduce hci_conn ref-counting
Date: Sun, 24 Feb 2013 19:36:57 +0100 [thread overview]
Message-ID: <1361731026-7428-8-git-send-email-dh.herrmann@gmail.com> (raw)
In-Reply-To: <1361731026-7428-1-git-send-email-dh.herrmann@gmail.com>
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
next prev parent reply other threads:[~2013-02-24 18:36 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-24 18:36 [PATCH 00/16] Rewrite HIDP Session Management David Herrmann
2013-02-24 18:36 ` [PATCH 01/16] Bluetooth: discard bt_sock_unregister() errors David Herrmann
2013-02-26 19:52 ` Gustavo Padovan
2013-02-24 18:36 ` [PATCH 02/16] Bluetooth: change bt_sock_unregister() to return void David Herrmann
2013-02-26 19:52 ` Gustavo Padovan
2013-02-24 18:36 ` [PATCH 03/16] Bluetooth: hidp: simplify error path in sock-init David Herrmann
2013-02-26 19:56 ` Gustavo Padovan
2013-02-27 11:16 ` David Herrmann
2013-02-24 18:36 ` [PATCH 04/16] Bluetooth: hidp: verify l2cap sockets David Herrmann
2013-02-26 20:01 ` Gustavo Padovan
2013-02-27 11:14 ` David Herrmann
2013-02-24 18:36 ` [PATCH 05/16] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
2013-02-24 18:36 ` [PATCH 06/16] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
2013-02-24 18:36 ` David Herrmann [this message]
2013-02-24 18:36 ` [PATCH 08/16] Bluetooth: hidp: remove unused session->state field David Herrmann
2013-02-24 18:36 ` [PATCH 09/16] Bluetooth: hidp: test "terminate" before sleeping David Herrmann
2013-02-24 18:37 ` [PATCH 10/16] Bluetooth: allow constant arguments for bacmp()/bacpy() David Herrmann
2013-02-24 18:37 ` [PATCH 11/16] Bluetooth: l2cap: add l2cap_sock_get_hci_conn() helper David Herrmann
2013-02-24 18:37 ` [PATCH 12/16] Bluetooth: add hci_conn_user sub-modules David Herrmann
2013-02-24 18:37 ` [PATCH 13/16] Bluetooth: hidp: move hidp_schedule() to core.c David Herrmann
2013-02-24 18:37 ` [PATCH 14/16] Bluetooth: hidp: add new session-management helpers David Herrmann
2013-02-24 18:37 ` [PATCH 15/16] Bluetooth: hidp: remove old session-management David Herrmann
2013-02-24 18:37 ` [PATCH 16/16] Bluetooth: hidp: handle kernel_sendmsg() errors correctly David Herrmann
2013-03-12 17:24 ` [PATCH 00/16] Rewrite HIDP Session Management David Herrmann
2013-03-16 14:09 ` Karl Relton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1361731026-7428-8-git-send-email-dh.herrmann@gmail.com \
--to=dh.herrmann@gmail.com \
--cc=gustavo@padovan.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=marcel@holtmann.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox