Hi Johan,
> Recently the LE passive scanning and auto-connections feature was
> introduced. It uses the hci_connect_le() API which returns a hci_conn
> along with a reference count to that object. All previous users would
> tie this returned reference to some existing object, such as an L2CAP
> channel, and there'd be no leaked references this way. For
> auto-connections however the reference was returned but not stored
> anywhere, leaving established connections with one higher reference
> count than they should have.
>
> Instead of playing special tricks with hci_conn_hold/drop this patch
> associates the returned reference from hci_connect_le() with the object
> that in practice does own this reference, i.e. the hci_conn_params
> struct that caused us to initiate a connection in the first place. Once
> the connection is established or fails to establish this reference is
> removed appropriately.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@xxxxxxxxx>
> ---
>
> We should ensure that this patch goes to 3.17 since otherwise we have a
> hci_conn reference count leak there for connections created using the
> new passive-scanning auto-connect feature. I wasn't actually able to
> verify that this really fixes the bug (originally reported by Lukasz)
> because of the way that the non-Android bluetoothd behaves (it never
> just closes the ATT socket), however I was at least able to verify that
> there doesn't seem to be any bad side-effects.
>
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_conn.c | 8 ++++++++
> net/bluetooth/hci_core.c | 5 +++++
> net/bluetooth/hci_event.c | 11 +++++++++--
> 4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5f0b77b71b45..b0ded1333865 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -464,6 +464,8 @@ struct hci_conn_params {
> HCI_AUTO_CONN_ALWAYS,
> HCI_AUTO_CONN_LINK_LOSS,
> } auto_connect;
> +
> + struct hci_conn *conn;
> };
>
> extern struct list_head hci_dev_list;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b50dabb3f86a..faff6247ac8f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -589,6 +589,14 @@ EXPORT_SYMBOL(hci_get_route);
> void hci_le_conn_failed(struct hci_conn *conn, u8 status)
> {
> struct hci_dev *hdev = conn->hdev;
> + struct hci_conn_params *params;
> +
> + params = hci_pend_le_action_lookup(&hdev->pend_le_conns, &conn->dst,
> + conn->dst_type);
> + if (params && params->conn) {
> + hci_conn_drop(params->conn);
> + params->conn = NULL;
> + }
>
> conn->state = BT_CLOSED;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index abeb5e47311e..455f3fee46d3 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3729,6 +3729,9 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
> if (!params)
> return;
>
> + if (params->conn)
> + hci_conn_drop(params->conn);
> +
> list_del(¶ms->action);
> list_del(¶ms->list);
> kfree(params);
> @@ -3759,6 +3762,8 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
> struct hci_conn_params *params, *tmp;
>
> list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
> + if (params->conn)
> + hci_conn_drop(params->conn);
> list_del(¶ms->action);
> list_del(¶ms->list);
> kfree(params);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index da7ab6b9bb69..b44b83d4a8e6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4226,8 +4226,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> hci_proto_connect_cfm(conn, ev->status);
>
> params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
> - if (params)
> + if (params) {
> list_del_init(¶ms->action);
> + if (params && params->conn) {
don't we already know that params is valid here?
> + hci_conn_drop(params->conn);
> + params->conn = NULL;
> + }
> + }
>
> unlock:
> hci_update_background_scan(hdev);
> @@ -4309,8 +4314,10 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
>
> conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
> HCI_LE_AUTOCONN_TIMEOUT, HCI_ROLE_MASTER);
> - if (!IS_ERR(conn))
> + if (!IS_ERR(conn)) {
> + params->conn = conn;
> return;
> + }
>
> switch (PTR_ERR(conn)) {
> case -EBUSY:
Regards
Marcel
--
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
