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: don't use bitmaps for random flag accesses

The bluetooth code uses our bitmap infrastructure for the two bits (!)
of connection setup flags, and in the process causes odd problems when
it converts between a bitmap and just the regular values of said bits.

It's completely pointless to do things like bitmap_to_arr32() to convert
a bitmap into a u32. It shoudln't have been a bitmap in the first
place. The reason to use bitmaps is if you have arbitrary number of
bits you want to manage (not two!), or if you rely on the atomicity
guarantees of the bitmap setting and clearing.

The code could use an "atomic_t" and use "atomic_or/andnot()" to set and
clear the bit values, but considering that it then copies the bitmaps
around with "bitmap_to_arr32()" and friends, there clearly cannot be a
lot of atomicity requirements.

So just use a regular integer.

In the process, this avoids the warnings about erroneous use of
bitmap_from_u64() which were triggered on 32-bit architectures when
conversion from a u64 would access two words (and, surprise, surprise,
only one word is needed - and indeed overkill - for a 2-bit bitmap).

That was always problematic, but the compiler seems to notice it and
warn about the invalid pattern only after commit 0a97953fd221 ("lib: add
bitmap_{from,to}_arr64") changed the exact implementation details of
'bitmap_from_u64()', as reported by Sudip Mukherjee and Stephen Rothwell.

Fixes: fe92ee6425a2 ("Bluetooth: hci_core: Rework hci_conn_params flags")
Link: https://lore.kernel.org/all/YpyJ9qTNHJzz0FHY@debian/
Link: https://lore.kernel.org/all/20220606080631.0c3014f2@canb.auug.org.au/
Link: https://lore.kernel.org/all/20220605162537.1604762-1-yury.norov@gmail.com/
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Reviewed-by: Yury Norov <yury.norov@gmail.com>
Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

+27 -39
+7 -10
include/net/bluetooth/hci_core.h
··· 155 155 u8 local_irk[16]; 156 156 }; 157 157 158 + /* Bitmask of connection flags */ 158 159 enum hci_conn_flags { 159 - HCI_CONN_FLAG_REMOTE_WAKEUP, 160 - HCI_CONN_FLAG_DEVICE_PRIVACY, 161 - 162 - __HCI_CONN_NUM_FLAGS, 160 + HCI_CONN_FLAG_REMOTE_WAKEUP = 1, 161 + HCI_CONN_FLAG_DEVICE_PRIVACY = 2, 163 162 }; 164 - 165 - /* Make sure number of flags doesn't exceed sizeof(current_flags) */ 166 - static_assert(__HCI_CONN_NUM_FLAGS < 32); 163 + typedef u8 hci_conn_flags_t; 167 164 168 165 struct bdaddr_list_with_flags { 169 166 struct list_head list; 170 167 bdaddr_t bdaddr; 171 168 u8 bdaddr_type; 172 - DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS); 169 + hci_conn_flags_t flags; 173 170 }; 174 171 175 172 struct bt_uuid { ··· 573 576 struct rfkill *rfkill; 574 577 575 578 DECLARE_BITMAP(dev_flags, __HCI_NUM_FLAGS); 576 - DECLARE_BITMAP(conn_flags, __HCI_CONN_NUM_FLAGS); 579 + hci_conn_flags_t conn_flags; 577 580 578 581 __s8 adv_tx_power; 579 582 __u8 adv_data[HCI_MAX_EXT_AD_LENGTH]; ··· 772 775 773 776 struct hci_conn *conn; 774 777 bool explicit_connect; 775 - DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS); 778 + hci_conn_flags_t flags; 776 779 u8 privacy_mode; 777 780 }; 778 781
+2 -2
net/bluetooth/hci_core.c
··· 2153 2153 2154 2154 bacpy(&entry->bdaddr, bdaddr); 2155 2155 entry->bdaddr_type = type; 2156 - bitmap_from_u64(entry->flags, flags); 2156 + entry->flags = flags; 2157 2157 2158 2158 list_add(&entry->list, list); 2159 2159 ··· 2634 2634 * callback. 2635 2635 */ 2636 2636 if (hdev->wakeup) 2637 - set_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, hdev->conn_flags); 2637 + hdev->conn_flags |= HCI_CONN_FLAG_REMOTE_WAKEUP; 2638 2638 2639 2639 hci_sock_dev_event(hdev, HCI_DEV_REG); 2640 2640 hci_dev_hold(hdev);
+1 -1
net/bluetooth/hci_request.c
··· 482 482 483 483 /* During suspend, only wakeable devices can be in accept list */ 484 484 if (hdev->suspended && 485 - !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags)) 485 + !(params->flags & HCI_CONN_FLAG_REMOTE_WAKEUP)) 486 486 return 0; 487 487 488 488 *num_entries += 1;
+3 -3
net/bluetooth/hci_sync.c
··· 1637 1637 * indicates that LL Privacy has been enabled and 1638 1638 * HCI_OP_LE_SET_PRIVACY_MODE is supported. 1639 1639 */ 1640 - if (!test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, params->flags)) 1640 + if (!(params->flags & HCI_CONN_FLAG_DEVICE_PRIVACY)) 1641 1641 return 0; 1642 1642 1643 1643 irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type); ··· 1666 1666 1667 1667 /* During suspend, only wakeable devices can be in acceptlist */ 1668 1668 if (hdev->suspended && 1669 - !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags)) 1669 + !(params->flags & HCI_CONN_FLAG_REMOTE_WAKEUP)) 1670 1670 return 0; 1671 1671 1672 1672 /* Select filter policy to accept all advertising */ ··· 4888 4888 hci_clear_event_filter_sync(hdev); 4889 4889 4890 4890 list_for_each_entry(b, &hdev->accept_list, list) { 4891 - if (!test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, b->flags)) 4891 + if (!(b->flags & HCI_CONN_FLAG_REMOTE_WAKEUP)) 4892 4892 continue; 4893 4893 4894 4894 bt_dev_dbg(hdev, "Adding event filters for %pMR", &b->bdaddr);
+14 -23
net/bluetooth/mgmt.c
··· 4013 4013 memcpy(ev.uuid, rpa_resolution_uuid, 16); 4014 4014 ev.flags = cpu_to_le32((enabled ? BIT(0) : 0) | BIT(1)); 4015 4015 4016 + // Do we need to be atomic with the conn_flags? 4016 4017 if (enabled && privacy_mode_capable(hdev)) 4017 - set_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags); 4018 + hdev->conn_flags |= HCI_CONN_FLAG_DEVICE_PRIVACY; 4018 4019 else 4019 - clear_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags); 4020 + hdev->conn_flags &= ~HCI_CONN_FLAG_DEVICE_PRIVACY; 4020 4021 4021 4022 return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, hdev, 4022 4023 &ev, sizeof(ev), ··· 4436 4435 4437 4436 hci_dev_lock(hdev); 4438 4437 4439 - bitmap_to_arr32(&supported_flags, hdev->conn_flags, 4440 - __HCI_CONN_NUM_FLAGS); 4438 + supported_flags = hdev->conn_flags; 4441 4439 4442 4440 memset(&rp, 0, sizeof(rp)); 4443 4441 ··· 4447 4447 if (!br_params) 4448 4448 goto done; 4449 4449 4450 - bitmap_to_arr32(&current_flags, br_params->flags, 4451 - __HCI_CONN_NUM_FLAGS); 4450 + current_flags = br_params->flags; 4452 4451 } else { 4453 4452 params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr, 4454 4453 le_addr_type(cp->addr.type)); ··· 4455 4456 if (!params) 4456 4457 goto done; 4457 4458 4458 - bitmap_to_arr32(&current_flags, params->flags, 4459 - __HCI_CONN_NUM_FLAGS); 4459 + current_flags = params->flags; 4460 4460 } 4461 4461 4462 4462 bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr); ··· 4500 4502 &cp->addr.bdaddr, cp->addr.type, 4501 4503 __le32_to_cpu(current_flags)); 4502 4504 4503 - bitmap_to_arr32(&supported_flags, hdev->conn_flags, 4504 - __HCI_CONN_NUM_FLAGS); 4505 + // We should take hci_dev_lock() early, I think.. conn_flags can change 4506 + supported_flags = hdev->conn_flags; 4505 4507 4506 4508 if ((supported_flags | current_flags) != supported_flags) { 4507 4509 bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)", ··· 4517 4519 cp->addr.type); 4518 4520 4519 4521 if (br_params) { 4520 - bitmap_from_u64(br_params->flags, current_flags); 4522 + br_params->flags = current_flags; 4521 4523 status = MGMT_STATUS_SUCCESS; 4522 4524 } else { 4523 4525 bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)", ··· 4527 4529 params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr, 4528 4530 le_addr_type(cp->addr.type)); 4529 4531 if (params) { 4530 - DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS); 4531 - 4532 - bitmap_from_u64(flags, current_flags); 4533 - 4534 4532 /* Devices using RPAs can only be programmed in the 4535 4533 * acceptlist LL Privacy has been enable otherwise they 4536 4534 * cannot mark HCI_CONN_FLAG_REMOTE_WAKEUP. 4537 4535 */ 4538 - if (test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, flags) && 4536 + if ((current_flags & HCI_CONN_FLAG_REMOTE_WAKEUP) && 4539 4537 !use_ll_privacy(hdev) && 4540 4538 hci_find_irk_by_addr(hdev, &params->addr, 4541 4539 params->addr_type)) { ··· 4540 4546 goto unlock; 4541 4547 } 4542 4548 4543 - bitmap_from_u64(params->flags, current_flags); 4549 + params->flags = current_flags; 4544 4550 status = MGMT_STATUS_SUCCESS; 4545 4551 4546 4552 /* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY 4547 4553 * has been set. 4548 4554 */ 4549 - if (test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, 4550 - params->flags)) 4555 + if (params->flags & HCI_CONN_FLAG_DEVICE_PRIVACY) 4551 4556 hci_update_passive_scan(hdev); 4552 4557 } else { 4553 4558 bt_dev_warn(hdev, "No such LE device %pMR (0x%x)", ··· 7147 7154 params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr, 7148 7155 addr_type); 7149 7156 if (params) 7150 - bitmap_to_arr32(&current_flags, params->flags, 7151 - __HCI_CONN_NUM_FLAGS); 7157 + current_flags = params->flags; 7152 7158 } 7153 7159 7154 7160 err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL); ··· 7156 7164 7157 7165 added: 7158 7166 device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action); 7159 - bitmap_to_arr32(&supported_flags, hdev->conn_flags, 7160 - __HCI_CONN_NUM_FLAGS); 7167 + supported_flags = hdev->conn_flags; 7161 7168 device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type, 7162 7169 supported_flags, current_flags); 7163 7170