| Message ID | 20220605162537.1604762-1-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/bluetooth: fix erroneous use of bitmap_from_u64() | expand |
On Sun, Jun 5, 2022 at 9:25 AM Yury Norov <yury.norov@gmail.com> wrote: > > The commit 0a97953fd221 ("lib: add bitmap_{from,to}_arr64") changed > implementation of bitmap_from_u64(), so that it doesn't typecast > argument to u64, and actually dereferences memory. Gaah. 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 nneds the actual atomic 'set_bit()' things and friends, and then when it copies the flags around it should just use 'flags' as an integer value. The bitmap functions are literally defined to work as "bit N in a set of 'unsigned long'" exactly so that you can do that mixing of values and bit operations, and not have to worry about insane architectures that do big-endian bit ordering or things like that. Using a 'bitmap' as if it's some bigger or potentially variable-sized thing for this kind of flags usage is crazy, when the code already does /* Make sure number of flags doesn't exceed sizeof(current_flags) */ static_assert(__HCI_CONN_NUM_FLAGS < 32); because other parts are limited to 32 bits. I wonder how painful it would be to just fix that odd type mistake. Linus
On Sun, Jun 5, 2022 at 9:34 AM Linus Torvalds <torvalds@linux-foundation.org> 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
On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > *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. I've committed that patch as commit e1cff7002b71 ("bluetooth: don't use bitmaps for random flag accesses"). That basically ends up reverting a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting HCI_CONN_FLAG_REMOTE_WAKEUP") 6126ffabba6b ("Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag") which did horrible things, and would end up overwriting the end of the bitmap allocation on 32-bit architectures. Luiz, if the reason for the change to use a bitmap type was because of some atomicity concerns, then you can do that by (a) change 'hci_conn_flags_t' to be an 'atomic_t' instead of a 'u8' (b) change the regular accesses to it to use 'atomic_read/write()' (c) change the "bitfield" operations to use 'atomic_or/andnot()' but honestly, when it used to mix atomic ops (set_bit/clear_bit/test_bit) with random non-atomic users (bitmap_from_u64(), bitmap_to_arr32() etc) it was never atomic to begin with. Regardless, trying to use bitmaps for this was absolutely not the right thing to ever do. It looks like gcc randomly started complaining when 'bitmap_from_u64()' was changed, but it was buggy before that too. Linus
Hi Linus, On Sun, Jun 5, 2022 at 5:02 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > *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. > > I've committed that patch as commit e1cff7002b71 ("bluetooth: don't > use bitmaps for random flag accesses"). > > That basically ends up reverting > > a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting > HCI_CONN_FLAG_REMOTE_WAKEUP") > 6126ffabba6b ("Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag") > > which did horrible things, and would end up overwriting the end of the > bitmap allocation on 32-bit architectures. > > Luiz, if the reason for the change to use a bitmap type was because of > some atomicity concerns, then you can do that by > > (a) change 'hci_conn_flags_t' to be an 'atomic_t' instead of a 'u8' > > (b) change the regular accesses to it to use 'atomic_read/write()' > > (c) change the "bitfield" operations to use 'atomic_or/andnot()' > > but honestly, when it used to mix atomic ops > (set_bit/clear_bit/test_bit) with random non-atomic users > (bitmap_from_u64(), bitmap_to_arr32() etc) it was never atomic to > begin with. > > Regardless, trying to use bitmaps for this was absolutely not the > right thing to ever do. It looks like gcc randomly started complaining > when 'bitmap_from_u64()' was changed, but it was buggy before that > too. Right, thanks for fixing it. About some of the changes perhaps we should use BIT when declaring values in enum hci_conn_flags?
On Mon, Jun 6, 2022 at 11:00 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Right, thanks for fixing it. About some of the changes perhaps we > should use BIT when declaring values in enum hci_conn_flags? That sounds sane, although with just two flag values I'm not sure it matters. But I guess it would document the fact that it's a bitmask, not an ordinal value, and it looks like that header is already using BIT() elsewhere so there are no new header file dependencies.. Linus
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 5abb2ca5b129..2de7e1ec4035 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); + bitmap_from_arr32(entry->flags, &flags, 32); list_add(&entry->list, list); diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 74937a834648..b63025c70c2c 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -4519,7 +4519,8 @@ 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); + bitmap_from_arr32(br_params->flags, ¤t_flags, + __HCI_CONN_NUM_FLAGS); status = MGMT_STATUS_SUCCESS; } else { bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)", @@ -4531,7 +4532,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data, if (params) { DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS); - bitmap_from_u64(flags, current_flags); + bitmap_from_arr32(flags, ¤t_flags, __HCI_CONN_NUM_FLAGS); /* Devices using RPAs can only be programmed in the * acceptlist LL Privacy has been enable otherwise they @@ -4546,7 +4547,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data, goto unlock; } - bitmap_from_u64(params->flags, current_flags); + bitmap_from_arr32(params->flags, ¤t_flags, __HCI_CONN_NUM_FLAGS); status = MGMT_STATUS_SUCCESS; /* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
The commit 0a97953fd221 ("lib: add bitmap_{from,to}_arr64") changed implementation of bitmap_from_u64(), so that it doesn't typecast argument to u64, and actually dereferences memory. With that change, compiler spotted few places in bluetooth code where bitmap_from_u64 is called for 32-bit variable. As reported by Sudip Mukherjee: "arm allmodconfig" fails with the error: In file included from ./include/linux/string.h:253, from ./include/linux/bitmap.h:11, from ./include/linux/cpumask.h:12, from ./include/linux/smp.h:13, from ./include/linux/lockdep.h:14, from ./include/linux/mutex.h:17, from ./include/linux/rfkill.h:35, from net/bluetooth/hci_core.c:29: In function 'fortify_memcpy_chk', inlined from 'bitmap_copy' at ./include/linux/bitmap.h:254:2, inlined from 'bitmap_copy_clear_tail' at ./include/linux/bitmap.h:263:2, inlined from 'bitmap_from_u64' at ./include/linux/bitmap.h:540:2, inlined from 'hci_bdaddr_list_add_with_flags' at net/bluetooth/hci_core.c:2156:2: ./include/linux/fortify-string.h:344:25: error: call to '__write_overflow_field' declared with attribute warning: +detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] 344 | __write_overflow_field(p_size_field, size); And, "csky allmodconfig" fails with the error: In file included from ./include/linux/cpumask.h:12, from ./include/linux/mm_types_task.h:14, from ./include/linux/mm_types.h:5, from ./include/linux/buildid.h:5, from ./include/linux/module.h:14, from net/bluetooth/mgmt.c:27: In function 'bitmap_copy', inlined from 'bitmap_copy_clear_tail' at ./include/linux/bitmap.h:263:2, inlined from 'bitmap_from_u64' at ./include/linux/bitmap.h:540:2, inlined from 'set_device_flags' at net/bluetooth/mgmt.c:4534:4: ./include/linux/bitmap.h:254:9: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] of object 'flags' +with type 'long unsigned int[1]' [-Werror=array-bounds] 254 | memcpy(dst, src, len); | ^~~~~~~~~~~~~~~~~~~~~ In file included from ./include/linux/kasan-checks.h:5, from ./include/asm-generic/rwonce.h:26, from ./arch/csky/include/generated/asm/rwonce.h:1, from ./include/linux/compiler.h:248, from ./include/linux/build_bug.h:5, from ./include/linux/container_of.h:5, from ./include/linux/list.h:5, from ./include/linux/module.h:12, from net/bluetooth/mgmt.c:27: net/bluetooth/mgmt.c: In function 'set_device_flags': net/bluetooth/mgmt.c:4532:40: note: 'flags' declared here 4532 | DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS); | ^~~~~ ./include/linux/types.h:11:23: note: in definition of macro 'DECLARE_BITMAP' 11 | unsigned long name[BITS_TO_LONGS(bits)] Fix it by replacing bitmap_from_u64 with bitmap_from_arr32. Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> Signed-off-by: Yury Norov <yury.norov@gmail.com> --- net/bluetooth/hci_core.c | 2 +- net/bluetooth/mgmt.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-)