Re: [PATCH] net/bluetooth: fix erroneous use of bitmap_from_u64()

From: Linus Torvalds
Date: Sun Jun 05 2022 - 14:52:14 EST

On Sun, Jun 5, 2022 at 9:34 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> That code shouldn't use DECLARE_BITMAP() at all, it should just use
>
> struct bdaddr_list_with_flags {
> ..
> unsigned long flags;
> };
>
> and then use '&br_params->flags' when it needs the actual atomic
> 'set_bit()' things and friends,

Actually, I'm not convinced those things should be atomic at all.

*Most* of the accesses to those connection flags seem to be with
hci_dev_lock() held, and the ones that aren't can't possibly depend on
atomicity since those things are currently copied around with random
other "copy bitmaps" functions.

So I think the bluetooth code would actually be much better off with
something like

/* Bitmask of connection flags */
enum hci_conn_flags {
HCI_CONN_FLAG_REMOTE_WAKEUP = 1,
HCI_CONN_FLAG_DEVICE_PRIVACY = 2,
};
typedef u8 hci_conn_flags_t;

instead of playing games with DECLARE_BITMAP() and then using a mix of
atomic set_bit/clear_bit() and random non-atomic "copy bitmap values
around".

This attached patch builds cleanly for me, doing the above. But see a
few comments about those atomicity issues...

And no, it doesn't really need that new "hci_conn_flags_t", and it
could just use "u32" (which is what "current_flags" and
"supported_flags" use in the code), but those structures that contain
those connection flags do seem to have various other byte fields and
it would appear to pack better using just that simple one-byte type.

It *literally* just uses two bits in it, after all, and as mentioned,
the whole atomicity situation right now is very very dubious, so it
seems to make sense to do something like this.

Reactions?

Linus
include/net/bluetooth/hci_core.h | 17 +++++++----------
net/bluetooth/hci_core.c | 4 ++--
net/bluetooth/hci_request.c | 2 +-
net/bluetooth/hci_sync.c | 6 +++---
net/bluetooth/mgmt.c | 37 ++++++++++++++-----------------------
5 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5a52a2018b56..c0ea2a4892b1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -155,21 +155,18 @@ struct bdaddr_list_with_irk {
u8 local_irk[16];
};

+/* Bitmask of connection flags */
enum hci_conn_flags {
- HCI_CONN_FLAG_REMOTE_WAKEUP,
- HCI_CONN_FLAG_DEVICE_PRIVACY,
-
- __HCI_CONN_NUM_FLAGS,
+ HCI_CONN_FLAG_REMOTE_WAKEUP = 1,
+ HCI_CONN_FLAG_DEVICE_PRIVACY = 2,
};
-
-/* Make sure number of flags doesn't exceed sizeof(current_flags) */
-static_assert(__HCI_CONN_NUM_FLAGS < 32);
+typedef u8 hci_conn_flags_t;

struct bdaddr_list_with_flags {
struct list_head list;
bdaddr_t bdaddr;
u8 bdaddr_type;
- DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
+ hci_conn_flags_t flags;
};

