Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Bluetooth: l2cap: defer conn param update to avoid conn->lock/hdev->lock inversion

When a BLE peripheral sends an L2CAP Connection Parameter Update Request
the processing path is:

process_pending_rx() [takes conn->lock]
l2cap_le_sig_channel()
l2cap_conn_param_update_req()
hci_le_conn_update() [takes hdev->lock]

Meanwhile other code paths take the locks in the opposite order:

l2cap_chan_connect() [takes hdev->lock]
...
mutex_lock(&conn->lock)

l2cap_conn_ready() [hdev->lock via hci_cb_list_lock]
...
mutex_lock(&conn->lock)

This is a classic AB/BA deadlock which lockdep reports as a circular
locking dependency when connecting a BLE MIDI keyboard (Carry-On FC-49).

Fix this by making hci_le_conn_update() defer the HCI command through
hci_cmd_sync_queue() so it no longer needs to take hdev->lock in the
caller context. The sync callback uses __hci_cmd_sync_status_sk() to
wait for the HCI_EV_LE_CONN_UPDATE_COMPLETE event, then updates the
stored connection parameters (hci_conn_params) and notifies userspace
(mgmt_new_conn_param) only after the controller has confirmed the update.

A reference on hci_conn is held via hci_conn_get()/hci_conn_put() for
the lifetime of the queued work to prevent use-after-free, and
hci_conn_valid() is checked before proceeding in case the connection was
removed while the work was pending. The hci_dev_lock is held across
hci_conn_valid() and all conn field accesses to prevent a concurrent
disconnect from invalidating the connection mid-use.

Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

authored by

Mikhail Gavrilov and committed by
Luiz Augusto von Dentz
91b5a598 4f42363c

