Message ID | 20220516203539.1708251-1-luiz.dentz@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2] Bluetooth: MGMT: Fix uses of bitmap_from_u64 | expand |
Hi Luiz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on next-20220516]
[cannot apply to bluetooth/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-MGMT-Fix-uses-of-bitmap_from_u64/20220517-050324
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: hexagon-randconfig-r041-20220516 (https://download.01.org/0day-ci/archive/20220517/202205171145.9ekbbpAa-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/15b968f930769afbaabbfb85c2cefcbc30506a7b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Luiz-Augusto-von-Dentz/Bluetooth-MGMT-Fix-uses-of-bitmap_from_u64/20220517-050324
git checkout 15b968f930769afbaabbfb85c2cefcbc30506a7b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> net/bluetooth/mgmt.c:7113:7: warning: variable 'params' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (err)
^~~
net/bluetooth/mgmt.c:7163:9: note: uninitialized use occurs here
params ? params->flags : NULL);
^~~~~~
net/bluetooth/mgmt.c:7113:3: note: remove the 'if' if its condition is always true
if (err)
^~~~~~~~
net/bluetooth/mgmt.c:7082:32: note: initialize the variable 'params' to silence this warning
struct hci_conn_params *params;
^
= NULL
1 warning generated.
vim +7113 net/bluetooth/mgmt.c
e8907f76544ffe Luiz Augusto von Dentz 2021-10-27 7076
2faade53e65f27 Marcel Holtmann 2014-06-29 7077 static int add_device(struct sock *sk, struct hci_dev *hdev,
2faade53e65f27 Marcel Holtmann 2014-06-29 7078 void *data, u16 len)
2faade53e65f27 Marcel Holtmann 2014-06-29 7079 {
2faade53e65f27 Marcel Holtmann 2014-06-29 7080 struct mgmt_cp_add_device *cp = data;
2faade53e65f27 Marcel Holtmann 2014-06-29 7081 u8 auto_conn, addr_type;
4c54bf2b093bb2 Abhishek Pandit-Subedi 2020-06-17 7082 struct hci_conn_params *params;
2faade53e65f27 Marcel Holtmann 2014-06-29 7083 int err;
2faade53e65f27 Marcel Holtmann 2014-06-29 7084
181d695352305c Marcel Holtmann 2020-05-06 7085 bt_dev_dbg(hdev, "sock %p", sk);
2faade53e65f27 Marcel Holtmann 2014-06-29 7086
6659358efe617b Johan Hedberg 2014-07-09 7087 if (!bdaddr_type_is_valid(cp->addr.type) ||
2faade53e65f27 Marcel Holtmann 2014-06-29 7088 !bacmp(&cp->addr.bdaddr, BDADDR_ANY))
2a1afb5ac8d580 Johan Hedberg 2015-03-06 7089 return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
2faade53e65f27 Marcel Holtmann 2014-06-29 7090 MGMT_STATUS_INVALID_PARAMS,
2faade53e65f27 Marcel Holtmann 2014-06-29 7091 &cp->addr, sizeof(cp->addr));
2faade53e65f27 Marcel Holtmann 2014-06-29 7092
4b9e7e7516135b Marcel Holtmann 2014-07-23 7093 if (cp->action != 0x00 && cp->action != 0x01 && cp->action != 0x02)
2a1afb5ac8d580 Johan Hedberg 2015-03-06 7094 return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
2faade53e65f27 Marcel Holtmann 2014-06-29 7095 MGMT_STATUS_INVALID_PARAMS,
2faade53e65f27 Marcel Holtmann 2014-06-29 7096 &cp->addr, sizeof(cp->addr));
2faade53e65f27 Marcel Holtmann 2014-06-29 7097
2faade53e65f27 Marcel Holtmann 2014-06-29 7098 hci_dev_lock(hdev);
2faade53e65f27 Marcel Holtmann 2014-06-29 7099
6659358efe617b Johan Hedberg 2014-07-09 7100 if (cp->addr.type == BDADDR_BREDR) {
4b9e7e7516135b Marcel Holtmann 2014-07-23 7101 /* Only incoming connections action is supported for now */
6659358efe617b Johan Hedberg 2014-07-09 7102 if (cp->action != 0x01) {
51d7a94d56f842 Johan Hedberg 2015-11-11 7103 err = mgmt_cmd_complete(sk, hdev->id,
51d7a94d56f842 Johan Hedberg 2015-11-11 7104 MGMT_OP_ADD_DEVICE,
51d7a94d56f842 Johan Hedberg 2015-11-11 7105 MGMT_STATUS_INVALID_PARAMS,
51d7a94d56f842 Johan Hedberg 2015-11-11 7106 &cp->addr, sizeof(cp->addr));
6659358efe617b Johan Hedberg 2014-07-09 7107 goto unlock;
6659358efe617b Johan Hedberg 2014-07-09 7108 }
6659358efe617b Johan Hedberg 2014-07-09 7109
3d4f9c00492b4e Archie Pusaka 2021-06-04 7110 err = hci_bdaddr_list_add_with_flags(&hdev->accept_list,
8baaa4038edbff Abhishek Pandit-Subedi 2020-06-17 7111 &cp->addr.bdaddr,
8baaa4038edbff Abhishek Pandit-Subedi 2020-06-17 7112 cp->addr.type, 0);
6659358efe617b Johan Hedberg 2014-07-09 @7113 if (err)
6659358efe617b Johan Hedberg 2014-07-09 7114 goto unlock;
a397407f266f8d Johan Hedberg 2014-07-09 7115
01b1cb87d37fb1 Johan Hedberg 2015-11-16 7116 hci_req_update_scan(hdev);
a397407f266f8d Johan Hedberg 2014-07-09 7117
6659358efe617b Johan Hedberg 2014-07-09 7118 goto added;
6659358efe617b Johan Hedberg 2014-07-09 7119 }
6659358efe617b Johan Hedberg 2014-07-09 7120
85813a7ec774b9 Johan Hedberg 2015-10-21 7121 addr_type = le_addr_type(cp->addr.type);
2faade53e65f27 Marcel Holtmann 2014-06-29 7122
4b9e7e7516135b Marcel Holtmann 2014-07-23 7123 if (cp->action == 0x02)
2faade53e65f27 Marcel Holtmann 2014-06-29 7124 auto_conn = HCI_AUTO_CONN_ALWAYS;
4b9e7e7516135b Marcel Holtmann 2014-07-23 7125 else if (cp->action == 0x01)
4b9e7e7516135b Marcel Holtmann 2014-07-23 7126 auto_conn = HCI_AUTO_CONN_DIRECT;
2faade53e65f27 Marcel Holtmann 2014-06-29 7127 else
a3451d279f839d Johan Hedberg 2014-07-02 7128 auto_conn = HCI_AUTO_CONN_REPORT;
2faade53e65f27 Marcel Holtmann 2014-06-29 7129
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7130 /* Kernel internally uses conn_params with resolvable private
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7131 * address, but Add Device allows only identity addresses.
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7132 * Make sure it is enforced before calling
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7133 * hci_conn_params_lookup.
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7134 */
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7135 if (!hci_is_identity_address(&cp->addr.bdaddr, addr_type)) {
51d7a94d56f842 Johan Hedberg 2015-11-11 7136 err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
51d7a94d56f842 Johan Hedberg 2015-11-11 7137 MGMT_STATUS_INVALID_PARAMS,
51d7a94d56f842 Johan Hedberg 2015-11-11 7138 &cp->addr, sizeof(cp->addr));
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7139 goto unlock;
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7140 }
9a0a8a8e852dab Jakub Pawlowski 2015-07-20 7141
bf5b3c8be07905 Marcel Holtmann 2014-06-30 7142 /* If the connection parameters don't exist for this device,
bf5b3c8be07905 Marcel Holtmann 2014-06-30 7143 * they will be created and configured with defaults.
bf5b3c8be07905 Marcel Holtmann 2014-06-30 7144 */
51d7a94d56f842 Johan Hedberg 2015-11-11 7145 if (hci_conn_params_set(hdev, &cp->addr.bdaddr, addr_type,
d06b50ce14119a Marcel Holtmann 2014-07-01 7146 auto_conn) < 0) {
51d7a94d56f842 Johan Hedberg 2015-11-11 7147 err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
51d7a94d56f842 Johan Hedberg 2015-11-11 7148 MGMT_STATUS_FAILED, &cp->addr,
51d7a94d56f842 Johan Hedberg 2015-11-11 7149 sizeof(cp->addr));
2faade53e65f27 Marcel Holtmann 2014-06-29 7150 goto unlock;
4c54bf2b093bb2 Abhishek Pandit-Subedi 2020-06-17 7151 } else {
4c54bf2b093bb2 Abhishek Pandit-Subedi 2020-06-17 7152 params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
4c54bf2b093bb2 Abhishek Pandit-Subedi 2020-06-17 7153 addr_type);
2faade53e65f27 Marcel Holtmann 2014-06-29 7154 }
2faade53e65f27 Marcel Holtmann 2014-06-29 7155
e8907f76544ffe Luiz Augusto von Dentz 2021-10-27 7156 err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL);
e8907f76544ffe Luiz Augusto von Dentz 2021-10-27 7157 if (err < 0)
e8907f76544ffe Luiz Augusto von Dentz 2021-10-27 7158 goto unlock;
51d7a94d56f842 Johan Hedberg 2015-11-11 7159
6659358efe617b Johan Hedberg 2014-07-09 7160 added:
8afef092a192cb Marcel Holtmann 2014-06-29 7161 device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
4c54bf2b093bb2 Abhishek Pandit-Subedi 2020-06-17 7162 device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
15b968f930769a Luiz Augusto von Dentz 2022-05-16 7163 params ? params->flags : NULL);
8afef092a192cb Marcel Holtmann 2014-06-29 7164
51d7a94d56f842 Johan Hedberg 2015-11-11 7165 err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
51d7a94d56f842 Johan Hedberg 2015-11-11 7166 MGMT_STATUS_SUCCESS, &cp->addr,
51d7a94d56f842 Johan Hedberg 2015-11-11 7167 sizeof(cp->addr));
2faade53e65f27 Marcel Holtmann 2014-06-29 7168
2faade53e65f27 Marcel Holtmann 2014-06-29 7169 unlock:
2faade53e65f27 Marcel Holtmann 2014-06-29 7170 hci_dev_unlock(hdev);
2faade53e65f27 Marcel Holtmann 2014-06-29 7171 return err;
2faade53e65f27 Marcel Holtmann 2014-06-29 7172 }
2faade53e65f27 Marcel Holtmann 2014-06-29 7173
Hi Luiz, > bitmap_from_u64 expects at least 8 bytes to be declared since it doesn't > take any parameter regarding the number of bits causing the following > warnings: > > 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:270:2, > inlined from 'bitmap_from_u64' at include/linux/bitmap.h:622:2, > inlined from 'set_device_flags' at net/bluetooth/mgmt.c:4534:4: > include/linux/bitmap.h:261:9: warning: 'memcpy' forming offset [4, 7] is > out of the bounds [0, 4] of object 'flags' with type > 'long unsigned int[1]' [-Warray-bounds] > 261 | memcpy(dst, src, len); > | ^~~~~~~~~~~~~~~~~~~~~ > In file included from include/linux/kasan-checks.h:5, > from include/asm-generic/rwonce.h:26, > from ./arch/arm/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)] > | ^~~~ > > In order to fix the above this initializes a variable using > DECLARE_BITMAP with the current_flags and then uses bitmap_subset to > check if the flags being set are a subset of hdev->conn_flags that way > all the checks are performed using bitmap APIs and conversion to u32 > only happen when really needed. > > Fixes: a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting HCI_CONN_FLAG_REMOTE_WAKEUP") > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Suggested-by: Yury Norov <yury.norov@gmail.com> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > net/bluetooth/mgmt.c | 43 ++++++++++++++++++------------------------- > 1 file changed, 18 insertions(+), 25 deletions(-) what is up with the kbot issues? Regards Marcel
Hi Marcel, On Thu, May 19, 2022 at 11:09 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > bitmap_from_u64 expects at least 8 bytes to be declared since it doesn't > > take any parameter regarding the number of bits causing the following > > warnings: > > > > 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:270:2, > > inlined from 'bitmap_from_u64' at include/linux/bitmap.h:622:2, > > inlined from 'set_device_flags' at net/bluetooth/mgmt.c:4534:4: > > include/linux/bitmap.h:261:9: warning: 'memcpy' forming offset [4, 7] is > > out of the bounds [0, 4] of object 'flags' with type > > 'long unsigned int[1]' [-Warray-bounds] > > 261 | memcpy(dst, src, len); > > | ^~~~~~~~~~~~~~~~~~~~~ > > In file included from include/linux/kasan-checks.h:5, > > from include/asm-generic/rwonce.h:26, > > from ./arch/arm/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)] > > | ^~~~ > > > > In order to fix the above this initializes a variable using > > DECLARE_BITMAP with the current_flags and then uses bitmap_subset to > > check if the flags being set are a subset of hdev->conn_flags that way > > all the checks are performed using bitmap APIs and conversion to u32 > > only happen when really needed. > > > > Fixes: a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting HCI_CONN_FLAG_REMOTE_WAKEUP") > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > > Suggested-by: Yury Norov <yury.norov@gmail.com> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > net/bluetooth/mgmt.c | 43 ++++++++++++++++++------------------------- > > 1 file changed, 18 insertions(+), 25 deletions(-) > > what is up with the kbot issues? Will fix that. > Regards > > Marcel >
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 74937a834648..24a779b706b5 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -4476,9 +4476,16 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data, static void device_flags_changed(struct sock *sk, struct hci_dev *hdev, bdaddr_t *bdaddr, u8 bdaddr_type, - u32 supported_flags, u32 current_flags) + unsigned long *flags) { struct mgmt_ev_device_flags_changed ev; + u32 supported_flags, current_flags = 0; + + bitmap_to_arr32(&supported_flags, hdev->conn_flags, + __HCI_CONN_NUM_FLAGS); + + if (flags) + bitmap_to_arr32(¤t_flags, flags, __HCI_CONN_NUM_FLAGS); bacpy(&ev.addr.bdaddr, bdaddr); ev.addr.type = bdaddr_type; @@ -4495,19 +4502,15 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data, struct bdaddr_list_with_flags *br_params; struct hci_conn_params *params; u8 status = MGMT_STATUS_INVALID_PARAMS; - u32 supported_flags; u32 current_flags = __le32_to_cpu(cp->current_flags); + DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS) = { current_flags }; bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x", - &cp->addr.bdaddr, cp->addr.type, - __le32_to_cpu(current_flags)); + &cp->addr.bdaddr, cp->addr.type, current_flags); - bitmap_to_arr32(&supported_flags, hdev->conn_flags, - __HCI_CONN_NUM_FLAGS); - - if ((supported_flags | current_flags) != supported_flags) { - bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)", - current_flags, supported_flags); + if (bitmap_subset(hdev->conn_flags, flags, __HCI_CONN_NUM_FLAGS)) { + bt_dev_warn(hdev, "Bad flag given (0x%lx) vs supported (0x%lx)", + *hdev->conn_flags, *flags); goto done; } @@ -4519,7 +4522,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_copy(br_params->flags, flags, + __HCI_CONN_NUM_FLAGS); status = MGMT_STATUS_SUCCESS; } else { bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)", @@ -4529,10 +4533,6 @@ 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. @@ -4546,7 +4546,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_copy(params->flags, flags, __HCI_CONN_NUM_FLAGS); status = MGMT_STATUS_SUCCESS; /* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY @@ -4568,7 +4568,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data, done: if (status == MGMT_STATUS_SUCCESS) device_flags_changed(sk, hdev, &cp->addr.bdaddr, cp->addr.type, - supported_flags, current_flags); + flags); return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_FLAGS, status, &cp->addr, sizeof(cp->addr)); @@ -7081,8 +7081,6 @@ static int add_device(struct sock *sk, struct hci_dev *hdev, u8 auto_conn, addr_type; struct hci_conn_params *params; int err; - u32 current_flags = 0; - u32 supported_flags; bt_dev_dbg(hdev, "sock %p", sk); @@ -7153,9 +7151,6 @@ static int add_device(struct sock *sk, struct hci_dev *hdev, } else { params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr, addr_type); - if (params) - bitmap_to_arr32(¤t_flags, params->flags, - __HCI_CONN_NUM_FLAGS); } err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL); @@ -7164,10 +7159,8 @@ 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); device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type, - supported_flags, current_flags); + params ? params->flags : NULL); err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE, MGMT_STATUS_SUCCESS, &cp->addr,