struct bt_uuid {
@@ -576,7 +573,7 @@ struct hci_dev {
struct rfkill *rfkill;

DECLARE_BITMAP(dev_flags, __HCI_NUM_FLAGS);
- DECLARE_BITMAP(conn_flags, __HCI_CONN_NUM_FLAGS);
+ hci_conn_flags_t conn_flags;

__s8 adv_tx_power;
__u8 adv_data[HCI_MAX_EXT_AD_LENGTH];
@@ -775,7 +772,7 @@ struct hci_conn_params {

struct hci_conn *conn;
bool explicit_connect;
- DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
+ hci_conn_flags_t flags;
u8 privacy_mode;
};

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5abb2ca5b129..59a5c1341c26 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2153,7 +2153,7 @@ int hci_bdaddr_list_add_with_flags(struct list_head *list, bdaddr_t *bdaddr,

bacpy(&entry->bdaddr, bdaddr);
entry->bdaddr_type = type;
- bitmap_from_u64(entry->flags, flags);
+ entry->flags = flags;

list_add(&entry->list, list);

@@ -2634,7 +2634,7 @@ int hci_register_dev(struct hci_dev *hdev)
* callback.
*/
if (hdev->wakeup)
- set_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, hdev->conn_flags);
+ hdev->conn_flags |= HCI_CONN_FLAG_REMOTE_WAKEUP;

hci_sock_dev_event(hdev, HCI_DEV_REG);
hci_dev_hold(hdev);
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 635cc5fb451e..38ecaf9264ee 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -482,7 +482,7 @@ static int add_to_accept_list(struct hci_request *req,

/* During suspend, only wakeable devices can be in accept list */
if (hdev->suspended &&
- !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
+ !(params->flags & HCI_CONN_FLAG_REMOTE_WAKEUP))
return 0;

*num_entries += 1;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 4d2203c5f1bb..286d6767f017 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1637,7 +1637,7 @@ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
* indicates that LL Privacy has been enabled and
* HCI_OP_LE_SET_PRIVACY_MODE is supported.
*/
- if (!test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, params->flags))
+ if (!(params->flags & HCI_CONN_FLAG_DEVICE_PRIVACY))
return 0;

irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
@@ -1666,7 +1666,7 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,

/* During suspend, only wakeable devices can be in acceptlist */
if (hdev->suspended &&
- !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
+ !(params->flags & HCI_CONN_FLAG_REMOTE_WAKEUP))
return 0;

/* Select filter policy to accept all advertising */
@@ -4888,7 +4888,7 @@ static int hci_update_event_filter_sync(struct hci_dev *hdev)
hci_clear_event_filter_sync(hdev);

list_for_each_entry(b, &hdev->accept_list, list) {
- if (!test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, b->flags))
+ if (!(b->flags & HCI_CONN_FLAG_REMOTE_WAKEUP))
continue;

bt_dev_dbg(hdev, "Adding event filters for %pMR", &b->bdaddr);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 74937a834648..ae758ab1b558 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4013,10 +4013,11 @@ static int exp_ll_privacy_feature_changed(bool enabled, struct hci_dev *hdev,
memcpy(ev.uuid, rpa_resolution_uuid, 16);
ev.flags = cpu_to_le32((enabled ? BIT(0) : 0) | BIT(1));

+ // Do we need to be atomic with the conn_flags?
if (enabled && privacy_mode_capable(hdev))
- set_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags);
+ hdev->conn_flags |= HCI_CONN_FLAG_DEVICE_PRIVACY;
else
- clear_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags);
+ hdev->conn_flags &= ~HCI_CONN_FLAG_DEVICE_PRIVACY;

return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, hdev,
&ev, sizeof(ev),
@@ -4435,8 +4436,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,

hci_dev_lock(hdev);

- bitmap_to_arr32(&supported_flags, hdev->conn_flags,
- __HCI_CONN_NUM_FLAGS);
+ supported_flags = hdev->conn_flags;

memset(&rp, 0, sizeof(rp));

@@ -4447,8 +4447,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
if (!br_params)
goto done;

- bitmap_to_arr32(&current_flags, br_params->flags,
- __HCI_CONN_NUM_FLAGS);
+ current_flags = br_params->flags;
} else {
params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
le_addr_type(cp->addr.type));
@@ -4456,8 +4455,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
if (!params)
goto done;

- bitmap_to_arr32(&current_flags, params->flags,
- __HCI_CONN_NUM_FLAGS);
+ current_flags = params->flags;
}

bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
@@ -4502,8 +4500,8 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
&cp->addr.bdaddr, cp->addr.type,
__le32_to_cpu(current_flags));

- bitmap_to_arr32(&supported_flags, hdev->conn_flags,
- __HCI_CONN_NUM_FLAGS);
+ // We should take hci_dev_lock() early, I think.. conn_flags can change
+ supported_flags = hdev->conn_flags;

if ((supported_flags | current_flags) != supported_flags) {
bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)",
@@ -4519,7 +4517,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
cp->addr.type);

if (br_params) {
- bitmap_from_u64(br_params->flags, current_flags);
+ br_params->flags = current_flags;
status = MGMT_STATUS_SUCCESS;
} else {
bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)",
@@ -4529,15 +4527,11 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
le_addr_type(cp->addr.type));
if (params) {
- DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
-
- bitmap_from_u64(flags, current_flags);
-
/* Devices using RPAs can only be programmed in the
* acceptlist LL Privacy has been enable otherwise they
* cannot mark HCI_CONN_FLAG_REMOTE_WAKEUP.
*/
- if (test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, flags) &&
+ if ((current_flags & HCI_CONN_FLAG_REMOTE_WAKEUP) &&
!use_ll_privacy(hdev) &&
hci_find_irk_by_addr(hdev, &params->addr,
params->addr_type)) {
@@ -4546,14 +4540,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}

- bitmap_from_u64(params->flags, current_flags);
+ params->flags = current_flags;
status = MGMT_STATUS_SUCCESS;

/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
* has been set.
*/
- if (test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY,
- params->flags))
+ if (params->flags & HCI_CONN_FLAG_DEVICE_PRIVACY)
hci_update_passive_scan(hdev);
} else {
bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
@@ -7154,8 +7147,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
addr_type);
if (params)
- bitmap_to_arr32(&current_flags, params->flags,
- __HCI_CONN_NUM_FLAGS);
+ current_flags = params->flags;
}

err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL);
@@ -7164,8 +7156,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,

added:
device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
- bitmap_to_arr32(&supported_flags, hdev->conn_flags,
- __HCI_CONN_NUM_FLAGS);
+ supported_flags = hdev->conn_flags;
device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
supported_flags, current_flags);