+89 -30
+1 -1
include/net/bluetooth/hci_core.h
··· 2495 2495 bdaddr_t *bdaddr, u8 addr_type); 2496 2496 2497 2497 int hci_abort_conn(struct hci_conn *conn, u8 reason); 2498 - u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency, 2498 + void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency, 2499 2499 u16 to_multiplier); 2500 2500 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand, 2501 2501 __u8 ltk[16], __u8 key_size);
+86 -19
net/bluetooth/hci_conn.c
··· 480 480 return hci_setup_sync_conn(conn, handle); 481 481 } 482 482 483 - u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency, 484 - u16 to_multiplier) 483 + struct le_conn_update_data { 484 + struct hci_conn *conn; 485 + u16 min; 486 + u16 max; 487 + u16 latency; 488 + u16 to_multiplier; 489 + }; 490 + 491 + static int le_conn_update_sync(struct hci_dev *hdev, void *data) 485 492 { 486 - struct hci_dev *hdev = conn->hdev; 493 + struct le_conn_update_data *d = data; 494 + struct hci_conn *conn = d->conn; 487 495 struct hci_conn_params *params; 488 496 struct hci_cp_le_conn_update cp; 497 + u16 timeout; 498 + u8 store_hint; 499 + int err; 489 500 501 + /* Verify connection is still alive and read conn fields under 502 + * the same lock to prevent a concurrent disconnect from freeing 503 + * or reusing the connection while we build the HCI command. 504 + */ 505 + hci_dev_lock(hdev); 506 + 507 + if (!hci_conn_valid(hdev, conn)) { 508 + hci_dev_unlock(hdev); 509 + return -ECANCELED; 510 + } 511 + 512 + memset(&cp, 0, sizeof(cp)); 513 + cp.handle = cpu_to_le16(conn->handle); 514 + cp.conn_interval_min = cpu_to_le16(d->min); 515 + cp.conn_interval_max = cpu_to_le16(d->max); 516 + cp.conn_latency = cpu_to_le16(d->latency); 517 + cp.supervision_timeout = cpu_to_le16(d->to_multiplier); 518 + cp.min_ce_len = cpu_to_le16(0x0000); 519 + cp.max_ce_len = cpu_to_le16(0x0000); 520 + timeout = conn->conn_timeout; 521 + 522 + hci_dev_unlock(hdev); 523 + 524 + err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CONN_UPDATE, 525 + sizeof(cp), &cp, 526 + HCI_EV_LE_CONN_UPDATE_COMPLETE, 527 + timeout, NULL); 528 + if (err) 529 + return err; 530 + 531 + /* Update stored connection parameters after the controller has 532 + * confirmed the update via the LE Connection Update Complete event. 533 + */ 490 534 hci_dev_lock(hdev); 491 535 492 536 params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type); 493 537 if (params) { 494 - params->conn_min_interval = min; 495 - params->conn_max_interval = max; 496 - params->conn_latency = latency; 497 - params->supervision_timeout = to_multiplier; 538 + params->conn_min_interval = d->min; 539 + params->conn_max_interval = d->max; 540 + params->conn_latency = d->latency; 541 + params->supervision_timeout = d->to_multiplier; 542 + store_hint = 0x01; 543 + } else { 544 + store_hint = 0x00; 498 545 } 499 546 500 547 hci_dev_unlock(hdev); 501 548 502 - memset(&cp, 0, sizeof(cp)); 503 - cp.handle = cpu_to_le16(conn->handle); 504 - cp.conn_interval_min = cpu_to_le16(min); 505 - cp.conn_interval_max = cpu_to_le16(max); 506 - cp.conn_latency = cpu_to_le16(latency); 507 - cp.supervision_timeout = cpu_to_le16(to_multiplier); 508 - cp.min_ce_len = cpu_to_le16(0x0000); 509 - cp.max_ce_len = cpu_to_le16(0x0000); 549 + mgmt_new_conn_param(hdev, &conn->dst, conn->dst_type, store_hint, 550 + d->min, d->max, d->latency, d->to_multiplier); 510 551 511 - hci_send_cmd(hdev, HCI_OP_LE_CONN_UPDATE, sizeof(cp), &cp); 552 + return 0; 553 + } 512 554 513 - if (params) 514 - return 0x01; 555 + static void le_conn_update_complete(struct hci_dev *hdev, void *data, int err) 556 + { 557 + struct le_conn_update_data *d = data; 515 558 516 - return 0x00; 559 + hci_conn_put(d->conn); 560 + kfree(d); 561 + } 562 + 563 + void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency, 564 + u16 to_multiplier) 565 + { 566 + struct le_conn_update_data *d; 567 + 568 + d = kzalloc_obj(*d); 569 + if (!d) 570 + return; 571 + 572 + hci_conn_get(conn); 573 + d->conn = conn; 574 + d->min = min; 575 + d->max = max; 576 + d->latency = latency; 577 + d->to_multiplier = to_multiplier; 578 + 579 + if (hci_cmd_sync_queue(conn->hdev, le_conn_update_sync, d, 580 + le_conn_update_complete) < 0) { 581 + hci_conn_put(conn); 582 + kfree(d); 583 + } 517 584 } 518 585 519 586 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand,
+2 -10
net/bluetooth/l2cap_core.c
··· 4706 4706 l2cap_send_cmd(conn, cmd->ident, L2CAP_CONN_PARAM_UPDATE_RSP, 4707 4707 sizeof(rsp), &rsp); 4708 4708 4709 - if (!err) { 4710 - u8 store_hint; 4711 - 4712 - store_hint = hci_le_conn_update(hcon, min, max, latency, 4713 - to_multiplier); 4714 - mgmt_new_conn_param(hcon->hdev, &hcon->dst, hcon->dst_type, 4715 - store_hint, min, max, latency, 4716 - to_multiplier); 4717 - 4718 - } 4709 + if (!err) 4710 + hci_le_conn_update(hcon, min, max, latency, to_multiplier); 4719 4711 4720 4712 return 0; 4721 4713 }